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

add new Activatable trait #214

Closed
wants to merge 1 commit into from
Closed

add new Activatable trait #214

wants to merge 1 commit into from

Conversation

flug
Copy link
Contributor

@flug flug commented May 20, 2015

No description provided.


namespace Knp\DoctrineBehaviors\Model\Activable;


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra empty line.

@akovalyov
Copy link
Contributor

Well, I am -1 for this PR.

  1. I do not like the name Activable.
  2. I do not like the implementation.
  3. I do not like obvious copypastes
  4. I am not even sure if it works because tests are broken.

@@ -154,3 +156,13 @@ services:
tags:
- { name: doctrine.event_subscriber }


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra empty line.

@akovalyov akovalyov force-pushed the master branch 4 times, most recently from 6cb1e53 to 0a444f6 Compare May 27, 2015 13:24
@flug flug force-pushed the activable branch 4 times, most recently from 24c0ba9 to a1cd487 Compare May 29, 2015 12:20
@flug flug changed the title add new Activable trait add new Activatable trait Jun 1, 2015
@docteurklein
Copy link
Contributor

not sure we should add too much new features, sorry.

@Einenlum
Copy link

Seems there is no consensus to add this new feature.

@Einenlum Einenlum closed this Sep 14, 2017
@flug flug deleted the activable branch September 14, 2017 12:55
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

6 participants