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

TASK: remigrate all Neos.Neos fusion files and implementations #4760

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Nov 17, 2023

We noticed that Neos 9.0 had one missing feature in the Fusion implementations. To make sure there are no other faulty fusion implementations i checked and remigrated some fusion implementations.

Especially the ConvertUris implementation seems to have been affected. It seems as that it was now optimised for performance with a loss in features.

see #4744

it seems $someone tried to optimize the convert uris stuff from 8.3 while migrating the code to 9.0
it should behave more performant with the cost of being less accurate and not covering all edgecases.

Maybe we should revert those performance optimization and introduce all those additional preg_match and replaces again for more accuracy. People might already have trouble enough with 9.0. Silent regressions like these will make things even harder.

@kitsunet wrote

We can probably work on this at another point again, given that this is cached I don't see much problem in having this regex there, also I am sure this won't be in the top 5 of performance problems of rendering 🙈

tl;dr; fine by me to go back to this.

Thats why i did checkout the 8.3 version of the ConvertUrisImplementation and remigrated its code to 9.0 without any additional performance in mind.

How i did it?

i reset the interesting fusion files, and checked what could possibly be wrong:

git checkout 8.3 -- Neos.Neos/Classes/Fusion/*Implementation.php
git checkout 8.3 -- Neos.Neos/Resources/Private/Fusion/
git checkout 8.3 -- Neos.Neos/Classes/Fusion/ExceptionHandlers/
git checkout 8.3 -- Neos.Neos/Classes/Fusion/Helper/

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign
Copy link
Member Author

#4552 and #4524 are the upcoming tasks to make the NodeUriImplementation work 100%

@mhsdesign mhsdesign force-pushed the task/remigrateAllNeosFusionFilesAndImplementations branch from 4670625 to 3cf2d55 Compare January 14, 2024 11:43
@mhsdesign mhsdesign marked this pull request as ready for review January 14, 2024 11:44
Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

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

Looks good.

I wouldn't mind to take over some of the small improvements code wise (type hints, etc). But we can do that afterwards.

…vertUrisImplementation

the convert uris stuff was optimized while migrating the code to 9.0
it should behave more performant with the cost of being less accurate and not covering all edge-cases.

This change reverts those performance optimization and introduce all those additional preg_match and replaces again for more accuracy.

This is the even more expensive but more accurate 8.3 solution.

neos#4744 (comment)
@mhsdesign mhsdesign force-pushed the task/remigrateAllNeosFusionFilesAndImplementations branch from 3cf2d55 to 6de70fd Compare February 11, 2024 19:50
@mhsdesign mhsdesign merged commit 162f8a5 into neos:9.0 Feb 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants