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

FEATURE: Introduce catchup hook for DocumentUriPath projection #4309

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Jun 6, 2023

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.

@dlubitz dlubitz changed the title FEATURE: Introduce catchup hook for DocumentUriPath projection 9.0 FEATURE: Introduce catchup hook for DocumentUriPath projection Jun 6, 2023
@dlubitz dlubitz self-assigned this Jun 6, 2023
@dlubitz dlubitz added Feature and removed Feature labels Jun 6, 2023
@bwaidelich bwaidelich changed the title 9.0 FEATURE: Introduce catchup hook for DocumentUriPath projection FEATURE: Introduce catchup hook for DocumentUriPath projection Jun 13, 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.

The cosmetic changes make it a little hard to review this, but it looks good to me!
I just added a few comments/questions

Copy link
Member

@mhsdesign mhsdesign left a 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

@dlubitz
Copy link
Contributor Author

dlubitz commented Jun 16, 2023

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.

@mhsdesign
Copy link
Member

mhsdesign commented Jun 16, 2023

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 ^^

@dlubitz dlubitz force-pushed the 90/feature-redirect-neos-adapter-catchup branch 2 times, most recently from d081761 to ceec36c Compare June 16, 2023 18:44
@dlubitz dlubitz requested review from bwaidelich and mhsdesign June 16, 2023 19:39
@dlubitz
Copy link
Contributor Author

dlubitz commented Jun 16, 2023

Formatting is removed now and your requests built in.

@bwaidelich
Copy link
Member

Awesome, thanks. I'll test asap.
btw: please don't force push, so it's easier to follow the changes, we can squash the commits when merging

@dlubitz
Copy link
Contributor Author

dlubitz commented Jun 19, 2023

Wasn't sure how to enforce the squash on merge. Didn't want to have tha back and forth in the history.

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.

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)

@bwaidelich bwaidelich merged commit 552e52e into 9.0 Jul 3, 2023
@bwaidelich bwaidelich deleted the 90/feature-redirect-neos-adapter-catchup branch July 3, 2023 16:36
@mhsdesign
Copy link
Member

upsi @bwaidelich i didnt mean to block it any further as you already guessed ^^

@bwaidelich
Copy link
Member

@mhsdesign no worries, happens to me all the time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants