Reintroduce HasLifecycleCallbacks annotation. #428

Merged
merged 13 commits into from Jan 9, 2014

4 participants

@jwage
Doctrine member

No description provided.

@jwage
Doctrine member

ping @kriswallsmith do you remember what it was you removed HasLifecycleCallbacks for? I want to make sure we don't regress whatever you were fixing. All the tests are passing after I reintroduce this flag to fix #427

@kriswallsmith
Doctrine member
@jwage
Doctrine member

@jmikola I want to reintroduce this so it is explicit whether or not we will check and register lifecycle callbacks. Thoughts?

@jmikola
Doctrine member

That sounds like it'd help the fellow in #474, as it'd allow us to annotate the methods on either a @Document or @MappedSuperclass but not actually register them unless the class, or inheriting class, has @HasLifecycleCallbacks. Is that correct?

If so, it'd be helpful to have some tests to illustrate how this works -- I imagine they've since been removed in the current code base.

@kriswallsmith
Doctrine member

Can we rename the annotation to @HasMethodAnnotations since @AlsoLoad doesn't constitute a lifecycle callback?

@jmikola Methods will only be scanned on classes marked as having method annotations, so I don't think this would help #474.

@jwage
Doctrine member

I think it it would make more sense to keep the same name, so it matches the ORM, and move @AlsoLoad to be outside of that check.

@jwage
Doctrine member

I think the issue in #474 is it's registering lifecycle callbacks multiple times since it's not an explicit check for whether or not they should be registered. i.e. the class has lifecyclecallbacks defined but we don't want to scan them and register them.

@kriswallsmith
Doctrine member

If we move @AlsoLoad outside of that check then we're back to where we were before — always scanning every method for annotations but ignoring the lifecycle callbacks if the proper annotation is not on the class…

@jwage
Doctrine member

I think that is okay now. Previously the only concern causing me to have HasLifecycleCallbacks was performance to avoid scanning the methods when not necessary, but it has an actual purpose. You may want to define mappings for lifecycle callbacks but not register them.

@kriswallsmith
Doctrine member

Do the other drivers support defining mappings for lifecycle callbacks but not registering them?

@jwage
Doctrine member

Yes, that is one of the purposes or side effects of HasLifecycleCallbacks. You can map a PostUpdate and it will not be registered unless HasLifecycleCallbacks.

@kriswallsmith
Doctrine member

I don't see anything in either the XML or YAML drivers that would allow one to define a callback but not register it… are we talking past each other again?

My understanding of the @HasLifecycleCallbacks annotation is that it forces you to opt-in to scanning every method in your model class for performance reasons.

@gpapakyriakopoulos

is this fix going to be merged ?

We are currently experiencing the same problem with dual execution of life-cycle event callbacks when having inheritance from a mapped super-class to a document.

It seems that both the mapped super-class event callbacks and the child document class event callbacks are registered which results in dual execution of the actual callbacks.

@jmikola
Doctrine member

@kriswallsmith / @jwage: This is the first I've seen of @AlsoLoad on methods. It appears to only be supported in annotations. Is that correct?

I don't see any logic in the XML or YAML drivers that would set ClassMetadataInfo::$alsoLoadMethods, which is in turn read by HydratorFactory::hydrate().

Noting @kriswallsmith's comments:

If we move @AlsoLoad outside of that check then we're back to where we were before — always scanning every method for annotations but ignoring the lifecycle callbacks if the proper annotation is not on the class…

My understanding of the @HasLifecycleCallbacks annotation is that it forces you to opt-in to scanning every method in your model class for performance reasons.

I don't believe @HasLifecycleCallbacks was just about performance, since it seems necessary to control the exact classes (in a possible inheritance tree) that we should invoke the methods. Multiple annotations throughout the tree will cause multiple invocations. I suppose if a mapper superclass used @HasLifecycleCallbacks and we wanted to override the method, we should just redefine the method but not annotate it in the child class, which would ensure that the child implementation gets invoked once by name?

I would say that @AlsoLoad belongs outside of the @HasLifecycleCallbacks check, since it's not really a lifecycle callback. Performance for loading annotation metadata wouldn't be worse than it is currently, since we're already scanning all method annotations.

@jwage
Doctrine member

👍 makes sense to me

@jmikola jmikola added a commit that referenced this pull request Oct 21, 2013
@jmikola jmikola Allow AlsoLoad annotation on methods without HasLifecycleCallbacks
AlsoLoad will be handled differently from actual lifecycle callback methods. See discussion in #428 for additional context.
64700f1
@jmikola
Doctrine member

@jwage / @kriswallsmith: Please review.

@kriswallsmith
Doctrine member

If it's not about scanning all of those methods, what is the purpose of this annotation? It still doesn't make sense to me.

@jmikola
Doctrine member

This annotation still controls scanning for the lifecycle callback methods, just not @AlsoLoad.

Re-introducing the @HasLifecycleCallbacks annotation would solve situations described in #474 and #427 since users would have explicit control over which methods get registered/triggered, but they could certainly run into the same problem with @AlsoLoad down the line.

But re-reading those tickets now, it probably is a bug that the reflection picks up inherited methods again. I'll look into whether we can work around that in the AnnotationDriver. @Bilge's case (#427) is definitely bug-worthy.

@jmikola
Doctrine member

@jwage / @kriswallsmith / @ocramius: Changed so that @AlsoLoad is only processed if @HasLifecycleCallbacks is present, to be consistent with ORM's handling of method annotations (even though it doesn't have @AlsoLoad). I replaced the test for #427 in @jwage's original commit with a more comprehensive test case, which should cover all of the permutations of annotations and inheritance.

@kriswallsmith kriswallsmith and 1 other commented on an outdated diff Oct 22, 2013
lib/Doctrine/ODM/MongoDB/Tools/DocumentGenerator.php
@@ -544,6 +544,10 @@ private function generateDocumentDocBlock(ClassMetadataInfo $metadata)
$lines[] = ' * )';
}
+ if ($metadata->lifecycleCallbacks) {
@kriswallsmith
Doctrine member

Is @AlsoLoad going to trigger this?

@jmikola
Doctrine member
jmikola added a note Oct 22, 2013

Nope. Good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriswallsmith
Doctrine member

Since @AlsoLoad isn't technically a lifecycle callback, shouldn't the annotation have a different name?

@jmikola
Doctrine member

Based on HydratorFactory::hydrate(), the @AlsoLoad annotation on a method will cause the method to be invoked after PreLoad but before hydration and PostLoad. In that sense, it could simply be reimplemented as a PreLoad callback.

I agree it isn't technically such a callback, but I think consistency with ORM is a bit point in the plus column.

@kriswallsmith
Doctrine member

Well, the ORM doesn't have an @AlsoLoad annotation…

If consistency with the ORM is a big deal, I would add the @HasLifecycleCallbacks annotation back, but leave @AlsoLoad outside of its scope. If performance of the driver is a big deal, I would introduce a @HasMethodAnnotations annotation.

Just my $.02 from the peanut gallery…

@jwage
Doctrine member

@jmikola what do you think about merging this?

jwage and others added some commits Oct 10, 2012
@jwage jwage Reintroduce HasLifecycleCallbacks annotation f85eab0
@jmikola jmikola Revise "Migrating Schemas" documentation 9125af7
@jmikola jmikola Fix RST title syntax cf2ce8b
@jmikola jmikola Document reintroduced HasLifecycleCallbacks documentation
This documentation was originally removed in abcf275, after the annotation itself was removed in e9af249.
0996fa6
@jmikola jmikola AnnotationDriver should ignore inherited methods
Only consider methods declared in the current class being processed. This is consistent with ORM's AnnotationDriver.

See: doctrine/doctrine2@67ae22b
7b08ec3
@jmikola jmikola Improve AlsoLoad annotation documentation
Discuss how AlsoLoad may be used for methods, and link to migrations page.
e6f250e
@jmikola jmikola Handle AlsoLoad methods like properties and without HasLifecycleCallb…
…acks

AlsoLoad method annotations will no longer require the HasLifecycleCallbacks class annotation. The latter will just be used for lifecycle callbacks.

To be consistent with property handling, an AlsoLoad method may be registered with multiple fields. Fields will be considered in order and the method will be invoked at most once with the first value found.

Wrote additional functional tests for AlsoLoad behavior for both properties and methods.
82245cb
@jmikola jmikola Inherit AlsoLoad methods from parent's ClassMetadata 1279266
@jmikola jmikola wip 18c3c76
@jmikola jmikola commented on an outdated diff Jan 9, 2014
...ongoDB/Tests/Functional/HasLifecycleCallbacksTest.php
+ $this->assertCount(1, $document->invoked);
+ $this->assertEquals('sub', $document->invoked[0]);
+ }
+
+ public function testHasLifecycleCallbacksSubOverrideAnnotatedExtendsSuperAnnotated()
+ {
+ $document = new HasLifecycleCallbacksSubOverrideAnnotatedExtendsSuperAnnotated();
+ $this->dm->persist($document);
+ $this->dm->flush();
+
+ /* Since both classes are annotated and declare the method, the callback
+ * is registered twice and the sub-class is invoked two times.
+ */
+ $this->assertCount(2, $document->invoked);
+ $this->assertEquals('sub', $document->invoked[0]);
+ $this->assertEquals('sub', $document->invoked[1]);
@jmikola
Doctrine member
jmikola added a note Jan 9, 2014

We should only invoke one method. Will be updated after we agree on some consistent behavior with the ORM folks. See doctrine/doctrine2#903.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
jmikola added some commits Jan 9, 2014
@jmikola jmikola Optimize checks for lifecycle callbacks and listeners 8057c71
@jmikola jmikola Do not allow duplicate lifecycle callbacks to be registered
Using the method name as a key in lifecycleCallbacks will ensure that methods are never registered (and invoked) multiple times. We'll still use the method name as the value so existing code can iterate over values in a lifecycleCallbacks event array to get method names.

Fixes #427, #474, and #695. Related to doctrine/doctrine2#903.
59a67f3
@jmikola jmikola ClassMetadata should only invoke lifecycle callbacks on its own objects
Since each mapped document class has its own ClassMetadata, we can expect the exact class and reject inheriting classes.
5bb9afa
@jmikola jmikola Changelog entry for HasLifecycleCallbacks and AlsoLoad 1d455d2
@jmikola
Doctrine member

@kriswallsmith:

If consistency with the ORM is a big deal, I would add the @HasLifecycleCallbacks annotation back, but leave @AlsoLoad outside of its scope.

It shall be so.

@jmikola jmikola merged commit dd838e5 into master Jan 9, 2014
@jmikola jmikola deleted the fix/gh427 branch Jan 16, 2014
@jmikola
Doctrine member

Ack! I forgot to rebase this with a proper commit message :)

@jmikola jmikola added a commit that referenced this pull request Apr 1, 2014
@jmikola jmikola Restore ClassMetadataInfo::$lifecycleCallbacks array structure (fixes #…
…803)

This restores the structure before 59a67f3 (#428) to be consistent with ORM while still ensuring that the same callback is not registered multiple times.
627b77c
@jmikola jmikola added a commit that referenced this pull request Apr 1, 2014
@jmikola jmikola Restore ClassMetadataInfo::$lifecycleCallbacks array structure (fixes #…
…803)

This restores the structure before 59a67f3 (#428) to be consistent with ORM while still ensuring that the same callback is not registered multiple times.
f0d8ebe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment