Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for tagging entity listeners, without the need to map the listener #298

Merged
merged 8 commits into from
Apr 19, 2015

Conversation

kimhemsoe
Copy link
Member

Adds support for tagging entity listeners, defining entity and method.

Every listener service is created when the entitymanager is created for now. A solution could be marking the services lazy as suggested by @Ocramius and @stof in #223

@kimhemsoe kimhemsoe changed the title Added support for tagging entity listeners, without the need to map the listner Added support for tagging entity listeners, without the need to map the listener Jun 13, 2014
@pkruithof
Copy link
Contributor

Very nice 👍

@jmontoyaa
Copy link

+1

@kimhemsoe
Copy link
Member Author

@stof Is this something you can accept into the bundle and what do you think about it ?

@Nemo64
Copy link

Nemo64 commented Jul 1, 2014

OK, I'd really like to have that right now (if it works like described)

@Phobetor
Copy link

+1

@chasen
Copy link

chasen commented Sep 22, 2014

In keeping with conventions of other event listeners and subscribers the config property type should probably be renamed to event

- { name: doctrine.orm.entity_listener, entity_manager: em2, entity: My/Entity2, event: preFlush, method: preFlushHandler}

But otherwise I would really love to see this feature added ASAP

@austinh
Copy link

austinh commented Feb 17, 2015

Would love to be able to use this feature!

@hacfi
Copy link
Contributor

hacfi commented Apr 19, 2015

👍 Trying to implement this feature myself I came across this PR. Would love to see this getting merged.

@kimhemsoe
Copy link
Member Author

@hacfi I never really tested this in a real app. It should work but im not 100% sure. Have you or anyone else tried it ?

@hacfi
Copy link
Contributor

hacfi commented Apr 19, 2015

@kimhemsoe Debugging this at the moment..looks like the entity listener isn’t added properly to the class metadata.

@kimhemsoe
Copy link
Member Author

@hacfi I have done a rebase to make it bit easier.


$args = array(
$serviceDef->getClass(),
$attributes['entity'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

$listenerId = sprintf('doctrine.orm.%s_listeners.attach_entity_listeners', $entityManager['name']);
$listenerDef = $container->setDefinition($listenerId, new Definition('%doctrine.orm.listeners.attach_entity_listeners.class%'));
$listenerDef->addTag('doctrine.event_listener', array('event' => 'loadClassMetadata'));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t understand this part..using ORM 2.5.0 the listener is missing in the attachToListener method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update @kimhemsoe !

I think this should be

if (version_compare(Version::VERSION, "2.5.0-DEV") > 0) {

instead. doctrine.orm.listeners.attach_entity_listeners.class isn’t available before 2.5 !?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, i was a bit to fast in the rebase.

@hacfi
Copy link
Contributor

hacfi commented Apr 19, 2015

@kimhemsoe Perfect...works as expected now.

As @chasen suggested the name event is more consistent that type. What do you think?

@kimhemsoe
Copy link
Member Author

The name "type" is from doctrine ORM, but "event" would be fine with me.

@hacfi
Copy link
Contributor

hacfi commented Apr 19, 2015

Great! Thanks so much for this @kimhemsoe...I almost started writing a custom annotation for this 😜

@kimhemsoe
Copy link
Member Author

You welcome @hacfi and thanks for the help. I will merge when travis and Scrutinizer returns green.

kimhemsoe added a commit that referenced this pull request Apr 19, 2015
Added support for tagging entity listeners, without the need to map the listener
@kimhemsoe kimhemsoe merged commit 4652c10 into doctrine:master Apr 19, 2015
@kimhemsoe kimhemsoe deleted the attach_listener branch April 19, 2015 17:11
@hacfi
Copy link
Contributor

hacfi commented Apr 19, 2015

@kimhemsoe Updating my current project to get the latest master..so useful I don’t want to wait :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants