Movatterモバイル変換

stof reviewed

Copy link

Member

stof left a comment

Choose a reason for hiding this comment

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

This is missing tests

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig Outdated


		{%blockform_help -%}
		{%ifform.vars.helpis notempty %}
		<smallclass="form-text text-muted">{{form.vars.help\|raw }}</small>

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

stofFeb 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

mpiotFeb 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link

Member

stofFeb 27, 2018

Choose a reason for hiding this comment

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

yes, as done for labels or other things

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig Outdated

		{# Help#}

		{%blockform_help -%}
		{%ifform.vars.helpis notempty %}

Copy link

Member

stofFeb 27, 2018

Choose a reason for hiding this comment

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

justhelp

Copy link

ContributorAuthor

mpiotFeb 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Thx for the shortcut, I edit it

Copy link

ContributorAuthor

mpiot commentedFeb 27, 2018•
edited
Loading

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

Nyholm reviewed

Copy link

Member

Nyholm left a comment

Choose a reason for hiding this comment

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

Great. Thank you for contributing this.

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig Outdated


		{%blockform_help -%}
		{%ifhelpis notempty %}
		<smallclass="form-text text-muted">{{help\|trans }}</small>

Copy link

Member

NyholmFeb 27, 2018

Choose a reason for hiding this comment

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

Let's make sure it respects the translation domain.

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

Copy link

ContributorAuthor

mpiotFeb 27, 2018

Choose a reason for hiding this comment

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

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

Copy link

Member

NyholmFeb 27, 2018

Choose a reason for hiding this comment

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

It checks iftranslation_domain is false, it it is we just printhelp. If it is not false we use translation with that domain.

Copy link

ContributorAuthor

mpiotFeb 27, 2018

Choose a reason for hiding this comment

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

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

Copy link

Member

NyholmFeb 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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 printhelp. (ie in the form_div_layout.html.twig)

Copy link

ContributorAuthor

mpiotFeb 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

mpiot commentedFeb 27, 2018

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

COMPOSER_ROOT_VERSION=4.0 composer installvendor/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/Twigcomposer update --no-progress --ansi --prefer-lowest --prefer-stableSYMFONY_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)

Copy link

Member

Nyholm commentedFeb 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

Copy link

Member

Nyholm commentedFeb 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.

Copy link

ContributorAuthor

mpiot commentedFeb 27, 2018

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

Test theform_help output, and then, maybe it change something form theform_row, no ?

Copy link

Member

Nyholm commentedFeb 27, 2018

That would be excellent.

Copy link

Member

javiereguiluz commentedFeb 27, 2018

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

Copy link

ContributorAuthor

mpiot commentedFeb 27, 2018

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

nicolas-grekas added this to the4.1 milestone

vudaltsov suggested changes

Copy link

Contributor

vudaltsov left a comment

Choose a reason for hiding this comment

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

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

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Add help option to FormTypesymfony/symfony-docs#9361

mpiot mentioned this pull request

Feb 28, 2018

Closed

Copy link

Member

Nyholm commentedFeb 28, 2018

Excellent. This looks real good.

(Bootstrap 4):

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

<labelfor="inputPassword5">Password</label><inputtype="password"id="inputPassword5"class="form-control"aria-describedby="passwordHelpBlock"><smallid="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>

Copy link

ContributorAuthor

mpiot commentedFeb 28, 2018•
edited
Loading

@Nyholm I fix it tomorrow :-)

Copy link

Member

Nyholm commentedMar 1, 2018

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

After that I’m very happy.

Copy link

ContributorAuthor

mpiot commentedMar 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 requested changes

Mar 1, 2018

Copy link

Member

Nyholm left a comment

Choose a reason for hiding this comment

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

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

Could you also make sure fabbot is happy?

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig Outdated


		{%blockform_help -%}
		{%ifhelpis notempty %}
		<smallid="{{id }}HelpBlock"class="form-text text-muted">{{translation_domainissame as(false)?help:help\|trans({},translation_domain) }}</small>

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

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

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig Outdated

		{{- form_label(form) -}}
		{{- form_errors(form) -}}
		{{- form_widget(form) -}}
		{{- form_widget(form, {'helpBlockDisplayed':true }) -}}

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

Couldn't we check ifhelp is not empty here?
That would make the logic inwidget_attributes simpler

Copy link

ContributorAuthor

mpiotMar 1, 2018•
edited
Loading

