TASK: Cosmetic cleanup of `DoctrineDbalContentGraphProjection` #5092

bwaidelich · 2024-05-24T17:50:20Z

Sorts when-matchers and -handlers alphabetically and moves other private methods to the bottom of the class

Note: This is just a cosmetic change, no behavior has been changed

Sorts `when`-matchers and -handlers alphabetically and moves other private methods to the bottom of the class

…trinedbaladapter-2

mhsdesign · 2024-05-26T07:56:24Z

Thanks for sorting things... but is it really worth to do so? Sorting a 1000Lines class is just a bit evil because git wont keep up with it ... git blame will be useless (unless magic cow powder is used) and there is history to preserve ...
Im not really sure if we should do this.

kitsunet · 2024-05-26T08:50:28Z

I wondered the same, because I will certainly forget doing that whenever I add something later, doing a quick search or using phpstorms structure thing are fast enough that I wouldn't even bother thinking something is alphabetical.

bwaidelich · 2024-05-26T09:18:53Z

It's a pitty that git (or rather the github diff view) isn't better with these pure cosmetic changes.
But IMO we should trust our tests (and the contributor that claims that this is a purely visual change).

The DoctrineDbalContentGraphProjection is one of the most important classes and it was in a real mess.
I mainly separated lowlevel helper methods from the when* handlers in this PR. But for the follow-ups we'll introduce more handlers and I always wonder where to put them. That's why I went for a defined order that will make it easier to navigate the class.

I will certainly forget doing that whenever I add something later

With the follow-ups (#5096 and #5097) I'll add more handlers, basically covering all remaining events. So it's not very likely that new handlers will be added very often after that. And then we at least have a common scheme that we can follow.. (and I will have an eye on that).

Besides, I set up my PHPStorm to automatically sort those for my own projections:

bwaidelich · 2024-05-26T09:48:57Z

I think I skipped over your concern re git history/blameability.. good point.
If you think it's not worth it, I'll close this one and rebase the dependent prs

mhsdesign · 2024-05-26T10:41:52Z

we should trust [..] the contributor that claims that this is a purely visual change.

haha trust i do not. :D I would actually review this by confirming that my php storm would sort them the same way and then diffing both outputs. So that is not the point as you also mentioned.

But blaming is not simple.
Notice by default blame will say they were all modified 2024-05-24

➜  Neos git:(task/cleanup-doctrinedbaladapter-2) ✗ git blame Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php -L734,762

60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 734)     private function copyReferenceRelations(
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 735)         NodeRelationAnchorPoint $sourceRelationAnchorPoint,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 736)         NodeRelationAnchorPoint $destinationRelationAnchorPoint
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 737)     ): void {
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 738)         $copyReferenceRelationStatement = <<<SQL
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 739)             INSERT INTO {$this->tableNames->referenceRelation()} (
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 740)               nodeanchorpoint,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 741)               name,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 742)               position,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 743)               destinationnodeaggregateid
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 744)             )
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 745)             SELECT
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 746)               :destinationRelationAnchorPoint AS nodeanchorpoint,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 747)               ref.name,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 748)               ref.position,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 749)               ref.destinationnodeaggregateid
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 750)             FROM
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 751)                 {$this->tableNames->referenceRelation()} ref
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 752)                 WHERE ref.nodeanchorpoint = :sourceNodeAnchorPoint
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 753)         SQL;
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 754)         try {
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 755)             $this->dbal->executeStatement($copyReferenceRelationStatement, [
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 756)                 'sourceNodeAnchorPoint' => $sourceRelationAnchorPoint->value,
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 757)                 'destinationRelationAnchorPoint' => $destinationRelationAnchorPoint->value
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 758)             ]);
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 759)         } catch (DbalException $e) {
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 760)             throw new \RuntimeException(sprintf('Failed to copy reference relations: %s', $e->getMessage()), 1716489394, $e);
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 761)         }
60753a8f70a (Bastian Waidelich 2024-05-24 19:49:58 +0200 762)     }

only when using -M --minimal or even -C we start to get real output:

➜  Neos git:(task/cleanup-doctrinedbaladapter-2) ✗ git blame Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php -L734,762 -M

e917cb706a2 Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (bwaidelich          2023-03-14 15:47:58 +0100 734)     private function copyReferenceRelations(
07c2a5123cc Neos.ContentGraph.DoctrineDbalAdapter/Classes/Domain/Projection/GraphProjector.php (Bernhard Schmitt    2022-02-19 14:46:22 +0100 735)         NodeRelationAnchorPoint $sourceRelationAnchorPoint,
07c2a5123cc Neos.ContentGraph.DoctrineDbalAdapter/Classes/Domain/Projection/GraphProjector.php (Bernhard Schmitt    2022-02-19 14:46:22 +0100 736)         NodeRelationAnchorPoint $destinationRelationAnchorPoint
3b629cb523d Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Sebastian Kurfuerst 2022-08-04 11:27:35 +0200 737)     ): void {
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 738)         $copyReferenceRelationStatement = <<<SQL
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 739)             INSERT INTO {$this->tableNames->referenceRelation()} (
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 740)               nodeanchorpoint,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 741)               name,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 742)               position,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 743)               destinationnodeaggregateid
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 744)             )
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 745)             SELECT
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 746)               :destinationRelationAnchorPoint AS nodeanchorpoint,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 747)               ref.name,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 748)               ref.position,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 749)               ref.destinationnodeaggregateid
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 750)             FROM
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 751)                 {$this->tableNames->referenceRelation()} ref
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 752)                 WHERE ref.nodeanchorpoint = :sourceNodeAnchorPoint
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 753)         SQL;
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 754)         try {
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 755)             $this->dbal->executeStatement($copyReferenceRelationStatement, [
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 756)                 'sourceNodeAnchorPoint' => $sourceRelationAnchorPoint->value,
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 757)                 'destinationRelationAnchorPoint' => $destinationRelationAnchorPoint->value
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 758)             ]);
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 759)         } catch (DbalException $e) {
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 760)             throw new \RuntimeException(sprintf('Failed to copy reference relations: %s', $e->getMessage()), 1716489394, $e);
8df673d13ab Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php   (Bastian Waidelich   2024-05-23 22:54:23 +0200 761)         }
ab4058b5f13 Neos.ContentGraph.DoctrineDbalAdapter/Classes/Domain/Projection/GraphProjector.php (Sebastian Kurfürst  2021-02-12 14:03:01 +0100 762)     }

so with the right tooling its not impossible, tough phpstorm and github ui sadly dont support any advanced git flags like -M: https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--Mltnumgt

Detect moved or copied lines within a file. When a commit moves or copies a block of lines (e.g. the original file has A and then B, and the commit changes it to B and then A), the traditional blame algorithm notices only half of the movement and typically blames the lines that were moved up (i.e. B) to the parent and assigns blame to the lines that were moved down (i.e. A) to the child commit. With this option, both groups of lines are blamed on the parent by running extra passes of inspection.

bwaidelich · 2024-05-26T10:51:25Z

@mhsdesign @kitsunet I closed this before because I really want to get to the actual changes. But I realized that..

...this is the last possibility to do these bigger cleanups
...the blame-argument is valid, but we already "lost" the simple history with TASK: Cleanup and stabilize Neos.ContentGraph.DoctrineDbalAdapter #5088 and the numerous refactorings before that
...rebasing the dependents will be a lot of error-prone work

mhsdesign

Actually i was blind. Select whole method -> right click -> Git -> Show History for Selection

actually works wonders. And thats the thing i always use all the time. So if you havent started to rebase the others yet i think we would do you a favour merging this one.

+1

bwaidelich · 2024-05-26T11:00:29Z

Select whole method -> right click -> Git -> Show History for Selection

Awesome, I didn't know that one!

mhsdesign · 2024-05-26T11:03:24Z

Lol couldnt live without that seriously :D

... but despite my phpstan being configured to order the same way as yours does i cannot confirm these changes. Idk. But in you i trust.

kitsunet

ooooookkkkkaaaaaay :D

TASK: Cosmetic cleanup of DoctrineDbalContentGraphProjection

60753a8

Sorts `when`-matchers and -handlers alphabetically and moves other private methods to the bottom of the class

github-actions bot added Task task/cleanup-doctrinedbaladapter labels May 24, 2024

bwaidelich marked this pull request as draft May 24, 2024 17:51

bwaidelich mentioned this pull request May 25, 2024

!!! TASK: Add workspace content stream mapping to content graph projection #5096

Merged

12 tasks

Merge branch 'task/cleanup-doctrinedbaladapter' into task/cleanup-doc…

ddf84e4

…trinedbaladapter-2

Base automatically changed from task/cleanup-doctrinedbaladapter to 9.0 May 26, 2024 07:46

bwaidelich marked this pull request as ready for review May 26, 2024 07:46

github-actions bot added 9.0 and removed task/cleanup-doctrinedbaladapter labels May 26, 2024

mhsdesign closed this May 26, 2024

mhsdesign reopened this May 26, 2024

bwaidelich closed this May 26, 2024

bwaidelich reopened this May 26, 2024

mhsdesign approved these changes May 26, 2024

View reviewed changes

kitsunet approved these changes May 26, 2024

View reviewed changes

kitsunet merged commit 26fd7f5 into 9.0 May 26, 2024
20 checks passed

kitsunet deleted the task/cleanup-doctrinedbaladapter-2 branch May 26, 2024 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK: Cosmetic cleanup of `DoctrineDbalContentGraphProjection` #5092

TASK: Cosmetic cleanup of `DoctrineDbalContentGraphProjection` #5092

bwaidelich commented May 24, 2024 •

edited

Loading

mhsdesign commented May 26, 2024

kitsunet commented May 26, 2024

bwaidelich commented May 26, 2024

bwaidelich commented May 26, 2024 •

edited

Loading

mhsdesign commented May 26, 2024 •

edited

Loading

bwaidelich commented May 26, 2024

mhsdesign left a comment

bwaidelich commented May 26, 2024

mhsdesign commented May 26, 2024 •

edited

Loading

kitsunet left a comment

TASK: Cosmetic cleanup of DoctrineDbalContentGraphProjection #5092

TASK: Cosmetic cleanup of DoctrineDbalContentGraphProjection #5092

Conversation

bwaidelich commented May 24, 2024 • edited Loading

mhsdesign commented May 26, 2024

kitsunet commented May 26, 2024

bwaidelich commented May 26, 2024

bwaidelich commented May 26, 2024 • edited Loading

mhsdesign commented May 26, 2024 • edited Loading

bwaidelich commented May 26, 2024

mhsdesign left a comment

Choose a reason for hiding this comment

bwaidelich commented May 26, 2024

mhsdesign commented May 26, 2024 • edited Loading

kitsunet left a comment

Choose a reason for hiding this comment

TASK: Cosmetic cleanup of `DoctrineDbalContentGraphProjection` #5092

TASK: Cosmetic cleanup of `DoctrineDbalContentGraphProjection` #5092

bwaidelich commented May 24, 2024 •

edited

Loading

bwaidelich commented May 26, 2024 •

edited

Loading

mhsdesign commented May 26, 2024 •

edited

Loading

mhsdesign commented May 26, 2024 •

edited

Loading