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

Add a data_help method in Form #26332

Merged
merged 35 commits into from Mar 27, 2018

Conversation

@mpiot
Contributor

mpiot commented Feb 27, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26331
License MIT
Doc PR symfony/symfony-docs#9361

Add a form_help method in twig to display a help message in form. A help keyword is added to all FormType to define the message.

@stof

This is missing tests

{% block form_help -%}
{% if form.vars.help is not empty %}
<small class="form-text text-muted">{{ form.vars.help|raw }}</small>

This comment has been minimized.

@stof

stof Feb 27, 2018

Member

displaying it raw is a bad idea IMO. It opens the door to XSS.

This comment has been minimized.

@mpiot

mpiot Feb 27, 2018

Contributor

Mmm, is it really a problem ? Because it's set in the FormType by the dev, not by a user. Then, if we want use html in the help text without raw, we can't. I don't really know

This comment has been minimized.

@stof

stof Feb 27, 2018

Member

Well, requiring to perform HTML escaping when setting the option is insecure IMO. It requires all devs to be aware that this options requires escaping when setting it. And it might totally be set based on some user input (coming from a previous configuration elsewhere) in some cases.

Btw, I would add translation support for the help text instead of requiring to translate it in the form type when setting the option.

This comment has been minimized.

@mpiot

mpiot Feb 27, 2018

Contributor

Ok, I remove it from Symfony, if someone want to change it, just to extend the Template and add the raw.

By translation you mean use |trans in Twig ?

This comment has been minimized.

@stof

stof Feb 27, 2018

Member

yes, as done for labels or other things

{# Help #}
{% block form_help -%}
{% if form.vars.help is not empty %}

This comment has been minimized.

@stof

stof Feb 27, 2018

Member

just help

This comment has been minimized.

@mpiot

mpiot Feb 27, 2018

Contributor

Thx for the shortcut, I edit it

@mpiot

This comment has been minimized.

Contributor

mpiot commented Feb 27, 2018

Yes, I fix existing tests, and wite new (I've never do it, it take a little times, sorry...)

@Nyholm

Great. Thank you for contributing this.

{% block form_help -%}
{% if help is not empty %}
<small class="form-text text-muted">{{ help|trans }}</small>

This comment has been minimized.

@Nyholm

Nyholm Feb 27, 2018

Member

Let's make sure it respects the translation domain.

{{ translation_domain is same as(false) ? help : help|trans({}, translation_domain) }}

This comment has been minimized.

@mpiot

mpiot Feb 27, 2018

Contributor

Okay, I edit it. (I don't really know what is it, but I trust you ;-))

This comment has been minimized.

@Nyholm

Nyholm Feb 27, 2018

Member

It checks if translation_domain is false, it it is we just print help. If it is not false we use translation with that domain.

This comment has been minimized.

@mpiot

mpiot Feb 27, 2018

Contributor

I was thinking about TranslationDomain, I don't know what it is, I'd watch.

This comment has been minimized.

@Nyholm

Nyholm Feb 27, 2018

Member

Have a look here: http://symfony.com/doc/current/components/translation.html#using-message-domains

It is basically a way to categorize translation messages.

Make sure you add this fix every time you print help. (ie in the form_div_layout.html.twig)

This comment has been minimized.

@mpiot

mpiot Feb 27, 2018

Contributor

Thanks, and it's done :-) (it's interresting to do PR, it permit to learn lot of things:-))

@mpiot

This comment has been minimized.

Contributor

mpiot commented Feb 27, 2018

Just a question, how can I execute tests ? Because when I do:

COMPOSER_ROOT_VERSION=4.0 composer install
vendor/bin/simple-phpunit src/Symfony/Bridge/Twig/Tests

It return to me alle is ok, but Skip a lot of tests, but in TravisCi, it doesn't skip test and there is a lot of error, I can't try and push everytime to see what say Travis :-(

I've try to do something like travis do:

cd src/Symfony/Bridge/Twig
composer update --no-progress --ansi --prefer-lowest --prefer-stable
SYMFONY_ROOT_DIR/phpunit --exclude-group tty,benchmark,intl-data

But it does less tests, Skip all, and thrw an exception:

Other deprecation notices (1)
1x: The each() function is deprecated. This message will be suppressed on further calls

Thanks a lot, and sorry, it's the first tim I'm looking into tests... (I know, it's wrong... :p)

@Nyholm

This comment has been minimized.

Member

Nyholm commented Feb 27, 2018

I usually do ./phpunit src/Symfony/Bridge/Twig. That should not skip the tests.

Here is a page that describes this in details: https://symfony.com/doc/current/contributing/code/tests.html

@Nyholm

This comment has been minimized.

Member

Nyholm commented Feb 27, 2018

Btw, I see that it is the "deps low" test that fails on Travis. That is okey for this PR since the changes on the different components depends on each other.

@mpiot

This comment has been minimized.

Contributor

mpiot commented Feb 27, 2018

Oki, but I thought it would be nice to add tests for the new elements, right?

Test the form_help output, and then, maybe it change something form the form_row, no ?

@Nyholm

This comment has been minimized.

Member

Nyholm commented Feb 27, 2018

That would be excellent.

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Feb 27, 2018

I'm pinging some of our biggest Symfony Form experts so they can double check these changes: @HeahDude, @vudaltsov, @yceruto Thanks!

@mpiot

This comment has been minimized.

Contributor

mpiot commented Feb 27, 2018

Thank you :-)
Because I'm not very comfortable with Symfony Form, I'm just discovering the insides.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Feb 27, 2018

@vudaltsov

Great PR, I like it.
Since we support Foundation framework, we should probably add form_help there too.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Feb 28, 2018

Excellent. This looks real good.

(Bootstrap 4):
screen shot 2018-02-28 at 19 00 09


There is one thing I'm missing. You need to add an id to the help block (id="{{id}}_help) and use the aria-describedby on the <input>.

<label for="inputPassword5">Password</label>
<input type="password" id="inputPassword5" class="form-control" aria-describedby="passwordHelpBlock">
<small id="passwordHelpBlock" class="form-text text-muted">
  Your password must be 8-20 characters long, contain letters and numbers, and must not contain spaces, special characters, or emoji.
</small>
@mpiot

This comment has been minimized.

Contributor

mpiot commented Feb 28, 2018

@Nyholm I fix it tomorrow :-)

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 1, 2018

Good. We also need the the aria-describedby on the .

After that I’m very happy.

@mpiot

This comment has been minimized.

Contributor

mpiot commented Mar 1, 2018

@Nyholm This is done, just a little reflexion to avoid add the aria-describedBy, when there is not helpBlock. And some difficulties in the FrameworkBundle views, I don't know wwhat is its usage, but I don't like it :p

@Nyholm

Good job. I did a much more thorough review now.

Could you also make sure fabbot is happy?

{% block form_help -%}
{% if help is not empty %}
<small id="{{ id }}HelpBlock" class="form-text text-muted">{{ translation_domain is same as(false) ? help : help|trans({}, translation_domain) }}</small>

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

id generated by symfony is snake_case. I suggest changing this to {{ id }}_help

@@ -297,7 +305,8 @@
<div>
{{- form_label(form) -}}
{{- form_errors(form) -}}
{{- form_widget(form) -}}
{{- form_widget(form, { 'helpBlockDisplayed': true }) -}}

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Couldn't we check if help is not empty here?
That would make the logic in widget_attributes simpler

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

The problem is when a user just display form_widget for exemple, the form_help is not call, then we don't have to display the aria-describedBy, because the block doesn't exists.

That's why I do the test later, else, user say if he declare the form_help to do the link between help block and input.

In the row, I do it automatically, because I'm sure the help block is displayed.

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

I see. Could we try something like:

{{- form_widget(form, { 'helpBlockDisplayed': (help is not empty) }) -}} or that may be a syntax error.

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

It's the same problem, we can't do it automatically, the help may be set, but don't displayed if the user do:

{{ form_label(form.name) }}
{{ form_errors(form.name) }}
{{ form_widget(form.name) }}

He doesn't call form_label, then help is not displayed, but set in the form.

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Yeah, But it that a problem?

If the user do:

{{ form_label(form.name) }}
{{ form_errors(form.name) }}
{{ form_widget(form.name) }}

Then no form help will be rendered and no aria-describedBy will be used.


If the user do:

{{ form_label(form.name) }}
{{ form_errors(form.name) }}
{{ form_widget(form.name) }}
{{ form_help(form.name) }}

Then form help will be rendered but no aria-describedBy will be used.


We should only make sure that the logic is clean and when the user do:

{{ form_row(form.name) }}

Then form help will be rendered AND aria-describedBy will be used.

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

Yes, we can do that, I've do it to permit the user to add the aria-describedBy when don't use form_row. Then, I'm not really sure it's the best way to do, the method must work for all.

@@ -384,6 +393,7 @@
id="{{ id }}" name="{{ full_name }}"
{%- if disabled %} disabled="disabled"{% endif -%}
{%- if required %} required="required"{% endif -%}
{%- if helpBlockDisplayed is defined and helpBlockDisplayed and help is not empty %} aria-describedby="{{ id }}HelpBlock"{% endif -%}

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Replace and helpBlockDisplayed and with and helpBlockDisplayed same as(true) and

@@ -0,0 +1,3 @@
<?php if (!empty($help)): ?>
<p id="<?php echo $view->escape($id) ?>HelpBlock" class="help-text"><?php echo $view->escape(false !== $translation_domain ? $view['translator']->trans($help, array(), $translation_domain) : $help) ?></p>

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

_help instead of HelpBlock

@@ -1,3 +1,4 @@
id="<?php echo $view->escape($id) ?>" name="<?php echo $view->escape($full_name) ?>"<?php if ($disabled): ?> disabled="disabled"<?php endif ?>
<?php if ($required): ?> required="required"<?php endif ?>
<?php echo $attr ? ' '.$view['form']->block($form, 'attributes') : '' ?>
<?php if (isset($helpBlockDisplayed) && $helpBlockDisplayed && !empty($help)): ?> aria-describedby="<?php echo $view->escape($id) ?>HelpBlock"<?php endif ?>

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

&& $helpBlockDisplayed && => && true === $helpBlockDisplayed &&

@@ -177,10 +178,12 @@ public function configureOptions(OptionsResolver $resolver)
'attr' => array(),
'post_max_size_message' => 'The uploaded file was too large. Please try to upload a smaller file.',
'upload_max_size_message' => $uploadMaxSizeMessage, // internal
'help' => '',

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Should the default be an empty string or null? Spontaneously I would say null, but Im sure there is a reason you chose the empty string.

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

I've not test with null... But In the line bellow, we must define the accepted type that is string. That's why I've put an empty string. But Maybe it works well with null, I can try it. Tests will scream if it's not good.

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

I confirm, if default is null, it trhown error because it expect a string value.

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Okey, Could we make it expect a string value and null? Or maybe false would make more sense.

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

I can try if I can give him an array of possibilities, but I don't know if it works, I don't know this part of the code.

This comment has been minimized.

@mpiot

mpiot Mar 1, 2018

Contributor

It's okay :-)

$resolver->setAllowedTypes('help', ['string', 'NULL']);

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Great

$this->assertMatchesXpath($html,
'/span
[@id="nameHelpBlock"]

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Update id

$this->assertMatchesXpath($html,
'/p
[@id="nameHelpBlock"]

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

Update id

@mpiot

This comment has been minimized.

Contributor

mpiot commented Mar 1, 2018

@Nyholm I've fixed some of your suggestions :-) Mmm, "Could you also make sure fabbot is happy?", how I do that ? :p

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 1, 2018

Thanks!

Fabbot is a script that make sure all PRs follow code standard and it also runs some test for "common things". It is real great.

See the PR status.
screen shot 2018-03-01 at 09 49 42 copy

You see that there are some things fabbot complains about. Just copy the command and fabbot will fix things for you.

screen shot 2018-03-01 at 09 51 2

@mpiot

This comment has been minimized.

Contributor

mpiot commented Mar 1, 2018

Oh yes, I've see that, but the correction, isn't the way used in all the other files... That's why I've not fix it and keep the way used in existing files.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 1, 2018

No, you can trust the fabbot. It does only correct/review the files you've edited in this PR.

@mpiot

This comment has been minimized.

Contributor

mpiot commented Mar 1, 2018

For exemple it say to fix:

<?php echo $view['form']->label($form) ?>
<?php echo $view['form']->errors($form) ?>
<?php echo $view['form']->widget($form, array('helpBlockDisplayed' => true)) ?>
<?php echo $view['form']->help($form) ?>

By:

<?php echo $view['form']->label($form); ?>
<?php echo $view['form']->errors($form); ?>
<?php echo $view['form']->widget($form, array('helpBlockDisplayed' => true)); ?>
<?php echo $view['form']->help($form); ?>

I've not edited other lines, just add one. That's why I've keep the syntax without the ; at end.

It's the same everywhere, it want to add ; at end of each php block, but it's actually used nowhere in the folder src/Symfony/Bundle/FrameworkBundle/Ressources/views/Form. That's why I don't really know if I must do like fabpot.io say or do like all files are.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 1, 2018

I guess fabbot has updated its rules since those other lines were added. I think we should follow the recommendations by fabbot in this case.

@Nyholm

Could you also run the following to fix the license in the file headers:

curl https://fabbot.io/patch/symfony/symfony/26332/80c66414450cbf0148d4fac6288c83a8b72172ae/license.diff | patch -p0
@@ -92,6 +92,7 @@
<div class="form-group{% if (not compound or force_error|default(false)) and not valid %} has-error{% endif %}">
{{- form_label(form) -}}
{{- form_widget(form) -}}

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

We should make sure aria-describedby is enabled here

@@ -218,6 +218,7 @@
<{{ element|default('div') }} class="form-group">
{{- form_label(form) -}}
{{- form_widget(form) -}}

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

We should make sure aria-describedby is enabled here

@@ -271,6 +271,7 @@
<div class="large-12 columns{% if (not compound or force_error|default(false)) and not valid %} error{% endif %}">
{{ form_label(form) }}
{{ form_widget(form) }}

This comment has been minimized.

@Nyholm

Nyholm Mar 1, 2018

Member

We should make sure aria-describedby is enabled here

@mpiot

This comment has been minimized.

Contributor

mpiot commented Mar 1, 2018

It bugs.... The diff is empty, the diff file doesn't exists and I've already apply the patch to add the licence header.

@fabpot

fabpot approved these changes Mar 27, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 27, 2018

Thank you @mpiot.

@fabpot fabpot merged commit 585ca28 into symfony:master Mar 27, 2018

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Mar 27, 2018

feature #26332 Add a data_help method in Form (mpiot, Nyholm)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Add a data_help method in Form

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26331
| License       | MIT
| Doc PR        | symfony/symfony-docs#9361

Add a form_help method in twig to display a help message in form. A `help` keyword is added to all FormType to define the message.

Commits
-------

585ca28 Add return type hint
859ee03 Revert: remove comment line from twig templates
d723756 Fix some mistakes
c74e0dc Use spaceless balises in Twig templates
8b937ff Try without try/catch
32bf1f6 Test the renderHelp method in all Tests about help to skip them if necessary.
437b77e Skip renderHelp test as skipped if not override
d84be70 Update composer files
075fcfd [FrameworkBundle] Add widgetAtt to formTable/form_row
f1d13a8 Fix Fabpot.io
69ded67 Added form_help on horizontal design and removed special variable
fd53bc5 Enable aria-described in row for all Templates
98065d3 fabpot.io fix
edb95f8 Use array long syntax
aada72c Set help option on nul as default
f948147 Rename help id (snake_case)
77fa317 Fix Test
30deaa9 PSR fix
bf4d08c Add aria-describedBy on input
1f3a15e Rename id
058489d Add an id to the help
6ea7a20 Remove vars option from form_help
ba798df FrameworkBundle Tests
4f2581d Use array long syntax
f15bc79 Fix coding standards
c934e49 Add test without help set
8094804 Add Tests
067c681 Template for table, Foundation and Bootstrap 3
d3e3e49 Fix: check translation domain
2c2c045 Adapt existant tests
831693a Add trans filter
e311838 Remove raw filter for help
8b97c1b Use a shortcut to acces help var in Twig template
1b89f9d Add a template fot div_layout
c8914f5 Add a data_help method in Form

@mpiot mpiot deleted the mpiot:form_help branch Mar 29, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 16, 2018

minor #9361 Add help option to FormType (mpiot)
This PR was squashed before being merged into the 4.1 branch (closes #9361).

Discussion
----------

Add help option to FormType

Add documentation about the new feature symfony/symfony#26332

Commits
-------

5bdf708 Add help option to FormType
@mhujer

This comment has been minimized.

Contributor

mhujer commented May 27, 2018

Maybe this should be added to UPGRADE-4.1.md as well? Because when you have your own Form Type Extension to handle help attribute, it starts to render twice in the form.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 27, 2018

Would you like to open this PR?

mhujer added a commit to mhujer/symfony that referenced this pull request May 27, 2018

mhujer added a commit to mhujer/symfony that referenced this pull request May 27, 2018

nicolas-grekas added a commit that referenced this pull request May 29, 2018

minor #27393 [Form] mention "help" field option in UPGRADE file (mhujer)
This PR was merged into the 4.1 branch.

Discussion
----------

[Form] mention "help" field option in UPGRADE file

| Q             | A
| ------------- | ---
| Branch?       | 4.1 <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT

When you have your own Form Extension to handle help option (e.g. similar to the one mentioned in the [blog post](https://symfony.com/blog/new-in-symfony-4-1-form-field-help)), it starts to render twice in the form after upgrading to 4.1 (where #26332 was added)

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

744362a update UPGRADE-4.1 for feature #26332 Form field help option

nicolas-grekas added a commit that referenced this pull request May 31, 2018

Merge branch '4.1'
* 4.1: (22 commits)
  [HttpKernel] Fix restoring trusted proxies in tests
  Update UPGRADE-4.0.md
  [Messenger] Fix suggested enqueue adapter package
  bumped Symfony version to 4.1.1
  updated VERSION for 4.1.0
  updated CHANGELOG for 4.1.0
  Insert correct parameter_bag service in AbstractController
  Revert "feature #26702 Mark ExceptionInterfaces throwable (ostrolucky)"
  CODEOWNERS: some more rules
  removed unneeded comments in tests
  removed unneeded comments in tests
  Change PHPDoc in ResponseHeaderBag::getCookies() to help IDEs
  [HttpKernel] fix registering IDE links
  update UPGRADE-4.1 for feature #26332 Form field help option
  [HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer.
  [Process] Consider \"executable\" suffixes first on Windows
  Triggering RememberMe's loginFail() when token cannot be created
  bumped Symfony version to 4.1.0
  updated VERSION for 4.1.0-BETA3
  updated CHANGELOG for 4.1.0-BETA3
  ...
@loru88

This comment has been minimized.

Contributor

loru88 commented Jun 19, 2018

Do you think it's possible to have this feature available also in Symfony 3.4?
I can't understand why this simple feature is stick just to Symfony 4

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jun 19, 2018

@loru88 We never add new features in patch versions, but only do that in minor releases. You can read more about our release process at http://symfony.com/doc/current/contributing/community/releases.html.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment