Skip to content
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

!!! BUGFIX: Make FluidAdaptor compatible with TYPO3Fluid 2.5.11+ and 2.6.10+ #2265

Merged
merged 7 commits into from Nov 20, 2020

Conversation

albe
Copy link
Member

@albe albe commented Nov 19, 2020

This is breaking in case you created your own ViewHelper that overrides the registerArgument() or overrideArgument() method. In that case you need to add a new boolean optional argument $escape = null and forward that to the parent method.

This is a backport of #2257
Fixes #2260

Argument validation has been deferred to a later point.
@albe
Copy link
Member Author

albe commented Nov 19, 2020

Still need to adjust composer dependency to ~2.5.11 || ^2.6.10

@kdambekalns
Copy link
Member

This affects Flow 4.3 as well, do we consider that worth a fix? It somehow falls into the "security" category, which means until December 4.3 is still covered…

@kdambekalns
Copy link
Member

We should get this merged and released: When using roave/security-advisories you cannot pin to 2.6.9 any longer…

@kdambekalns
Copy link
Member

Question: why not "backport" #2259 as well?

@bwaidelich
Copy link
Member

Question: why not "backport" #2259 as well?

Yep, I had the same question. @albe any reason for this over #2259 for old versions? I think the "breakiness" is roughly equal

@bwaidelich
Copy link
Member

Doh, I just realized it's not as easy because we rely on the former exception in https://github.com/neos/flow-development-collection/blob/5.3/Neos.FluidAdaptor/Classes/Core/ViewHelper/AbstractViewHelper.php#L123 for example (for 6+ this was removed with #1574)

@albe
Copy link
Member Author

albe commented Nov 20, 2020

Well, we could fix that catch block. But yeah, it is a bit more breaking than just adjusting the signature. Anyway, without 90edda3 and the Fluid version constraint the argument validation will run twice - for latest Fluid versions. Should we accept that?

This reverts commit caa0816.
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

I reverted my last commit since it is more breaking than I thought initially.
Tested the original fix again and it works just fine.
Will merge if CI is green

@albe
Copy link
Member Author

albe commented Nov 20, 2020

Alexander Berl: 5 minutes ago
Don't forget to raise the Fluid version constraint!

Or we revert 90edda3 too

@bwaidelich
Copy link
Member

Or we revert 90edda3 too

@albe I lost track now.. And since I made a lot of problematic decisions today (including an accidental direct push to the 5.3 branch) I don't dare to hit the merge button now.. I guess I'll have to look at this when my brain works again, or you guys take it from here

@albe
Copy link
Member Author

albe commented Nov 20, 2020

@kdambekalns what do you think? Restrict Fluid version (which I'd prefer) or run with potentially running the arguments validation twice (should not break something but is obviously unnecessary work)?

@kdambekalns
Copy link
Member

@kdambekalns what do you think? Restrict Fluid version (which I'd prefer) or run with potentially running the arguments validation twice (should not break something but is obviously unnecessary work)?

Restricting is fine with me, the older versions are rejected by roave/security-advisories and should not be used anyway.

@albe
Copy link
Member Author

albe commented Nov 20, 2020

Talking about stupid little mistakes... 🧸

@albe
Copy link
Member Author

albe commented Nov 20, 2020

So this should be fine now - I'd merge once travis passes (again). Can also do upmerges and tag releases later this evening or maybe tomorrow

@kaystrobach
Copy link
Contributor

2.4.3 => 2.4.4 for flow 4.x is also breaking.

@kdambekalns
Copy link
Member

But anything below Flow 5.3 is EOL by now…

@albe
Copy link
Member Author

albe commented Jan 1, 2021

Right. Anything Flow < 5.3 should pin the Fluid dependency.

@kaystrobach
Copy link
Contributor

That’s what we do right now 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants