Feature/many to many support #509

pietaj · 2022-08-18T12:14:16Z

I am targeting this branch, because the bundle does not support m:n auditing and the subject was already mentioned
here #302, and here #321 but was never finished

Changelog

### Added
- Support for ManyToMany

VincentLanglet · 2022-08-18T12:33:02Z

I fixed the cs in #510.
Please rebase and revert the extra style changes.

We don't work on this library a lot so the more the PR will be small, the easier it will be for us to review ^^

And take a look at the build ;)

pietaj · 2022-08-22T13:41:46Z

I've rebased the code and reverted the style changes.

I've also left the deprecation fixes out of this PR as it makes the code harder to review.

pietaj · 2022-08-24T19:47:50Z

@VincentLanglet I've checked the stan / psalm issues reported by the CI and I can fix them, no problem, but this will make the code changes in this PR even more complex, as there are quite a lot of things to review already. Shall I proceed with the fixes and throw them all in a separate commit? What would be the most convenient way to proceed?

VincentLanglet · 2022-08-24T19:50:43Z

src/AuditReader.php

+                    $tableName = $this->config->getTablePrefix(
+                        ).$assoc['joinTable']['name'].$this->config->getTableSuffix();


Suggested change

$tableName = $this->config->getTablePrefix(

).$assoc['joinTable']['name'].$this->config->getTableSuffix();

$tableName = $this->config->getTablePrefix()

.$assoc['joinTable']['name']

.$this->config->getTableSuffix();

VincentLanglet · 2022-08-24T19:51:07Z

src/AuditReader.php

+                    $query = 'SELECT '.implode(
+                            ', ',
+                            $columnList
+                        ).' FROM '.$tableName.' e WHERE '.$whereSQL.' ORDER BY e.'.$this->config->getRevisionFieldName(
+                        ).' DESC';


Prefer using sprintf

VincentLanglet · 2022-08-24T19:51:35Z

src/AuditReader.php

+                        $tableName = $this->config->getTablePrefix(
+                            ).$targetAssoc['joinTable']['name'].$this->config->getTableSuffix();


Suggested change

$tableName = $this->config->getTablePrefix(

).$targetAssoc['joinTable']['name'].$this->config->getTableSuffix();

$tableName = $this->config->getTablePrefix()

.$targetAssoc['joinTable']['name']

.$this->config->getTableSuffix();

VincentLanglet · 2022-08-24T19:51:45Z

src/AuditReader.php

+                        $query = 'SELECT '.implode(
+                                ', ',
+                                $columnList
+                            ).' FROM '.$tableName.' e WHERE '.$whereSQL.' ORDER BY e.'.$this->config->getRevisionFieldName(
+                            ).' DESC';


prefer sprintf

VincentLanglet · 2022-08-24T19:53:45Z

src/EventListener/LogRevisionsListener.php

+     * @return string
+     *
+     * @psalm-param ClassMetadata<object> $targetClass
+     * @psalm-param array{cache?: array, cascade: array<string>, declared?: class-string, fetch: mixed, fieldName: string, id?: bool, inherited?: class-string, indexBy?: string, inversedBy: null|string, isCascadeRemove: bool, isCascadePersist: bool, isCascadeRefresh: bool, isCascadeMerge: bool, isCascadeDetach: bool, isOnDeleteCascade?: bool, isOwningSide: true, joinColumns?: array<array{name: string, referencedColumnName: string, unique?: bool, quoted?: bool, fieldName?: string, onDelete?: string, columnDefinition?: string, nullable?: bool}>, joinColumnFieldNames?: array<string, string>, joinTable?: array, joinTableColumns?: list<mixed>, mappedBy: null|string, orderBy?: array, originalClass?: class-string, originalField?: string, orphanRemoval?: bool, relationToSourceKeyColumns?: array, relationToTargetKeyColumns?: array, sourceEntity: class-string, sourceToTargetKeyColumns?: array<string, string>, targetEntity: class-string, targetToSourceKeyColumns?: array<string, string>, type: int, unique?: bool} $assoc


You can use @phpstan-import-type AssociationMapping from ... https://github.com/doctrine/orm/blob/2.13.x/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L110
instead on the class level.

And then use it in the phpdoc.

VincentLanglet · 2022-08-24T19:54:15Z

src/EventListener/LogRevisionsListener.php

@@ -389,6 +426,49 @@ private function getInsertRevisionSQL(ClassMetadata $class): string
        return $this->insertRevisionSQL[$class->name];
    }

