TYPO3 / Fluid

Fluid template rendering engine - Standalone version
GNU Lesser General Public License v3.0
153 stars 93 forks source link

Breaking change in 2.6.10 - signature change in registerArgument(…) #529

Closed kdambekalns closed 3 years ago

kdambekalns commented 4 years ago

As can be seen in https://github.com/TYPO3/Fluid/commit/cc5c3a208513b2310465b295dd63c0a9e316f8aa#diff-e13c23923563c9560a966f5c72c2a337a60bc825f76380ce6ca9259a0b06495cR166 2.6.10 changes the signature of a method that is clearly part of the public API.

This breaks any code extending that method.

albe commented 4 years ago

Also the behaviour change with the argumentsDefinitions (https://github.com/TYPO3/Fluid/commit/022c306f9b796971898c5a5b10bab6e11740e2f3) is a bit unfortunate for us, because we also rewrote the constructor (https://github.com/neos/flow-development-collection/pull/2257/files#diff-49e570e1bc1244134546047aafef5997be1a90cc27789383ac17378b47f12e26), though that's not technically a breaking change (we should not do what we do :D)

NamelessCoder commented 4 years ago

I'm sorry this broke your implementation - but we didn't find another way to explicitly make some arguments change their escaping behavior. My personal reasoning why this was "okay" was that the security flaw made any versions before the ones released today not viable for use anyway - so a minimum requirement bump was bound to be required either way for every supported 2.x branch. I don't completely agree that overrides of protected methods is strictly considered public API but that's also not so relevant for this discussion.

I had a deeper look at Neos' implementation of Fluid and from what I can see, these overrides are there in order to throw a different type of exception (Fluid already throws a similar exception) which probably has to do with some legacy elsewhere in the code where try/catch listens for a different exception. Would it perhaps be a good opportunity to switch these to listen for Fluid's native exception and remove the overrides? If I'm right that these are for legacy purposes, the actual catching of exceptions should already be encapsulated completely in code that you control and these exceptions would never reach the third-party user space, so the exact type of exception shouldn't matter as long as it is caught.

I'd be happy to assist with alternative suggestions if you wish to decrease the complexity of the overrides you currently use. I suspect there are good opportunities for an improved implementation!

albe commented 4 years ago

these overrides are there in order to throw a different type of exception (Fluid already throws a similar exception) ... Would it perhaps be a good opportunity to switch these to listen for Fluid's native exception and remove the overrides?

That's actually a helpful observation! We'll take a look

bmack commented 4 years ago

Hmm, one could also use func_get_args() to ensure backwards-compatibility, and not change the method signature. That's how other PHP libraries deal with backwards-compatibility.

kdambekalns commented 4 years ago

I don't completely agree that overrides of protected methods is strictly considered public API

Not in any case, but it has an @api annotation… anyway, thanks for the quick & constructive reply!

tippfelher commented 4 years ago

Any plans on fixing this issue? Unable to install neos-base-distribution@4.3.16 for over 2 days now..

kdambekalns commented 4 years ago

Any plans on fixing this issue? Unable to install neos-base-distribution@4.3.16 for over 2 days now..

Oh, the workaround is easy: require typo3fluid/fluid 2.5.10 or 2.6.9 for now.

albe commented 4 years ago

We'll tackle this in Flow with https://github.com/neos/flow-development-collection/pull/2265

DrillSergeant commented 3 years ago

Same change was made in 2.4.4. We have some Neos 3.* projects that broke after updating. As we try to update the last Neos 3.3 patch before EOL we will just pin to typo3fluid/fluid to 2.4.3.

kaystrobach commented 3 years ago

same with 2.4.3 => 2.4.4

ohader commented 3 years ago

It seems that Flow changes were merged... I guess we can close this issue, correct?