Mapped entity listeners are now processed by metadata exporters #6593

tPl0ch · 2017-08-01T10:15:58Z

The current implementation of the exporters are not taking the entity
listeners into account. I have added test cases for most of the edge
cases I could think of and implemented the Exporter handling.

This PR originates from #5864,
I was overwhelmed by the amount of conflicts I had to resolve so I
started anew on a clean master HEAD.

Ocramius · 2017-08-03T20:39:13Z

lib/Doctrine/ORM/Tools/Export/Driver/PhpExporter.php

+                    $lines[] = sprintf(
+                        '$metadata->addEntityListener(\'%s\', \'%s\', \'%s\');',
+                        $event,
+                        $entityListener['class'],


Where is this code tested? Can't seem to find it. Also, use var_export() instead of single quotes around a %s

Ocramius · 2017-08-03T20:40:22Z

lib/Doctrine/ORM/Tools/Export/Driver/XmlExporter.php

+            $entityListenerXml->addAttribute('class', $entityListener['class']);
+            $entityListenersXmlMap[$entityListener['class']] = $entityListenerXml;
+        } else {
+            $entityListenerXml = $entityListenersXmlMap[$entityListener['class']];


Use an early return. Swap the negation on the if to get rid of the else overall

Ocramius · 2017-08-03T20:40:44Z

lib/Doctrine/ORM/Tools/Export/Driver/YamlExporter.php

+     *
+     * @return array
+     */
+    private function processEntityListeners(ClassMetadataInfo $metadata, $array)


You can now add return hints (we're on PHP 7.1 ;-) )

Ocramius · 2017-08-03T20:40:52Z

lib/Doctrine/ORM/Tools/EntityGenerator.php

+     *
+     * @return string
+     */
+    protected function generateEntityListenerAnnotation(ClassMetadataInfo $metadata)


Return type

Ocramius · 2017-08-03T20:41:17Z

lib/Doctrine/ORM/Tools/Export/Driver/PhpExporter.php

+     * @param ClassMetadataInfo $metadata
+     * @param array             &$lines
+     */
+    private function processEntityListeners(ClassMetadataInfo $metadata, &$lines)


Can you remove the by-ref usage, and instead use array_merge() in the caller?

Ocramius · 2017-08-03T20:42:25Z

lib/Doctrine/ORM/Tools/Export/Driver/YamlExporter.php

+     *
+     * @return array
+     */
+    private function processEntityListenerConfig($array, $entityListenerConfig, $event)


Return type

Ocramius · 2017-08-03T20:42:34Z

tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php

+     *
+     * @param ClassMetadata $class
+     */
+    public function testEntityListenersGetExported($class)


Ocramius · 2017-08-03T20:42:44Z

tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php

@@ -371,6 +373,26 @@ public function testCascadeAllCollapsed()
        }
    }

