8000 [Form][TwigBridge] Add help_attr by mpiot · Pull Request #27043 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form][TwigBridge] Add help_attr #27043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 17, 2018
Merged

[Form][TwigBridge] Add help_attr #27043

merged 1 commit into from
Oct 17, 2018

Conversation

mpiot
Copy link
Contributor
@mpiot mpiot commented Apr 25, 2018
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#...

Add help_attr to the form_help method.

@@ -1,3 +1,4 @@
<?php if (!empty($help)): ?>
<p id="<?php echo $view->escape($id); ?>_help" class="help-text"><?php echo $view->escape(false !== $translation_domain ? $view['translator']->trans($help, array(), $translation_domain) : $help); ?></p>
<?php $help_attr['class'] = trim((isset($help_attr['class']) ? $help_attr['class'].' help-text' : 'help-text')); ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could apply the trimming around the concatenation only, as the other alternative is already trimmed.

@nicolas-grekas nicolas-grekas changed the title Add help_attr [Form][TwigBridge] Add help_attr Apr 25, 2018
@xabbuh
Copy link
Member
xabbuh commented Jun 30, 2018

Is this still needed given that #26332 was merged?

@mpiot
Copy link
Contributor Author
mpiot commented Jun 30, 2018

This follow the last, this PR add a 'help_attr', like 'label_attr' for exemple. That permit to configure the 'help' entry.

@xabbuh
Copy link
Member
xabbuh commented Sep 15, 2018

@mpiot LGTM, can you please rebase here?

@mpiot
Copy link
Contributor Author
mpiot commented Sep 15, 2018

@xabbuh yes, I do that on Monday ;-) And I'll see to add a part about that in the doc

@xabbuh
Copy link
Member
xabbuh commented Sep 21, 2018

We need to update the constraints for the Form component in the composer.json file of the Twig bridge to conflict with releases before 4.2.

@mpiot
Copy link
Contributor Author
mpiot commented Sep 21, 2018

Mmm, sorry I don't really know what to do :-/
Change the:
"require-dev": {
...
"symfony/form": "^4.1",
...
},
"conflict": {
...
"symfony/form": "^4.1",
...
},

I replace all 4.1 per ... ? 4.2 ? But I need to declare somewhere the version change ?

And I need to do the same in the Twig Bridge ?

I'm so sorry, I don't really know how it works.

@mpiot
Copy link
Contributor Author
mpiot commented Sep 26, 2018

@xabbuh Thanks

I try to understand errors with deps=high. On my machine when I do something like:

cd src/Symfony/Bundle/FrameworkBundle/
composer update --no-progress --no-suggest --ansi
phpunit --exclude-group tty,benchmark,intl-data

And tests are OK, I don't see the difference with Travis, because I use the same commands as in the travis test.

Edit: I try to add the form version requirement in the FrameworkBundle too

@xabbuh
Copy link
Member
xabbuh commented Oct 1, 2018

@mpiot This is a bit tricky. We use Travis to ensure that 4.1 versions of components work with 4.2 dependencies. The form theme tests are not very flexible as they mainly just compare the rendered HTML. I try to find a way to relax them a bit.

@fabpot
Copy link
Member
fabpot commented Oct 10, 2018

I've not reviewed this PR but I've spotted a merge commit in the history. @mpiot Can you rebase to get rid of it (as PR with merge commits cannot be merged)? Thank you.

@mpiot
Copy link
Contributor Author
mpiot commented Oct 16, 2018

@xabbuh thanks, so there's no way to pass the tests right now?

@xabbuh
Copy link
Member
xabbuh commented Oct 16, 2018

@mpiot I try to look into it tonight.

@xabbuh
Copy link
Member
xabbuh commented Oct 16, 2018

If I don't miss anything here, tests would pass just when we merge this into the master branch. We could skip the new tests in case the Form component is not available in a compatible version, but that would only be needed for this PR. Afterwards, FrameworkBundle as well as TwigBridge will conflict with these versions of the Form component. So IMO not worth it to me and I vote for just merging as is.

@mpiot
Copy link
Contributor Author
mpiot commented Oct 16, 2018

Thank you for the time you spent watching this :-)

I'll write the doc asap :-)

@fabpot
Copy link
Member
fabpot commented Oct 17, 2018

Thank you @mpiot.

@fabpot fabpot merged commit 42d54b7 into symfony:master Oct 17, 2018
fabpot added a commit that referenced this pull request Oct 17, 2018
This PR was merged into the 4.2-dev branch.

Discussion
----------

[Form][TwigBridge] Add help_attr

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Add help_attr to the  form_help method.

Commits
-------

42d54b7 Add help_attr
@xabbuh
Copy link
Member
xabbuh commented Oct 17, 2018

@mpiot FYI, @nicolas-grekas helped me to spot why the tests where failing (I moved the test method from the Form component to the concrete subclasses in FrameworkBundle and TwigBridge to fix it).

@mpiot
Copy link
Contributor Author
mpiot commented Oct 17, 2018

@xabbuh Thank you for this feedback, always important to know what was wrong :-)

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 18, 2018
This PR was squashed before being merged into the master branch (closes #10511).

Discussion
----------

[FormType] Add help_attr documentation

Add documentation about the new feature symfony/symfony#27043

Commits
-------

f6162bc [FormType] Add help_attr documentation
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@xabbuh xabbuh xabbuh approved these changes

Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0