BUG: Fluid 2.8.0 causes type hint errors

[Sebobo]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

Flow / Neos crash with the following error:

Exception #1355480641 in line 397 of /Users/USERNAME/Workspace/Projects/neos-development-7/Packages/Framework/Neos.Flow/Classes/Core/Booting/Scripts.php: PHP Fatal error:  Declaration of Neos\FluidAdaptor\Core\ViewHelper\TemplateVariableContainer::resolveSubVariableReferences($propertyPath) must be compatible with TYPO3Fluid\Fluid\Core\Variables\StandardVariableProvider::resolveSubVariableReferences(string $propertyPath): string in /Users/USERNAME/Workspace/Projects/neos-development-7/Packages/Framework/Neos.FluidAdaptor/Classes/Core/ViewHelper/TemplateVariableContainer.php on line 55

Fatal error: Declaration of Neos\FluidAdaptor\Core\ViewHelper\TemplateVariableContainer::resolveSubVariableReferences($propertyPath) must be compatible with TYPO3Fluid\Fluid\Core\Variables\StandardVariableProvider::resolveSubVariableReferences(string $propertyPath): string in /Users/USERNAME/Workspace/Projects/neos-development-7/Packages/Framework/Neos.FluidAdaptor/Classes/Core/ViewHelper/TemplateVariableContainer.php on line 55

Expected Behavior

No error

Steps To Reproduce

Install or update Flow / Neos with the latest Fluid 2.8.0.

Environment

- Flow: 7.2+
- PHP: any

Anything else?

The change and Fluid release TYPO3/Fluid@b3d7030
Their PR TYPO3/Fluid#736

[Sebobo]

Thx to @crydotsnake for reporting

[Sebobo]

So one solution would be to change the dependency on Fluid to 2.7.* to only accept patch releases and then adjust to minor releases when we have verified them.

[lolli42]

Hey. Just saw this. That fluid type hint change was done by me.

I actually did not expect the type hint on resolveSubVariableReferences(string $propertyPath) on that protected method would cause Flow to fail: The StandardVariableProvider should be a leaf class, own providers should implement the interface instead. As such, I went ahead and added the argument and the return type hint.

Of course, you're correct, strictly speaking this was breaking and I'm sorry Flow run into this. Fluid could remove return and argument type hint in this case again, though, if that is helpful.

The other solution is to not extend StandardVariableProvider but implement an own version in Flow that only implements the interface. Further patches in Fluid will do that for other Variable providers that extend StandardVariableProvider as well, and StandardVariableProvider will have an @todo to declare it final with next major.

There is a reason I started touching this area: The Variable provider stuff is one of the most often called constructs at runtime, I was able to squeeze quite a bit of performance out of the construct with a series of patches included in 2.8. And there is potential for more: The template compiler / node converter could pre-optimize calls on StandardVariableProvider when StandardVariableProvider has some more dedicated methods for cases where less magic is needed. For instance the call to resolveSubVariableReferences() could be skipped entirely if there is no sub variable reference (which is known at compile time), and a dotted path could be exploded to an array up-front to not explode() at runtime each time. To do that, VariableProviderInterface will be streamlined by loosing some methods (eg. ArrayAccess), and StandardVariableProvider will provide more specialized methods which will be activated in VariableProviderInterface with next major.

In general, I'm trying to streamline the Fluid codebase a bit to reduce complexity and by avoiding constructs that have been added for performance reasons that don't pay off, or make the constructs more convoluted than they should be.

Also, I'll continue to add return type hints here and there - for constructs that I'd consider internal. Those will gain an "@internal" or "@todo declare final" along the way. TemplateParser and NodeConverter are examples. Maybe Flow could help here by streamlining the adapter a bit: In TYPO3, the ext:fluid "adapter" is meanwhile a very slim layer that does not extend many of the Fluid core classes anymore. It would be good if Flow could see if it could make it's adapter more slim as well to reduce impact when Fluid changes internal details.

Long story short: I'll try to add type hints here and there, even without branching a major (even though a major has to follow at some point), but i'll try to be careful with that. And we can easily change the offending patch you run into back to skip the type hints. What do you think? Which way helps you the most?

[kitsunet]

Thank for the explanation @lolli42 makes perfect sense, I think protected stuff is the gray area where breaking is rather relative. Way back when we created the Adaptor many decisions were probably not the best ones for going forward, but I also don't know which interfaces existed back then. We could probably make the adaptor way more light these days.
As we are currently not putting a lot of work into the adaptor I think sticking to bugfix versions is the way to go, so should be fine if you continue cleaning up.

[lolli42]

Ok, so your decision is to stick with 2.7 for now and then allow new minors only after you looked at the adapter compatibility once in a while?

That's good and bad for Fluid: When I'm doing more critical things with Fluid, I always trigger TYPO3 CI with latest changes or PR's to see if I missed something and it breaks. Just as additional safety net. We've now run into regressions in combination with Flow twice, and I'm not aware of a workflow I could trigger as easily (did Flow CI find the above issue, or did that only pop up in projects?). So, not allowing minors in Flow is good for Fluid since it gives Fluid more freedom in case Flow's adapter does funny things, and it's bad since we don't find issues early and essentially shift the responsibility to Flow in case Fluid did something fishy.

[markusguenther]

We released a quickfix that now pins to 2.7.x at the moment. Thanks for you describing messages 🙏