Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Form] DeprecatesearchAndRenderBlock returning empty string#27247

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@ostrolucky
Copy link
Contributor

@ostroluckyostrolucky commentedMay 12, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#26531
LicenseMIT
Doc PR-

I would like to remove this silent behavior, because it's confusing

@nicolas-grekas
Copy link
Member

That'd be for 4.2 now.

@nicolas-grekasnicolas-grekas added this to thenext milestoneMay 13, 2018
@ostrolucky
Copy link
ContributorAuthor

This isn't new feature so shouldn't be affected by feature freeze. But understood.

@nicolas-grekas
Copy link
Member

Semver defines that deprecations should follow the same process as features.

@ostroluckyostrolucky changed the base branch from4.1 tomasterMay 15, 2018 09:04
@ostrolucky
Copy link
ContributorAuthor

Changed the base. Should I create UPGRADE-4.2.md in root now?

@nicolas-grekas
Copy link
Member

Changed the base. Should I create UPGRADE-4.2.md in root now?

yes please

After:
```twig
{% for field in fields %}
{% if not field.rendered %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is a bit misleading to me. Quickly reading it, you might think this check is always necessary when iterating form fields to render them.
I'd rather show an example where you need to render the same field twice and thus store the result in a variable which would be rendered twice in the "after" code.

Copy link
ContributorAuthor

@ostroluckyostroluckyMay 15, 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 a bit misleading yes. But your suggested example is misleading too. Current code does not allow to render same field twice and neither this patch allows that. We shouldn't present a solution for rendering same field multiple times, as this has nothing to do with that.

Maybe example should just renamefields intofieldsWithPossibleDuplicates

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright


{%-blockform_rows -%}
{%forchildinform %}
{%forchildinformifnotchild.rendered%}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really do this change? Better get the deprecation and then the exception if you're trying to render the same row twice?

Choose a reason for hiding this comment

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

isn't this required to support partial form auto-rendering?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already supposed to be handled inform_rest:

{%-blockform_rest -%}
{%forchildinform -%}
{%ifnotchild.rendered %}
{{- form_row(child) -}}
{%endif %}
{%-endfor -%}

Copy link
ContributorAuthor

@ostroluckyostroluckyMay 15, 2018•
edited
Loading

Choose a reason for hiding this comment

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

This change is necessary for tests to pass, which means not doing this would be BC break

Copy link
Contributor

Choose a reason for hiding this comment

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

Which test? If the test is failing by triggering the deprecation, that's expected and should by marked as legacy, right?

jvasseur and yceruto reacted with thumbs up emoji
Copy link
ContributorAuthor

@ostroluckyostroluckyMay 15, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Ok yes, it's just deprecation. It was triggered by FormExtensionDivLayoutTest, inherited testcase testRestWithChildrenForms.

Negative of just marking tests as legacy is that it triggers unresolvable deprecation notices in user land. There should be some effort in avoiding it, not just mark test as legacy as first thing IMHO.

I might have found another way though

edit: here is a chain of calls which trigger deprecation:

form_rest -> form_row -> form_widget_compound -> form_rows

@ostroluckyostroluckyforce-pushed thesearchAndRenderBlock-deprecate-empty-return branch fromad04302 to259ef8bCompareMay 15, 2018 19:24

if ($renderOnlyOnce &&$view->isRendered()) {
// This is not allowed, because it would result in rendering same IDs multiple times, which is not valid.
@trigger_error(sprintf('Calling the "%s()" method for fields which were already rendered is deprecated since Symfony 4.1 and will throw an exception in 5.0.',__METHOD__),E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

should be 4.2 here.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add the field name here as well to help figuring out where that comes from in an app?

Copy link
Member

Choose a reason for hiding this comment

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

people don't callsearchAndRenderBlock directly, so this deprecation is confusing as they probably don't know about it.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@stof What do you suggest? I agree people don't call this directly, but formRenderer is responsible for this confusing behavior so deprecation needs to be triggered from within.

Copy link
Member

Choose a reason for hiding this comment

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

well,$blockNameSuffix allows you to know whetherform_row orform_widget was called originally, so you can use it to report it.

and I agree about the field name

----

* Deprecated calling`FormRenderer::searchAndRenderBlock` for fields which were already rendered.
Instead of expecting such calls return empty string, check if field has already been rendered.
Copy link
Member

Choose a reason for hiding this comment

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

The whole block must be indented properly under the* to fix rendering.

Copy link
Member

Choose a reason for hiding this comment

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

[...] calls to return [...] if the field [...]

4.2.0
-----

* deprecated calling`FormRenderer::searchAndRenderBlock` for fields which were already rendered.
Copy link
Member

Choose a reason for hiding this comment

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

the dot should be removed

{%-ifformisrootform -%}
{{ form_errors(form) }}
{%-endif -%}
{{-block('form_rows') -}}
Copy link
Member

Choose a reason for hiding this comment

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

this is a BC break in case someone overwrites the block

Copy link
ContributorAuthor

@ostroluckyostroluckyMay 18, 2018•
edited
Loading

Choose a reason for hiding this comment

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

For this reason I liked#27247 (diff) solution more. That would be BC break too in case someone overrides FormRenderer (and expect it to be called for already rendered field too), but that is much less likely imho. What do you think?

<?phpif (!$form->parent &&$errors):?>
<?phpecho$view['form']->errors($form)?>
<?phpendif?>
<?phpecho$view['form']->block($form,'form_rows')?>
Copy link
Member

Choose a reason for hiding this comment

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

same issue here about the BC break


if ($renderOnlyOnce &&$view->isRendered()) {
// This is not allowed, because it would result in rendering same IDs multiple times, which is not valid.
@trigger_error(sprintf('Calling the "%s()" method for fields which were already rendered is deprecated since Symfony 4.1 and will throw an exception in 5.0.',__METHOD__),E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

people don't callsearchAndRenderBlock directly, so this deprecation is confusing as they probably don't know about it.

@ostroluckyostroluckyforce-pushed thesearchAndRenderBlock-deprecate-empty-return branch 2 times, most recently from70840ed to3e54c12CompareMay 21, 2018 23:50
@ostrolucky
Copy link
ContributorAuthor

status: needs review

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

minor CS

{{ form_widget(field) }}
{% endif %}
{% endfor %}
```
Copy link
Member

Choose a reason for hiding this comment

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

missing empty line after the end of this block


if ($renderOnlyOnce &&$view->isRendered()) {
// This is not allowed, because it would result in rendering same IDs multiple times, which is not valid.
@trigger_error(sprintf('You are calling "form_%s" for field "%s" which has already been rendered before. Trying to render fields which were already rendered is deprecated since Symfony 4.2 and will throw an exception in 5.0.',$blockNameSuffix,$view->vars['name']),E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

We're trying to have only one sentence for deprecations. Might be changed to[...] rendered before, try [...]

Copy link
ContributorAuthor

@ostroluckyostroluckyMay 22, 2018•
edited
Loading

Choose a reason for hiding this comment

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

This?

You are calling "form_%s" for field "%s" which has already been rendered before, trying to render fields which were already rendered is deprecated since Symfony 4.2 and will throw an exception in 5.0.

I think there is too much information in there for one sentence. I don't know how to fit all of it in a way sentence seems natural 🤔

After:
```twig
{% for field in fieldsWithPotentialDuplicates %}
{% if not field.rendered %}
Copy link
Member

Choose a reason for hiding this comment

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

can be simplified:{% for field in fieldsWithPotentialDuplicates if not field.rendered %}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

IIRC this kind of filtering has limitations, depending on what's inside the for block, so I went with safer approach here. But I don't remember what was the issue then, so will change.

Copy link
Member

Choose a reason for hiding this comment

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

the limitation is that anyloop.* variables depending on the number of items in the loop won't work anymore (because it won't know in advance).
But if you skip the rendering entirely for some items with an inner filtering, these would not work well anyway, as some code usingloop.last would probably expect it to be the lastrendered item, not the last checked one.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

So do you think we should still go with short way? Another limitation is when you don't want to skip whole loop cycle, but only rendering

{%forfieldinfieldsWithPotentialDuplicates %}{# ...some code which works with field...#}  {%ifnotfield.rendered %}     {{ form_row(field) }}  {%endif %}{%endif %}

Copy link
Member

Choose a reason for hiding this comment

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

then it is not the same original code (and then, you would have to evaluate whether the additional code should run when the field does not get rendered), and so requires a case-by-case decision.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(after rebase)

@ostroluckyostroluckyforce-pushed thesearchAndRenderBlock-deprecate-empty-return branch 2 times, most recently from041e9d2 to5b47226CompareJune 20, 2018 21:38
@ostrolucky
Copy link
ContributorAuthor

Rebased. Failures unrelated

----

* Deprecated calling`FormRenderer::searchAndRenderBlock` for fields which were already rendered.
Instead of expecting such calls to return empty string, check if the field has already been rendered.
Copy link
Member

Choose a reason for hiding this comment

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

strings

@ostroluckyostroluckyforce-pushed thesearchAndRenderBlock-deprecate-empty-return branch from5b47226 toc7e01f4CompareJune 22, 2018 12:26
@ostrolucky
Copy link
ContributorAuthor

status: needs review

@fabpotfabpotforce-pushed thesearchAndRenderBlock-deprecate-empty-return branch fromc7e01f4 to02f2f0eCompareJune 25, 2018 06:48
@fabpot
Copy link
Member

Thank you@ostrolucky.

@fabpotfabpot merged commit02f2f0e intosymfony:masterJun 25, 2018
fabpot added a commit that referenced this pull requestJun 25, 2018
…y string (ostrolucky)This PR was squashed before being merged into the 4.2-dev branch (closes#27247).Discussion----------[Form] Deprecate `searchAndRenderBlock` returning empty string| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#26531| License       | MIT| Doc PR        | -I would like to remove this silent behavior, because it's confusingCommits-------02f2f0e [Form] Deprecate `searchAndRenderBlock` returning empty string
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
nicolas-grekas added a commit that referenced this pull requestMay 29, 2019
…eady rendered (yceruto)This PR was merged into the 5.0-dev branch.Discussion----------[Form] Throw exception when render a field which was already rendered| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -See#27247Commits-------d1bbad0 Throw exception when render a field which was already rendered
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

7 participants

@ostrolucky@nicolas-grekas@fabpot@stof@xabbuh@ogizanagi@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp