Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

DDC-1955 - @EntityListeners #423

Merged
merged 41 commits into from Feb 2, 2013

Conversation

Projects
None yet
10 participants
Owner

FabioBatSilva commented Aug 12, 2012

http://www.doctrine-project.org/jira/browse/DDC-1955

Hi.

This path adds support for @EntityListeners

This path add another way to handle events
allow configure the same listener for many specific entities.
And give the EventArg in the current lifecycle callback system.

Usage :

<?php
/**
 * @EntityListeners({"ContractListener"})
 * @HasLifecycleCallbacks
 * @Entity  
 */
class Contract
{
    /** @PostLoad */
    public function postLoadHandler(LifecycleEventArgs $event)
    {
        // do something
    }
}
class ContractListener
{
    /** @PrePersist  */
    public function prePersistHandler(Contract $contract)
    {
        // do something
    }
    /**
     * @PostPersist
     * Most of cases just the entity is needed.
     * as a second parameter LifecycleEventArgs allow access to the entity manager.
     */
    public function postPersistHandler(Contract $contract, LifecycleEventArgs $args)
    {
        // do something
    }
}

This pull request fails (merged 0bc4ef76 into 72ce9a7).

@fprochazka fprochazka and 2 others commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+
+ /**
+ * Call the entity listeners.
+ *
+ * @param string $eventName The event name.
+ * @param object $entity An instance of the mapped entity
+ * @param \Doctrine\Common\EventArgs $arg The Event args
+ */
+ public function dispatchEntityListeners($eventName, $entity, EventArgs $arg)
+ {
+ foreach ($this->entityListeners[$eventName] as $listener) {
+ $class = $listener['class'];
+ $method = $listener['method'];
+
+ if ( ! isset(self::$entityListenerInstances[$class])) {
+ self::$entityListenerInstances[$class] = new $class();
@fprochazka

fprochazka Aug 12, 2012

Contributor

I should be capable of providing my own instance. What if the listener requires something other, like services?

@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

I also have same question as @hosiplan

@FabioBatSilva

FabioBatSilva Aug 12, 2012

Owner

I agree with you guys.

For sure this is not the best approach.
But the current events system can't handle this without create a event manager per entity class.
And we don't have a DI to store shared instances.

If you have a better solution are welcome :)

@fprochazka fprochazka and 1 other commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -424,6 +425,18 @@ class ClassMetadataInfo implements ClassMetadata
public $lifecycleCallbacks = array();
/**
+ * READ-ONLY: The registered entity listeners.
+ *
+ * @var array
+ */
+ public $entityListeners = array();
+
+ /**
+ * @var array entity listeners instances.
+ */
+ static private $entityListenerInstances = array();
@fprochazka

fprochazka Aug 12, 2012

Contributor

Static? Really?

@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

How would this work with serialized/cached instances?

@fprochazka

fprochazka Aug 12, 2012

Contributor

I wouldn't store the instances in ClassMetadata. I would create new class, that would provide them. Or EventManager could provide them.

Contributor

fprochazka commented Aug 12, 2012

If you figure out, how to fetch the listeners from EventManager (and incorporate it), this could be a lot better.

Contributor

Koc commented Aug 12, 2012

IMHO better solve this problem from other side

<?php

$eventManager->addEventListener(
    array(Events::prePersist, Events::postPersist),
    new ContractListener(),
    $applyOnlyToEntity = `Contract`
);

@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
}
+ $entityResult['entityClass'] = $this->fullyQualifiedClassName($entityResult['entityClass']);
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -2356,6 +2356,51 @@ public function setLifecycleCallbacks(array $callbacks)
}
/**
+ * Adds a entity listener for entities of this class.
+ *
+ * @param string $callback
+ * @param string $eventName
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

Missing throws documentation

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ * @param string $eventName
+ */
+ public function addEntityListener($eventName, $class, $method)
+ {
+ $class = $this->fullyQualifiedClassName($class);
+
+ if ( ! class_exists($class)) {
+ throw MappingException::entityListenerClassNotFound($class, $this->name);
+ }
+
+ if ( ! method_exists($class, $method)) {
+ throw MappingException::entityListenerMethodNotFound($class, $method, $this->name);
+ }
+
+ $this->entityListeners[$eventName][] = array(
+ 'class' => $class,
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

Extra space between index name and =>.

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php
@@ -547,6 +547,37 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$metadata->addLifecycleCallback((string)$lifecycleCallback['method'], constant('Doctrine\ORM\Events::' . (string)$lifecycleCallback['type']));
}
}
+
+ // Evaluate entity listener
+ if (isset($xmlRoot->{'entity-listeners'})) {
+ foreach ($xmlRoot->{'entity-listeners'}->{'entity-listener'} as $listenerElement) {
+ $listeners = array();
+
+ foreach ($listenerElement as $type => $callbackElement) {
+ list($prefix, $suffix) = explode('-', $type);
+
+ $eventName = $prefix . ucfirst($suffix);
+ $methodName = (string) $callbackElement['method'];
+ $listeners[] = array($eventName, $methodName);
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco and 1 other commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
+
+ foreach ($entityListener as $eventName => $callbackElement){
+ foreach ($callbackElement as $methodName){
+ $listeners[] = array($eventName, $methodName);
+ }
+ }
+
+ if (null !== $className) {
+ foreach ($listeners as $item){
+ $metadata->addEntityListener($item[0], $className, $item[1]);
+ }
+
+ continue;
+ }
+
+ // evaluate as lifecycle callback if the listener class is not given.
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

First letter should always be uppercased. Do you start a paragraph using lowercase? =P

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/MappingException.php
@@ -413,6 +413,22 @@ public static function lifecycleCallbackMethodNotFound($className, $methodName)
return new self("Entity '" . $className . "' has no method '" . $methodName . "' to be registered as lifecycle callback.");
}
+ public static function entityListenerClassNotFound($listenerName, $className)
+ {
+ return new self(sprintf(
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

You can inline this error constructor.

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/Mapping/MappingException.php
@@ -413,6 +413,22 @@ public static function lifecycleCallbackMethodNotFound($className, $methodName)
return new self("Entity '" . $className . "' has no method '" . $methodName . "' to be registered as lifecycle callback.");
}
+ public static function entityListenerClassNotFound($listenerName, $className)
+ {
+ return new self(sprintf(
+ 'Entity Listener "%s" declared on "%s" not found.',
+ $listenerName, $className
+ ));
+ }
+
+ public static function entityListenerMethodNotFound($listenerName, $methodName, $className)
+ {
+ return new self(sprintf(
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

You can inline this error constructor.

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012

lib/Doctrine/ORM/UnitOfWork.php
foreach ($entities as $entity) {
+
@guilhermeblanco

guilhermeblanco Aug 12, 2012

Owner

Remove extra line break

This pull request passes (merged 0c77c1c7 into 72ce9a7).

Contributor

Koc commented Aug 12, 2012

@guilhermeblanco please consider other variants of solving this problem before merging.

Owner

FabioBatSilva commented Aug 12, 2012

Guys, what do you think about the following solution ?

<?php
class EntityListenerManager 
{
    private $listenerInstances;

    public function setListenerInstance($instance) 
    {
        $this->listenerInstances[get_class($instance)] = $instance;
    }

    public function dispatchEntityListeners($metadata, $eventName, $entity, EventArgs $arg)
    {
        foreach ($metadata->entityListeners[$eventName] as $listener) {
            $class  = $listener['class'];
            $method = $listener['method'];

            if ( ! isset($this->listenerInstances[$class])) {
                $this->listenerInstances[$class] = new $class();
            }

            $this->listenerInstances[$class]->{$method}($entity, $arg);
        }
    }
}

class UnitOfWork
{
    public function __construct(EntityManager $em)
    {
        $this->em               = $em;
        $this->evm              = $em->getEventManager();
        $this->entityListener   = $em->getEntityListenerManager();
    }

    private function persistNew($class, $entity)
    {
        if (isset($class->entityListeners[Events::prePersist])) {
            $this->entityListener->dispatchEntityListeners($class, Events::prePersist, $entity, $event);
        }
    }
}
Owner

guilhermeblanco commented Aug 12, 2012

@FabioBatSilva I think it could come from Configuration instead of straight from EM.
Instead of using as a manager, I'd only implement a Resolver that is able to instantiate, following the same idea as our naming strategy.

Owner

FabioBatSilva commented Aug 12, 2012

@guilhermeblanco seems good for me,
i'll work on the approach

Thanks o/

This pull request passes (merged 51a1a13a into 72ce9a7).

This pull request passes (merged 67fd9bb4 into 72ce9a7).

This pull request passes (merged 144aef21 into f8a582d).

This pull request passes (merged 83ad070f into f8a582d).

@stof stof commented on an outdated diff Aug 19, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
@@ -391,7 +395,7 @@ private function addInheritedSqlResultSetMappings(ClassMetadata $subClass, Class
}
}
- /**
+ /**
@stof

stof Aug 19, 2012

Member

this change should be reverted

@stof stof commented on the diff Aug 19, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
@@ -155,6 +155,10 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
$this->addInheritedSqlResultSetMappings($class, $parent);
}
+ if ($parent && !empty($parent->entityListeners) && empty($class->entityListeners)) {
+ $class->entityListeners = $parent->entityListeners;
@stof

stof Aug 19, 2012

Member

shouldn't parent listeners be inherited too (by merging the arrays together) ?

@FabioBatSilva

FabioBatSilva Aug 20, 2012

Owner

How to remove a parent listener following this approach ?

@stof

stof Aug 20, 2012

Member

well, the point is that you have to duplicate all listeners of the mapped superclass if you want to add an additional one currently (making it likely to break things if you don't look at the mapped superclass carefully when mapping your entity). A solution could be to add another parameter in the annotation controlling the inheritance. It should default to inherit them, so that if you set it explicitly to false, you are expected to understand that you need to check the parent.

@FabioBatSilva

FabioBatSilva Aug 21, 2012

Owner

I see your point.

But I think that listener are not the kind of stuff mergeable.
IMHO If listeners are changed the user should handle this behavior.

IRC This is the expected behavior on hibernate.

@stof stof commented on an outdated diff Aug 19, 2012

...octrine/ORM/Mapping/DefaultEntityListenerResolver.php
+ */
+ public function clear($className = null)
+ {
+ if ($className === null) {
+ $this->instances = array();
+
+ return;
+ }
+
+ if (isset($this->instances[$className = trim($className, '\\')])) {
+ unset($this->instances[$className]);
+ }
+ }
+
+ /**
+ * {@inheritdoc}"
@stof

stof Aug 19, 2012

Member

you should remove the extra "

@stof stof and 2 others commented on an outdated diff Aug 19, 2012

doctrine-mapping.xsd
+
+ <xs:complexType name="pre-update">
+ <xs:attribute name="method" type="xs:string"/>
+ </xs:complexType>
+
+ <xs:complexType name="post-update">
+ <xs:attribute name="method" type="xs:string"/>
+ </xs:complexType>
+
+ <xs:complexType name="pre-remove">
+ <xs:attribute name="method" type="xs:string"/>
+ </xs:complexType>
+
+ <xs:complexType name="post-remove">
+ <xs:attribute name="method" type="xs:string"/>
+ </xs:complexType>
@stof

stof Aug 19, 2012

Member

do you really need so many types ? They are all exactly the same: lifecycle callbacks

@FabioBatSilva

FabioBatSilva Aug 20, 2012

Owner

It seen more explicit for me, also similar with hibernate implementation..

@stof

stof Aug 20, 2012

Member

but not similar with the other parts of this XSD. See for instance the cascading: https://github.com/doctrine/doctrine2/blob/master/doctrine-mapping.xsd#L25

@guilhermeblanco

guilhermeblanco Aug 20, 2012

Owner

@FabioBatSilva I think I'm with @stof on this one...
I totally agree we should follow JPA/Hibernate closely, but there're some optimizations and minimal changes required sometimes.

@FabioBatSilva

FabioBatSilva Aug 21, 2012

Owner

OK guys.. i'll change it

This pull request passes (merged fdae6959 into ece6a00).

This pull request passes (merged bb50bde5 into ece6a00).

@stof stof and 1 other commented on an outdated diff Aug 22, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
{
foreach ($this->lifecycleCallbacks[$lifecycleEvent] as $callback) {
- $entity->$callback();
+ $entity->$callback($event);
@stof

stof Aug 22, 2012

Member

this is wrong. lifecycle callbacks are not meant to receive the event args. The entities should not get access to the unit of work.

@stof

stof Aug 22, 2012

Member

thus, it has nothing to do with the feature you are adding in this PR.

@beberlei

beberlei Oct 5, 2012

Owner

It is actually part of the PR, we want to enable this for people that want to go more torwards Active Record.

@stof stof commented on an outdated diff Aug 22, 2012

lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
- if (isset($annotations['Doctrine\ORM\Mapping\PostRemove'])) {
- $metadata->addLifecycleCallback($method->getName(), \Doctrine\ORM\Events::postRemove);
- }
+ // Evaluate as lifecycle callback if the listener class is not given.
@stof

stof Aug 22, 2012

Member

What is this part ?

@stof stof commented on an outdated diff Aug 22, 2012

lib/Doctrine/ORM/Mapping/EntityListeners.php
+ * @since 2.4
+ *
+ * @Annotation
+ * @Target("CLASS")
+ */
+final class EntityListeners implements Annotation
+{
+ /**
+ * Specifies the names of the entity listeners.
+ *
+ * @var array<string>
+ */
+ public $value = array();
+
+ /**
+ * Specifies the entity the entity lifecycle callbacks.
@stof

stof Aug 22, 2012

Member

typo here

Member

stof commented Aug 22, 2012

I don't like the syntax allowing to use the @EntityListeners annotation to configure lifecycle callbacks of the entity itself:

  • this mixes the responsibilities of the annotation with 2 different purposes (see the code you wrote in each mapping driver: you handle 2 cases in each of them)
  • there is already a way to configure lifecycle callbacks, so why adding a second one ?
Owner

FabioBatSilva commented Aug 22, 2012

Hi @stof

This is something that I discussed with Benjamin when i talked to him about the event listeners.

The Idea is allow the reuse of the current lifecycle callbacks
The old way might be replaced in doctrine 3.0.
But i'm not sure if I understand exactly his idea about this.

Maybe Benjamin could help us..

What do you think about this @beberlei ?

Owner

FabioBatSilva commented Oct 29, 2012

@beberlei please take a look,
A new class has been created "ListenersInvoker" in order to extract the listeners call from the metadata class as we discussed on IRC.

cc/ @stof @guilhermeblanco

@stof stof commented on an outdated diff Oct 29, 2012

lib/Doctrine/ORM/Event/ListenersInvoker.php
+ }
+
+ /**
+ * Dispatches the lifecycle event of the given entity to the registered entity listeners.
+ *
+ * @param \Doctrine\ORM\Mapping\ClassMetadata $name The Entity metadata class.
+ * @param string $eventName The entity lifecycle event.
+ *
+ * @return integer Bitmask for subscribed event systems.
+ */
+ public function getSubscribedSystems(ClassMetadata $metadata, $eventName)
+ {
+ $invoke = ListenersInvoker::INVOKE_NONE;
+
+ if (isset($metadata->lifecycleCallbacks[$eventName])) {
+ $invoke += ListenersInvoker::INVOKE_CALLBACKS;
@stof

stof Oct 29, 2012

Member

I would use |= which is more logical to build a binary flag

@stof stof commented on an outdated diff Oct 29, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -2357,6 +2354,31 @@ public function setLifecycleCallbacks(array $callbacks)
}
/**
+ * Adds a entity listener for entities of this class.
+ *
+ * @param string $callback
@stof

stof Oct 29, 2012

Member

wrong parameters

@stof stof commented on the diff Oct 29, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -2812,4 +2831,17 @@ public function getAssociationsByTargetClass($targetClass)
}
return $relations;
}
-}
+
+ /**
+ * @param string $className
+ * @return string
+ */
+ public function fullyQualifiedClassName($className)
@stof

stof Oct 29, 2012

Member

should it really be a public method ? I would have made it private

@stof stof commented on an outdated diff Oct 29, 2012

lib/Doctrine/ORM/UnitOfWork.php
@@ -496,7 +511,7 @@ public function getEntityChangeSet($entity)
public function computeChangeSet(ClassMetadata $class, $entity)
{
$oid = spl_object_hash($entity);
-
+
@stof

stof Oct 29, 2012

Member

Please don't add trailing whitespaces

@stof stof commented on an outdated diff Oct 29, 2012

...Doctrine/Tests/ORM/Functional/EntityListenersTest.php
@@ -0,0 +1,249 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Tests\Models\Company\CompanyFixContract;
+
+require_once __DIR__ . '/../../TestInit.php';
@stof

stof Oct 29, 2012

Member

This require_once is useless

@stof stof commented on an outdated diff Oct 29, 2012

...rine/Tests/ORM/Mapping/EntityListenerResolverTest.php
@@ -0,0 +1,99 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Mapping;
+
+use Doctrine\ORM\Mapping\DefaultEntityListenerResolver;
+
+require_once __DIR__ . '/../../TestInit.php';
@stof

stof Oct 29, 2012

Member

this line is useless

Owner

beberlei commented Nov 25, 2012

I am not sure about having the annotations on EntityListeners as well and would rather prefer the naming automation that the current listeners have with event name => method name.

@FabioBatSilva what do you think?

Owner

FabioBatSilva commented Nov 25, 2012

@beberlei, Not sure about naming automation,
I self like to name my listeners with different names like onPrePersist or prePersistHandler...
IMO Reusing the current annotatios make easier to understand the new event system and annotations on listeners classes allow to use the same listener in deferent entities just by declaring the listener.

Owner

beberlei commented Nov 25, 2012

Yes, but this requires configuration for all methods everywhere. In the case of XML and YML mapping you have to map over and over again, because you cannot re-use the mapping. Say you use one listener with three methods in N entities, then this is lots of duplication of mapping the methods.

And you could still use instanceof checks in methods if you want. I see the new @EventListener more in context of the old EventLiteners/Subscribers. So keep the method<->event convention and allow them to be used on a subset of entities only, instead globally.

Owner

FabioBatSilva commented Nov 26, 2012

@beberlei I see your point..

Maybe we can allow the listener declaration without mapping for the callbacks..
In this case the reader should look up for methods that match with the event life cycle conventions.

Owner

beberlei commented Dec 4, 2012

My thinking was that the events are named like the constants in Doctrine\ORM\Events and the methods are the same name. So postLoad becomes MyListener#postLoad()

Owner

FabioBatSilva commented Dec 4, 2012

Nothing against support the naming convention.
I agree, it makes easier to reuse the listener in different entities.
But i'd like to support both, naming convention and configuration.
IMO the configuration is more explicit.

What do you think guys : @beberlei, @Ocramius, @guilhermeblanco, @stof, @asm89 ?

gena01 commented Jan 24, 2013

I was actually looking to do something similar today. I had a much simpler idea. Currently Doctrine\ORM\Mapper\ClassMetadataInfo has the following code:

/**
* Adds a lifecycle callback for entities of this class.
*
* @param string $callback
* @param string $event
*
* @return void
*/
    public function addLifecycleCallback($callback, $event)
    {
        $this->lifecycleCallbacks[$event][] = $callback;
    }

and:

 /**
* Dispatches the lifecycle event of the given entity to the registered
* lifecycle callbacks and lifecycle listeners.
*
* @param string $lifecycleEvent The lifecycle event.
* @param object $entity The Entity on which the event occured.
*
* @return void
*/
    public function invokeLifecycleCallbacks($lifecycleEvent, $entity)
    {
        foreach ($this->lifecycleCallbacks[$lifecycleEvent] as $callback) {
            $entity->$callback();
        }
    }

If $callback was a "Callable" then anybody could subscribe and listen for events using closures, anonymous functions, external classes, etc.....

Owner

beberlei commented Jan 24, 2013

@FabioBatSilva Ok lets do it your way then. Can you update the documentation as well and rebase? Then its good to merge.

Owner

beberlei commented Jan 24, 2013

@gena01 this is not possible as you mentioned it, because ClassMetadataInfo comes from a cache and lifecycleCallbacks that are callable cannot be serialized/unserialized easily.

FabioBatSilva added some commits Jul 29, 2012

@FabioBatSilva FabioBatSilva entity listeners mapping 368cf73
@FabioBatSilva FabioBatSilva support short class name 0f081d7
@FabioBatSilva FabioBatSilva move call listeners tests to AbstractMappingDriverTest 3c223a5
@FabioBatSilva FabioBatSilva test entity listener metadata c5d59ab
@FabioBatSilva FabioBatSilva test entity listener calls ccc0a2a
@FabioBatSilva FabioBatSilva call listeners in UoW 315f7ba
@FabioBatSilva FabioBatSilva test @PostLoad dbd0697
@FabioBatSilva FabioBatSilva give event to lifecycle callbacks c6adcda
@FabioBatSilva FabioBatSilva test lifecycle callbacks event args 4cfe229
@FabioBatSilva FabioBatSilva fix previous test 6be7a03
@FabioBatSilva FabioBatSilva xml driver 7e54ae3
@FabioBatSilva FabioBatSilva test invalid class/method 917aa70
@FabioBatSilva FabioBatSilva yaml driver f0b0437
@FabioBatSilva FabioBatSilva php static driver 415c2a9
@FabioBatSilva FabioBatSilva php driver 7021f00
@FabioBatSilva FabioBatSilva support @LifecycleCallback in @EntityListeners fd6f592
@FabioBatSilva FabioBatSilva evaluate as lifecycle callback if the listener class is not given. 256cecb
@FabioBatSilva FabioBatSilva test event listeners lifecycle callback 69bfc71
@FabioBatSilva FabioBatSilva added doctrine-mapping.xsd 46474bf
@FabioBatSilva FabioBatSilva rename subscribers to listeners a265511
@FabioBatSilva FabioBatSilva Fix some CS 27745bb
@FabioBatSilva FabioBatSilva implements a entity listener resolver a01d658
@FabioBatSilva FabioBatSilva rename test 8495eca
@FabioBatSilva FabioBatSilva small refactoring 4be25cb
@FabioBatSilva FabioBatSilva fix CS 6b7e588
@FabioBatSilva FabioBatSilva change xml driver to use <lifecycle-callback\> 195b639
@FabioBatSilva FabioBatSilva Fix typo ffc8d03
@FabioBatSilva FabioBatSilva remove @LifecycleCallback c60e3e4
@FabioBatSilva FabioBatSilva move listeners invocation from ClassMetadataInfo to ListenerInvoker 0d0f91a
@FabioBatSilva FabioBatSilva Fix DocBlock 0d0fc32
@FabioBatSilva FabioBatSilva split override test 7b0f59e
@FabioBatSilva FabioBatSilva Use bitmask of subscribed event systems. 6d7b386
@FabioBatSilva FabioBatSilva Fix typo 3f9a4c8
@FabioBatSilva FabioBatSilva Fix DocBlock e9c89ca
@FabioBatSilva FabioBatSilva use '!==' instead of '!=' 46fea51
@FabioBatSilva FabioBatSilva support naming convention for listeners without mapping. e6d9d1d
@FabioBatSilva FabioBatSilva added missing file ec2d5af
@FabioBatSilva FabioBatSilva remove extra comma 7177306
@FabioBatSilva FabioBatSilva Update docs/en/reference/events.rst
Docs for lifecycle-callback event arg
76c4be1
@FabioBatSilva FabioBatSilva Docs for Entity Listener and Entity Listener Resolver 7764ed9
Owner

FabioBatSilva commented Feb 2, 2013

Hi @beberlei,

Rebased, docs updated. :D

Cheers

@beberlei beberlei and 1 other commented on an outdated diff Feb 2, 2013

docs/en/reference/events.rst
+
+ <lifecycle-callback type="postUpdate" method="postUpdateHandler"/>
+ <lifecycle-callback type="preUpdate" method="preUpdateHandler"/>
+
+ <lifecycle-callback type="postRemove" method="postRemoveHandler"/>
+ <lifecycle-callback type="preRemove" method="preRemoveHandler"/>
+ </entity-listener>
+ </entity-listeners>
+ <!-- .... -->
+ </entity>
+ </doctrine-mapping>
+ .. code-block:: yaml
+
+ MyProject\Entity\User:
+ type: entity
+ UserListener:
@beberlei

beberlei Feb 2, 2013

Owner

this looks wrong

@beberlei beberlei added a commit that referenced this pull request Feb 2, 2013

@beberlei beberlei Merge pull request #423 from FabioBatSilva/DDC-1955
DDC-1955 - @EntityListeners
71a68a5

@beberlei beberlei merged commit 71a68a5 into doctrine:master Feb 2, 2013

@FabioBatSilva FabioBatSilva deleted the FabioBatSilva:DDC-1955 branch Feb 5, 2013

I chased a bug in another project to this commit.

The problem is that if you set the c in Category to a lowercase c,

     * @OneToMany(targetEntity="Category", mappedBy="parent")
     * @OneToMany(targetEntity="category", mappedBy="parent")

you get an invalid class name.

The documentation shows it with a capital, but for most things it does work with a lowercase. Also, it seems weird that some fields can be lower case like mappedBy, or inversedBy in this example from the documentation:

    * @ManyToOne(targetEntity="Product", inversedBy="features")

So,

a) why is the mechanism different among the different fields?
b) should we do some checking and throw an error if the class doesn't actually exist?
c) or should we correct the class name using some reflection logic?

Owner

Ocramius replied May 22, 2017

This is because PHP is case-insensitive: if your filesystem is case sensitive (usually true for *nix systems), then you may encounter an autoloading crash when having a casing mismatch in your mappings.

Please always use the correct casing.

I thought that's what you might say but I don't see in the documentation where it's clear that field correlates to the actual class and not just the entity short hand and that might be valuable information. Also, I just wonder about the strength of this routine if it's not going to throw an error.

Owner

Ocramius replied May 22, 2017 edited

Tbh, I never abbreviate, I just use Product::class instead of "Product" or the product FQCN, these days.

Probably something to add to the bucket of "things to get rid of in 3.x"

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