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: Partially repair convert uris implementation according to some 8.3 tests #4744

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

mhsdesign
Copy link
Member

Upgrade instructions

Review instructions

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 mhsdesign changed the title BUGFIX: Repair convert uris implementation according to 8.3 spec BUGFIX: Partially repair convert uris implementation according to some 8.3 tests Nov 12, 2023
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.

Makes sense IMO, thanks for taking care.
But maybe we can split bugfix from the feature!?

Not to make things more complicated, but I can't really judge on the replaceLinkTargets() changes right now, the rest I would +1

@mhsdesign
Copy link
Member Author

But maybe we can split bugfix from the feature!?

imo nope its a regression to remove a feature that worked previously.

@mhsdesign
Copy link
Member Author

Not to make things more complicated, but I can't really judge on the replaceLinkTargets() changes right now

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 bugs like these will make things even harder.

or we find a way to accurately performantly archive the same behaviour as in 8.3

@bwaidelich
Copy link
Member

imo nope its a regression to remove a feature that worked previously.

OK, you're right. I just fail to give an +1 for the replaceLinkTargets() – OTOH this is not the nicest code to begin with, and almost any change to it will make it better. And if the new version is closer to the 8.3 behavior I'd say: Let's get this in!

Are you OK with changing the trait name though – it's not a big thing, but also easy to change :)

@kitsunet
Copy link
Member

+1 for renaming the trait and then getting this in.

@mhsdesign
Copy link
Member Author

Hmm could you please diff the 8.3 and new 9.0 convert uris implementation?

im really just about to checkout the 8.3 part again and migrate it without any additional performance improvements, as those will cause bugs.

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

protected function replaceLinkTargets(string $processedContent): string

@kitsunet
Copy link
Member

kitsunet commented Nov 17, 2023

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.

@mhsdesign mhsdesign force-pushed the bugfix/90-fix-convertUrisImplementation branch from f843e39 to 0ff614f Compare November 17, 2023 13:14
@mhsdesign
Copy link
Member Author

Okay ill merge this now an prepare a pr where i check all neos fusion objects if they should better be remigrated ;)

@mhsdesign mhsdesign merged commit 23089d7 into neos:9.0 Nov 17, 2023
4 checks passed
@mhsdesign mhsdesign deleted the bugfix/90-fix-convertUrisImplementation branch November 17, 2023 13:17
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Nov 17, 2023
…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
Copy link
Member Author

see #4760

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Nov 17, 2023
…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 added a commit to mhsdesign/neos-development-collection that referenced this pull request Jan 14, 2024
…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 added a commit to mhsdesign/neos-development-collection that referenced this pull request Feb 11, 2024
…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)
neos-bot pushed a commit to neos/neos that referenced this pull request Feb 12, 2024
…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/neos-development-collection#4744 (comment)
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.

3 participants