Choose a reason for hiding this comment

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

The problem is when a user just displayform_widget for exemple, theform_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.

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

I see. Could we try something like:

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

Copy link

ContributorAuthor

mpiotMar 1, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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 callform_label, then help is not displayed, but set in the form.

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

Yeah, But it that a problem?

If the user do:

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

Thenno form help will be rendered and noaria-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 helpwill be rendered but noaria-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 helpwill be rendered ANDaria-describedBy will be used.

Copy link

ContributorAuthor

mpiotMar 1, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig Outdated

		id="{{id }}" name="{{full_name }}"
		{%-ifdisabled %} disabled="disabled"{%endif -%}
		{%-ifrequired %} required="required"{%endif -%}
		{%-ifhelpBlockDisplayedisdefinedandhelpBlockDisplayedandhelpis notempty %} aria-describedby="{{id }}HelpBlock"{%endif -%}

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

Replaceand helpBlockDisplayed and withand helpBlockDisplayed same as(true) and

src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/form_help.html.php Outdated

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

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

_help instead ofHelpBlock

src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/widget_attributes.html.php Outdated

		id="<?phpecho$view->escape($id)?>" name="<?phpecho$view->escape($full_name)?>"<?phpif ($disabled):?> disabled="disabled"<?phpendif?>
		<?phpif ($required):?> required="required"<?phpendif?>
		<?phpecho$attr ?''.$view['form']->block($form,'attributes') :''?>
		<?phpif (isset($helpBlockDisplayed) &&$helpBlockDisplayed && !empty($help)):?> aria-describedby="<?phpecho$view->escape($id)?>HelpBlock"<?phpendif?>

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

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

src/Symfony/Component/Form/Extension/Core/Type/FormType.php Outdated

		'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' =>'',

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

mpiotMar 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

mpiotMar 1, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

mpiotMar 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

mpiotMar 1, 2018

Choose a reason for hiding this comment

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

It's okay :-)

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

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

Great

src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php Outdated


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

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

Update id

src/Symfony/Component/Form/Tests/AbstractLayoutTest.php Outdated


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

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

Update id

Copy link

ContributorAuthor

mpiot commentedMar 1, 2018

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

Copy link

Member

Nyholm commentedMar 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.

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

Copy link

ContributorAuthor

mpiot commentedMar 1, 2018•
edited
Loading

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.

Copy link

Member

Nyholm commentedMar 1, 2018

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

Copy link

ContributorAuthor

mpiot commentedMar 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 foldersrc/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.

Copy link

Member

Nyholm commentedMar 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 requested changes

Mar 1, 2018

Copy link

Member

Nyholm left a comment

Choose a reason for hiding this comment

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

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

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig Outdated

		{%blockform_row -%}
		<divclass="form-group{%if (notcompoundorforce_error\|default(false))andnotvalid %} has-error{%endif %}">
		{{- form_label(form) -}}
		{{- form_widget(form) -}}

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

We should make sure aria-describedby is enabled here

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig Outdated

		{%-endif -%}
		<{{element\|default('div') }} class="form-group">
		{{- form_label(form) -}}
		{{- form_widget(form) -}}

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

We should make sure aria-describedby is enabled here

src/Symfony/Bridge/Twig/Resources/views/Form/foundation_5_layout.html.twig Outdated

		<divclass="row">
		<divclass="large-12 columns{%if (notcompoundorforce_error\|default(false))andnotvalid %} error{%endif %}">
		{{ form_label(form) }}
		{{ form_widget(form) }}

Copy link

Member

NyholmMar 1, 2018

Choose a reason for hiding this comment

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

We should make sure aria-describedby is enabled here

Copy link

ContributorAuthor

mpiot commentedMar 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.

mpiot added8 commits

March 23, 2018 15:53

Update composer files

d84be70

Skip renderHelp test as skipped if not override

437b77e

Test the renderHelp method in all Tests about help to skip them if ne…

32bf1f6

…cessary.

Try without try/catch

8b937ff

Use spaceless balises in Twig templates

c74e0dc

Fix some mistakes

d723756

Revert: remove comment line from twig templates

859ee03

Add return type hint

585ca28

carsonbot added Status: Needs Review and removed Status: Reviewed labels

fabpot approved these changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Copy link

Member

fabpot commentedMar 27, 2018

Thank you@mpiot.

fabpot merged commit585ca28 intosymfony:master

fabpot added a commit that referenced this pull request