+    /**
+     * @depends testExportedMetadataCanBeReadBackIn


Do you really want to use @depends here?

That's how all other export tests are structured, so I just adhered to the structure within the test case.

Ocramius · 2017-08-03T20:43:13Z

tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php

+        $this->assertNotEmpty($class->entityListeners);
+        $this->assertCount(2, $class->entityListeners[Events::prePersist]);
+        $this->assertCount(2, $class->entityListeners[Events::postPersist]);
+        $this->assertEquals(UserListener::class, $class->entityListeners[Events::prePersist][0]['class']);


Ah, this is the test that I was missing.

Ocramius · 2017-08-03T20:43:37Z

tests/Doctrine/Tests/ORM/Tools/Export/annotation/Doctrine.Tests.ORM.Tools.Export.User.php

@@ -5,6 +5,11 @@
 /**
 * @Entity
 * @HasLifecycleCallbacks
+ * @EntityListeners({
+ *     Doctrine\Tests\ORM\Tools\Export\UserListener::class,


These should probably be prefixed with \ to avoid import collisions

There is a weird issue with this, the resulting string is "\Doctrine\Tests\ORM\Tools\Export\UserListener", although ::class notation normally results in class names without root namespace:

<?php echo \SimpleXMLElement::class; // SimpleXMLElement

Using the ::class notation that way works as intended.

@tPl0ch gotcha, seems weird, but it's probably a doctrine/annotations bug.

Moved to doctrine/annotations#141

Ocramius · 2017-08-14T00:49:13Z

@tPl0ch see failures on https://travis-ci.org/doctrine/doctrine2/jobs/260960064#L594

tPl0ch · 2017-08-14T10:39:32Z

@Ocramius I think I have fixed the issues regarding the nullable return types. I totally forgot that a method that has no return statement now actually is none and not null. Also inversed a few conditions to have early returns. Gonna rebase and squash against the current HEAD.

The current implementation of the exporters are not taking the entity listeners into account. I have added test cases for most of the edge cases I could think of and implemented the Exporter handling. This PR originates from doctrine#5864, I was overwhelmed by the amount of conflicts I had to resolve so I started anew on a clean master HEAD. Squashed commits: - Code review aftermath - Add even more return type declarations - Added `return null` to methods declared with nullable return types - Removed unneeded docblocks when types are self-explanatory

Ocramius · 2017-08-15T08:22:39Z

lib/Doctrine/ORM/Tools/Export/Driver/AnnotationExporter.php

@@ -46,7 +46,7 @@ class AnnotationExporter extends AbstractExporter
     */
    public function exportClassMetadata(ClassMetadataInfo $metadata): string
    {
-        if ( ! $this->_entityGenerator) {
+        if (!$this->_entityGenerator) {


Eh, we use if ( ! $withTheSpace

This reverts commit ec3eed6.

tPl0ch · 2017-08-16T10:28:23Z

@Ocramius should be fine now (hopefully)...

@tPl0ch

…pe changes That's still to be considered a BC break, since child classes are broken if incompatible. Sorry @tPl0ch :-(

…string`

…ty-listeners' Close #6593 Close #5864 ge aborts

Ocramius · 2017-08-16T18:00:29Z

@tPl0ch manually merged via 76e2155

Sorry for the method hints - I had to revert them, since those classes are open for inheritance, so the additional hints are a BC break.

See https://3v4l.org/kSIhN for the specifics.

Ocramius · 2017-08-16T18:03:36Z

Also, thanks for enduring the review hell we generally fire in contributors' direction :-)

tPl0ch · 2017-08-16T19:10:44Z

@Ocramius is there a phpcs standards configuration file for Doctrine so that all that CS issues can be caught during development?

Ocramius · 2017-08-16T19:44:59Z

There is, but we don't apply CS changes yet due to the amount of open PRs that would conflict for no good reason

Ocramius · 2017-08-16T19:45:41Z

https://github.com/doctrine/coding-standard

…

On 16 Aug 2017 9:10 PM, "Thomas Ploch" ***@***.***> wrote: @Ocramius <https://github.com/ocramius> is there a phpcs standards configuration file for Doctrine so that all that CS issues can be caught during development? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6593 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakDnXSzCZ5F10qVpA0dtIVkMNhAJiks5sYz61gaJpZM4OphCN> .

tPl0ch mentioned this pull request Aug 1, 2017

Entity listeners are now processed by exporters #5864

Closed

Ocramius suggested changes Aug 3, 2017

View reviewed changes

Ocramius added the Improvement label Aug 3, 2017

Ocramius assigned tPl0ch Aug 3, 2017

Ocramius mentioned this pull request Aug 14, 2017

::class notations with FQCN notation and leading \ causes \ to be part of the produced string value doctrine/annotations#141

Closed

tPl0ch force-pushed the hotfix-entity-listener-export branch 2 times, most recently from b3d4e8c to 83497d0 Compare August 14, 2017 08:14

tPl0ch force-pushed the hotfix-entity-listener-export branch from 83497d0 to d485c18 Compare August 14, 2017 10:41

tPl0ch force-pushed the hotfix-entity-listener-export branch from d485c18 to b7ae5b4 Compare August 15, 2017 08:09

Minor coding style fixes and more docblock removal

ec3eed6

Ocramius changed the title ~~Entity listeners are now processed by exporters~~ Mapped entity listeners are now processed by metadata exporters Aug 15, 2017

Ocramius suggested changes Aug 15, 2017

View reviewed changes

tPl0ch added 5 commits August 15, 2017 10:28

Revert "Minor coding style fixes and more docblock removal"

741819f

This reverts commit ec3eed6.

[CS] Remove obsolete doc blocks

fe88422

[CS] Remove obsolete doc blocks doctrine#2

5998655

[FX] Looking up PHP internal functions from the root namespace is faster

b1e091b

[FX] Use single quotes for XML root in XmlExporter

e4006e5

Ocramius added this to the 2.6.0 milestone Aug 16, 2017

Ocramius assigned Ocramius and unassigned tPl0ch Aug 16, 2017

Ocramius merged commit e4006e5 into doctrine:master Aug 16, 2017

Ocramius added a commit that referenced this pull request Aug 16, 2017

#6593 #5864 reverted all public/protected API parameter and return ty…

e1acba3

…pe changes That's still to be considered a BC break, since child classes are broken if incompatible. Sorry @tPl0ch :-(

Ocramius added a commit that referenced this pull request Aug 16, 2017

#6593 #5864 removing incorrect null return value in API marked as `…

894490c

…string`

Ocramius added a commit that referenced this pull request Aug 16, 2017

Merge branch 'feature/#6593-#5864-export-metadata-should-contain-enti…

76e2155

…ty-listeners' Close #6593 Close #5864 ge aborts

tPl0ch deleted the hotfix-entity-listener-export branch August 16, 2017 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mapped entity listeners are now processed by metadata exporters #6593

Mapped entity listeners are now processed by metadata exporters #6593

tPl0ch commented Aug 1, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

tPl0ch Aug 4, 2017

Ocramius Aug 3, 2017

Ocramius Aug 3, 2017

tPl0ch Aug 4, 2017 •

edited

Loading

Ocramius Aug 14, 2017

Ocramius Aug 14, 2017

Ocramius commented Aug 14, 2017

tPl0ch commented Aug 14, 2017 •

edited

Loading

Ocramius Aug 15, 2017

tPl0ch commented Aug 16, 2017

Ocramius commented Aug 16, 2017

Ocramius commented Aug 16, 2017

tPl0ch commented Aug 16, 2017

Ocramius commented Aug 16, 2017

Ocramius commented Aug 16, 2017 via email

Mapped entity listeners are now processed by metadata exporters #6593

Mapped entity listeners are now processed by metadata exporters #6593

Conversation

tPl0ch commented Aug 1, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tPl0ch Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Aug 14, 2017

tPl0ch commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

tPl0ch commented Aug 16, 2017

Ocramius commented Aug 16, 2017

Ocramius commented Aug 16, 2017

tPl0ch commented Aug 16, 2017

Ocramius commented Aug 16, 2017

Ocramius commented Aug 16, 2017 via email

tPl0ch Aug 4, 2017 •

edited

Loading

tPl0ch commented Aug 14, 2017 •

edited

Loading