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.

Sign up for GitHub

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

Jump to bottom

TASK: Cleanup and stabilize Neos.ContentGraph.DoctrineDbalAdapter #5088

Merged
merged 9 commits into from
May 26, 2024
Merged

TASK: Cleanup and stabilize Neos.ContentGraph.DoctrineDbalAdapter #5088

merged 9 commits into from
May 26, 2024

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented May 23, 2024 •
edited
Loading

Cleans up and stabilizes the implementation of the Neos.ContentGraph.DoctrineDbalAdapter implementation by...

  • replacing inline SQL strings with Heredoc for better readability and IDE support
  • properly handling and declaring Exceptions
  • synchronizing some internal namings
  • moving all when*() methods from Traits to the main DoctrineDbalContentGraphProjection
  • fixing method/field visibility (no protected in final classes)

@github-actions github-actions bot added the Task label May 23, 2024
bwaidelich added 3 commits May 23, 2024 22:57
@bwaidelich
replace another inline SQL query with heredoc
761d116
@bwaidelich
Fix method visibility
9cde7c5
@bwaidelich
Delete EventCouldNotBeAppliedToContentGraph
03dff89 
and use RuntimeException instead

Note: There is no need for a custom exception class for these unexpected cases and a DomainException is not the correct exception class anyways
dlubitz
dlubitz reviewed May 24, 2024
View reviewed changes
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.

Wow, this was quite a lot 🙈
Thanks for working through this. I like especially the fetchFirstColumn improvement. 👍

I just found one litte typo.

Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php Outdated Show resolved Hide resolved
mhsdesign
mhsdesign reviewed May 24, 2024
View reviewed changes
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.

Thanks for the adjustments so far, left some comments. I still have to review the sql changes in these classes:

  • Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/ProjectionIntegrityViolationDetector.php
  • Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ProjectionContentGraph.php

@kitsunet
Copy link
Member

I like what I am seeing so far 😆 When the reminaing convos are fixed this is good to go IMHO. Thanks!

Base automatically changed from bugfix/fix-projection-query-in-integrity-violation-detector to 9.0 May 24, 2024 11:14
@github-actions github-actions bot added the 9.0 label May 24, 2024
@bwaidelich bwaidelich requested a review from mhsdesign May 24, 2024 13:13
kdambekalns
kdambekalns reviewed May 24, 2024
View reviewed changes
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

I thought I would give this a thorough review, but… the files with the large diffs are beyond me today…

So take a carefully crafted "LGTM, IMHO, 👍 by 😎" from me.

@bwaidelich
Copy link
Member Author

the files with the large diffs are beyond me today…

I can fully comprehend.. Thanks for trying – and hopefully those changes make future diffs easier to read :)

@bwaidelich bwaidelich mentioned this pull request May 24, 2024
Merged
mhsdesign
mhsdesign approved these changes May 26, 2024
View reviewed changes
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.

Okay this was not a pleasure to review :P (especially because you pushed new changes :D) but thanks for your cleanup ^^

Ps i actually tried out the phpstorm gh intergration and here the diff is a little more plesant and for the cases even phpstorm couldnt keep up you can copy the text and select another pasage and compare to clipboard.

So my eagle eye says +1 Thanks for keeping this separate :D

...tentGraph.DoctrineDbalAdapter/src/Domain/Projection/ProjectionIntegrityViolationDetector.php
*/
public function referencesAreDistinctlySorted(): Result
{
// TODO implement
Copy link
Member

Choose a reason for hiding this comment

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

hello world, but you just added the todo, seems like it wasnt implemented beforehand as well

Copy link
Member Author

Choose a reason for hiding this comment

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

correct. Todos can be searched for. There should be no stubs without

@bwaidelich bwaidelich merged commit 784cbfc into 9.0 May 26, 2024
10 checks passed
@bwaidelich bwaidelich deleted the task/cleanup-doctrinedbaladapter branch May 26, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlubitz dlubitz dlubitz left review comments

@kdambekalns kdambekalns kdambekalns left review comments

@mhsdesign mhsdesign mhsdesign approved these changes

Assignees
No one assigned
Labels
9.0 Task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bwaidelich @kitsunet @kdambekalns @dlubitz @mhsdesign