search

neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 221 forks source link

TASK: Cosmetic cleanup of `DoctrineDbalContentGraphProjection` #5092

Closed bwaidelich closed 4 months ago

bwaidelich commented 4 months ago

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

mhsdesign commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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:

image

bwaidelich commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

bwaidelich commented 4 months ago

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

Awesome, I didn't know that one!

mhsdesign commented 4 months ago

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.