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

Refactoring/listener #21

Merged
merged 8 commits into from
Feb 15, 2013
Merged

Refactoring/listener #21

merged 8 commits into from
Feb 15, 2013

Conversation

PedroTroller
Copy link
Member

Add an abstract listener and refactor all listeners

* @param string $traitName
* @param boolean $isRecursive
*/
protected function isEntityUseTrait(\ReflectionClass $class, $traitName, $isRecursive = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

isEntityUsingTrait makes more sense, doesn't it?

Anyway, isn't the goal of this PR to abstract wether this listener applies on Document or Entity?
In this case, this method could be called isObjectUsingTrait

Last thing, as I believe these 3 methods will be usefull in many other places, you might want to move it to its own class and inject it in the listener (something like Knp\DoctrineBehaviors\Reflection\ClassAnalyzer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmm ok....
About the method name, the goal of DoctrineBehaviors is to add features to Doctrine Entities. Is that a real problem if methods names talk about entities.

The class Analyser is a good idea.

@doup
Copy link
Contributor

doup commented Feb 14, 2013

Which is the status of this PR? I'm interested on having this on master as it seems to support Trait hierarchies, is it correct?

Thank you!

@docteurklein
Copy link
Contributor

it's ok to merge. we should just use it with "odm" branch.

@doup
Copy link
Contributor

doup commented Feb 14, 2013

Just into the odm branch? It's that the main dev branch or something?
Thank you!

PedroTroller added a commit that referenced this pull request Feb 15, 2013
@PedroTroller PedroTroller merged commit a2bd961 into master Feb 15, 2013
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.

4 participants