-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
FEATURE: Introduce catchup hook for DocumentUriPath projection #4309
Conversation
Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathFinder.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cosmetic changes make it a little hard to review this, but it looks good to me!
I just added a few comments/questions
Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathFinder.php
Outdated
Show resolved
Hide resolved
Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathFinder.php
Outdated
Show resolved
Hide resolved
Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathFinder.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry im against the cosmetic changes in this pr (otherwise debatable ^^).
but i actually dislike the leading comma
We already discussed that somewhere... conclusion was that it make sense as it reduces unneccessary changes in future. If you add or remove a parameter, that doesn't effect the line above/below. Same for arrays. |
Im not aware of this but i would be happy to discuss this in the next team meeting? Either way in favor of your actual changes in the PR, i would keep the noise (the code style change) out of it, so the discussion can sorely focus about your feature. If we agree on the code style we should make a batch pr with all the changes, but i wouldnt want them to be included in every diff for the next prs of the next years ^^ I would find it pretty exhausting to review then ^^ |
d081761
to
ceec36c
Compare
Formatting is removed now and your requests built in. |
Awesome, thanks. I'll test asap. |
Wasn't sure how to enforce the squash on merge. Didn't want to have tha back and forth in the history. |
ceec36c
to
5de6839
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
As discussed, a lot of the code can hopefully be removed once #4289 is resolved, but it makes sense for now, especially since you also extended the DocumentUriPathFinder
API as well
@mhsdesign I'll merge this despite of your block (as Denny reverted the cosmetic changes). In general I would suggest to use the -1 vote only for severe concerns (or to keep a close eye on the PR to remove it once issues are resolved)
upsi @bwaidelich i didnt mean to block it any further as you already guessed ^^ |
@mhsdesign no worries, happens to me all the time :) |
Allows to hook into the DocumentUriPath projection during event handling. This enables the
Neos.Redirect.NeosAdapter
to react on the events accordingly.Also the finder got a new method to query for children of a given DocumentNodeInfo.