!!!BUGFIX: Stop assigning all variables to section when no variables are given

[rkrahnen]

As discussed in slack (https://neos-project.slack.com/archives/C050KKBEB/p1608132977224400) I removed the function.

[albe]

Thanks a lot for the PR! While this makes a lot of sense (as the previously existing comment already suggested), we do have a test covering that behaviour and changing it will be very subtle in it's breakiness. So we need to discuss how we should handle that sufficiently. !!! next master only, or is there some way we can make the change more visible and land it in 7.1?

PS: From what I understand, you can't even work around this by explicitly specifying an empty array as arguments for the section. So you'd need to provide a non-existing variable as argument to prevent the section from receiving any arguments at all (though an unlikely case).

[albe]

Robin Krahnen 3 months ago As this change breaks with the current behavior, maybe even this is not what you want. In my opinion it is a bug, but is has been there for many years, so I do not know if changing it with 7.1 or 8.0 would be the better solution for you.

Alexander Berl:unicorn2: 3 months ago I think that's some kind of breakyness that would be okayis for a minor release, though I would maybe need a bit clearer picture of the cases where this breaks to have a final informed opinion. In any case master is correct then, as 8.0 just means "don't merge before 7.3 is released".

Robin Krahnen 3 months ago That is easy to explain :slightly_smiling_face: When using <f:render partial="MyPartial" section="MySection" /> you would expect no variables (except the settings ) available in the rendered content, because arguments is not provided and the documentation says “Array of variables to be transferred. Use {_all} for all variables”. But because of the function I removed, it was working the same way like with arguments="{_all}" .

Alexander Berl:unicorn2: 3 months ago Yeah, but fixing that will cause what in the worst case? A form being rendered with incomplete data that will effectively cause things being overwritten with empty? :scream: And can this be done in a way that informs the user that this will happen if he doesn't add a {_all} (I guess not)?

Robin Krahnen 3 months ago For example, yes. In the project I have found this “bug” this change would break the whole page, as the header is rendered with this view helper and then the variables needed would be missing.

this change would break the whole page gives me the feeling we should hold this back for next major.

[sorenmalling]

Hi @rkrahnen

Can you please create a corresponding issue, that we can link to the Flow 8 project board? :)

[albe]

@kitsunet sorry for tagging you, but I'd like to get another informed opinion that knows Fluid well and you were the one that added the TODO comment (https://github.com/neos/flow-development-collection/commit/da3b1472d7d5ecc5c4e032a285d7c22e4238b0c2). Do you think this is sensible to do/remove?

If so I'd love to have a test added that covers this case and would then get this into 8.0

[albe]

@mficzel @kitsunet @kdambekalns one last thing to decide upon before 8.0 (at least from Flow's side) - if we'll not have it this time around I fear it will only become viable for 9.0 at which point it's even unclear what Fluid status will be like (maybe 3.0 will be finished https://github.com/TYPO3/Fluid/issues/561). IMO the general idea here makes sense

[albe]

So it seems this part was actually a workaround to fix broken _all passing in our FluidAdaptor?

1. Neos\FluidAdaptor\Tests\Functional\View\StandaloneViewTest::renderSectionIsEvaluatedCorrectly Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'test bar' +'test '

/home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/Packages/Framework/Neos.FluidAdaptor/Tests/Functional/View/StandaloneViewTest.php:79 /home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/Packages/Framework/Neos.FluidAdaptor/Tests/Functional/View/StandaloneViewTest.php:42 /home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/bin/phpunit:120

2. Neos\FluidAdaptor\Tests\Functional\View\StandaloneViewTest::interceptorsWorkInPartialRenderedInStandaloneSection First rendering was not escaped. Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'Christian uses <h1>HACK</h1>' +'Christian uses'

/home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/Packages/Framework/Neos.FluidAdaptor/Tests/Functional/View/StandaloneViewTest.php:321 /home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/Packages/Framework/Neos.FluidAdaptor/Tests/Functional/View/StandaloneViewTest.php:42 /home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/bin/phpunit:120

[kdambekalns]

Fun fact: Even when declaring foo as the variable to use in renderSectionIsEvaluatedCorrectly(), the test fails. 🤔

[mficzel]

Can be merged as soon as tests are green again ...

[kdambekalns]

So it seems this part was actually a workaround to fix broken _all passing in our FluidAdaptor?

Now I got, I think. You mean, we added a workaround to fix a bug with _all in (upstream) Fluid?

[kitsunet]

This thing comes all the way from our Fluid rendering in Fusion, as we allowed to render sections without the surrounding tmeplate there, and that was not properly supported back then (and to me still seems not supported in the way we use it). I guess we could/should fix this first in the TemplateImplementation in Fusion so that we provide the variables argument to renderSection in there.

https://github.com/neos/neos-development-collection/blob/8.3/Neos.Fusion/Classes/FusionObjects/TemplateImplementation.php#L141