+    /**
+     * @param ClassMetadata $class


Suggested change

* @param ClassMetadata $class

* @param ClassMetadata<object> $class

avoid the need of psalm-param

VincentLanglet · 2022-08-24T19:54:24Z

src/EventListener/LogRevisionsListener.php

+     *
+     * @return string
+     *
+     * @psalm-param ClassMetadata<object> $targetClass


We prefer phpstan-param

VincentLanglet · 2022-08-24T19:55:24Z

src/EventListener/LogRevisionsListener.php

@@ -403,34 +483,51 @@ private function saveRevisionEntityData(ClassMetadata $class, array $entityData,
            if ($class->isInheritanceTypeJoined() && $class->isInheritedAssociation($field)) {
                continue;
            }
-            if (
-                ($assoc['type'] & ClassMetadata::TO_ONE) === 0
+            if (!(($assoc['type'] & ClassMetadata::TO_ONE) === 0


Change !($a || $b ||$c) to !a && !b && !c

VincentLanglet · 2022-08-24T19:57:17Z

@VincentLanglet I've checked the stan / psalm issues reported by the CI and I can fix them, no problem, but this will make the code changes in this PR even more complex, as there are quite a lot of things to review already. Shall I proceed with the fixes and throw them all in a separate commit? What would be the most convenient way to proceed?

Yes please fix the issues.

Lot of them where already fix on some others places of the code with some if checks or some assert. You can do the same.

pietaj · 2022-08-24T20:02:11Z

@VincentLanglet I've checked the stan / psalm issues reported by the CI and I can fix them, no problem, but this will make the code changes in this PR even more complex, as there are quite a lot of things to review already. Shall I proceed with the fixes and throw them all in a separate commit? What would be the most convenient way to proceed?

Yes please fix the issues.

Lot of them where already fix on some others places of the code with some if checks or some assert. You can do the same.

Oh my, thanks for the ultra fast reply, I didn't mean to push ;) I will fix all issues and will come back to you after everything will pass green

SonataCI · 2022-09-04T08:21:21Z

Could you please rebase your PR and fix merge conflicts?

VincentLanglet · 2022-09-15T05:12:24Z

Any issue @pietaj ? :)

@halilibrahim

Code ported from halilibrahim@862974f thx @halilibrahim§

@halilibrahim

Code ported from halilibrahim@474a29c thx @halilibrahim

@halilibrahim

Code ported from halilibrahim@20e703c thx @halilibrahim

pietaj · 2022-09-23T13:36:19Z

My only issue was my day job :) did the requested psalm / phpstan compliance cleanup and the requested changes

VincentLanglet · 2022-09-23T13:45:02Z

My only issue was my day job :) did the requested psalm / phpstan compliance cleanup and the requested changes

Seems like build is still failing

pietaj · 2022-09-23T14:03:00Z

That's odd ... both rector and phpstan pass on dev machine. I'll have a closer look

VincentLanglet · 2022-09-23T14:05:17Z

That's odd ... both rector and phpstan pass on dev machine. I'll have a closer look

Tru to update your dependencies maybe ?

pietaj · 2022-09-23T14:10:48Z

yeah, thanks for the tip. Outdated dependencies, I'll rerun and fix the rest

pietaj · 2022-09-26T09:06:38Z

@VincentLanglet one of the failing checks is linked with rector, failing with

PHP Fatal error: Uncaught _PHPStan_9a6ded56a\Nette\DI\InvalidConfigurationException: The item 'conditionalTags › PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule › phpstan.rules.rule' expects to be bool, array given. in phar:///home/runner/work/EntityAuditBundle/EntityAuditBundle/vendor/rector/rector/vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Compiler.php:219

I did some quick checks and the error is linked with phpstan/phpstan bumping from 1.8.5 => 1.8.6. Did you encounter a similar problems in other sonata projects as well?

jordisala1991 · 2022-09-26T09:20:16Z

@VincentLanglet one of the failing checks is linked with rector, failing with

PHP Fatal error: Uncaught _PHPStan_9a6ded56a\Nette\DI\InvalidConfigurationException: The item 'conditionalTags › PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule › phpstan.rules.rule' expects to be bool, array given. in phar:///home/runner/work/EntityAuditBundle/EntityAuditBundle/vendor/rector/rector/vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Compiler.php:219

I did some quick checks and the error is linked with phpstan/phpstan bumping from 1.8.5 => 1.8.6. Did you encounter a similar problems in other sonata projects as well?

Yes, ignore rector for now.

FYI, error already reported: rectorphp/rector#7509, waiting for a fix. (The most you can do is upgrade to rector ^0.14, to ensure we will get the fix.

VincentLanglet

It's ok for me, except some formatting issues.
Is this PR ok for you @jordisala1991 ?

VincentLanglet · 2022-09-27T21:27:46Z

src/AuditReader.php

+            $tableAlias = $classMetadata->isInheritanceTypeJoined() && $classMetadata->isInheritedField(
+                $field
+            ) && !$classMetadata->isIdentifier($field)


This reformatting is weird. I would prefer

Suggested change

$tableAlias = $classMetadata->isInheritanceTypeJoined() && $classMetadata->isInheritedField(

$field

) && !$classMetadata->isIdentifier($field)

$tableAlias = $classMetadata->isInheritanceTypeJoined()

&& $classMetadata->isInheritedField($field)

&& !$classMetadata->isIdentifier($field)

VincentLanglet · 2022-09-27T21:28:44Z

src/AuditReader.php

+                $tableAlias = $classMetadata->isInheritanceTypeJoined() && $classMetadata->isInheritedField(
+                    $field
+                ) && !$classMetadata->isIdentifier($field)


Suggested change

$tableAlias = $classMetadata->isInheritanceTypeJoined() && $classMetadata->isInheritedField(

$field

) && !$classMetadata->isIdentifier($field)

$tableAlias = $classMetadata->isInheritanceTypeJoined()

&& $classMetadata->isInheritedField($field)

&& !$classMetadata->isIdentifier($field)

VincentLanglet · 2022-09-27T21:29:17Z

src/AuditReader.php

+            'INNER JOIN '.$tableName.' e ON r.id = e.'.$this->config->getRevisionFieldName(
+            ).' WHERE '.$whereSQL.' ORDER BY r.id DESC';


this formatting is weird

VincentLanglet · 2022-09-27T21:29:38Z

src/AuditReader.php

+                            $otherEntityAssoc = $this->em->getClassMetadata(
+                                $targetEntity
+                            )->associationMappings[$mappedBy];


this formatting is weird

VincentLanglet · 2022-09-27T21:31:18Z

src/EventListener/LogRevisionsListener.php

+     * @param ClassMetadata        $class
+     * @param array<string, mixed> $assoc
+     *
+     * @psalm-param ClassMetadata<object> $targetClass


Suggested change

* @psalm-param ClassMetadata<object> $targetClass

* @phpstan-param ClassMetadata<object> $targetClass

VincentLanglet · 2022-09-27T21:31:46Z

src/EventListener/LogRevisionsListener.php

+            && isset($assoc['relationToSourceKeyColumns'], $assoc['relationToTargetKeyColumns'], $assoc['joinTable']['name'])) {
+            $placeholders = ['?', '?'];
+
+            // $tableName = $this->config->getTableName($class);


Why adding a code commented ? can we remove it ?

jordisala1991

Do we need more information on the readme, or should we only remove the last todo?

tests/RelationTest.php

jordisala1991 · 2022-09-27T21:46:55Z

This Pr also needs a changelog.

The version bump is necessary because phpstan/phpstan bump 1.8.5 => 1.8.6 breaks rector < 0.14.4 @see rectorphp/rector#7509

jordisala1991 · 2022-10-08T19:11:31Z

Thanks @pietaj

jordisala1991 · 2022-10-08T19:12:27Z

I guess the todo list on the readme should be updated too.

VincentLanglet added the minor label Aug 18, 2022

VincentLanglet requested a review from a team August 18, 2022 12:35

pietaj force-pushed the feature/many-to-many-support branch from 5d79c19 to ce94043 Compare August 22, 2022 13:37

VincentLanglet requested changes Aug 24, 2022

View reviewed changes

SonataCI added the pending author label Sep 4, 2022

halilibrahim and others added 9 commits September 23, 2022 13:03

Add audit tables for m:n relations

0e2a21e

Code ported from halilibrahim@862974f thx @halilibrahim§

Add reading audited m:n relations

6cbbf2d

Code ported from halilibrahim@474a29c thx @halilibrahim

Add writing audited m:n relations

e597bcc

Code ported from halilibrahim@20e703c thx @halilibrahim

Add m:n tests

edcfc9f

fixup! Add writing audited m:n relations

72b7e23

Style changes

832b09b

Psalm fixes

356b1ab

PhpStan fixes

b4346e0

More psalm fixes

1e5aa67

pietaj force-pushed the feature/many-to-many-support branch from ce94043 to 1e5aa67 Compare September 23, 2022 13:31

SonataCI removed the pending author label Sep 23, 2022

Use sprintf instead of string contat for queries

fcb9d70

Comply with cs-fixer rules

2749c4f

VincentLanglet requested changes Sep 27, 2022

View reviewed changes

jordisala1991 reviewed Sep 27, 2022

View reviewed changes

tests/RelationTest.php Outdated Show resolved Hide resolved

jordisala1991 added the Needs: Changelog note label Sep 27, 2022

Bump rector to ^0.14

715b1de

The version bump is necessary because phpstan/phpstan bump 1.8.5 => 1.8.6 breaks rector < 0.14.4 @see rectorphp/rector#7509

VincentLanglet previously approved these changes Sep 28, 2022

View reviewed changes

VincentLanglet removed the Needs: Changelog note label Sep 28, 2022

VincentLanglet requested a review from jordisala1991 September 28, 2022 10:04

Cleanup code formatting

66f7124

pietaj dismissed VincentLanglet’s stale review via 66f7124 September 28, 2022 10:05

pietaj force-pushed the feature/many-to-many-support branch from e2f14fd to 66f7124 Compare September 28, 2022 10:05

VincentLanglet approved these changes Sep 28, 2022

View reviewed changes

jordisala1991 approved these changes Oct 8, 2022

View reviewed changes

jordisala1991 merged commit 5a98e1a into sonata-project:1.x Oct 8, 2022

VincentLanglet mentioned this pull request Oct 12, 2022

Many To Many audit relation fail with new entity in the relation. #522

Closed

X-Coder264 mentioned this pull request Feb 13, 2023

Fix schema creation for self referencing many to many association #537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/many to many support #509

Feature/many to many support #509

pietaj commented Aug 18, 2022 •

edited by VincentLanglet

Loading

VincentLanglet commented Aug 18, 2022 •

edited

Loading

pietaj commented Aug 22, 2022

pietaj commented Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet Aug 24, 2022

VincentLanglet commented Aug 24, 2022

pietaj commented Aug 24, 2022

SonataCI commented Sep 4, 2022

VincentLanglet commented Sep 15, 2022

pietaj commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

pietaj commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

pietaj commented Sep 23, 2022

pietaj commented Sep 26, 2022

jordisala1991 commented Sep 26, 2022 •

edited

Loading

VincentLanglet left a comment

VincentLanglet Sep 27, 2022

VincentLanglet Sep 27, 2022

VincentLanglet Sep 27, 2022

VincentLanglet Sep 27, 2022

VincentLanglet Sep 27, 2022

VincentLanglet Sep 27, 2022

jordisala1991 left a comment

jordisala1991 commented Sep 27, 2022

jordisala1991 commented Oct 8, 2022

jordisala1991 commented Oct 8, 2022

		$tableName = $this->config->getTablePrefix(
		).$assoc['joinTable']['name'].$this->config->getTableSuffix();

		$tableName = $this->config->getTablePrefix(
		).$targetAssoc['joinTable']['name'].$this->config->getTableSuffix();

	* @param ClassMetadata $class
	* @param ClassMetadata<object> $class

		'INNER JOIN '.$tableName.' e ON r.id = e.'.$this->config->getRevisionFieldName(
		).' WHERE '.$whereSQL.' ORDER BY r.id DESC';

	* @psalm-param ClassMetadata<object> $targetClass
	* @phpstan-param ClassMetadata<object> $targetClass

Feature/many to many support #509

Feature/many to many support #509

Conversation

pietaj commented Aug 18, 2022 • edited by VincentLanglet Loading

Changelog

VincentLanglet commented Aug 18, 2022 • edited Loading

pietaj commented Aug 22, 2022

pietaj commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Aug 24, 2022

pietaj commented Aug 24, 2022

SonataCI commented Sep 4, 2022

VincentLanglet commented Sep 15, 2022

pietaj commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

pietaj commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

pietaj commented Sep 23, 2022

pietaj commented Sep 26, 2022

jordisala1991 commented Sep 26, 2022 • edited Loading

VincentLanglet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordisala1991 left a comment

Choose a reason for hiding this comment

jordisala1991 commented Sep 27, 2022

jordisala1991 commented Oct 8, 2022

jordisala1991 commented Oct 8, 2022

pietaj commented Aug 18, 2022 •

edited by VincentLanglet

Loading

VincentLanglet commented Aug 18, 2022 •

edited

Loading

jordisala1991 commented Sep 26, 2022 •

edited

Loading