-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[CoreBundle] Add custom constraint for "enabled" entity fields and prevent delete all channels #3150
Conversation
/* | ||
* @author Gustavo Perdomo <gperdomor@gmail.com> | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank line.
ping @pjedrzejewski |
pong @pjedrzejewski xD |
@gperdomor Ping pong. :) I will have a look at this PR as soon as humanly possible. :) I need to give it a bit more thought and we are at PHP Summer Camp right now, things are busy. :) Thanks for your patience! |
*/ | ||
class ChannelListener | ||
{ | ||
private $repository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docblock.
return $entityManager; | ||
} | ||
|
||
private function ensureEntityHasProvidedEnabledField($entity, Constraint $constraint, ObjectManager $entityManager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docblock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missin docblock, anyway it would be nice to replace Constraint $constraint
with $enabledPropertyPath
. If so, I would recommend given order ObjectManager $objectManager, $object, $enabledPropertyPath
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There are some specs and Behat scenarios missing. |
Behat scenarios are failing when run the What should i do in this case? |
@gperdomor I fixed it in #3177, it will be merged today (I think rather in the morning than in the evening :)). |
Refactor methods based on feedback. Fix of state label on channel macro template. Fix missing doc blocks.
Fix missing docs blocks. Change of some variables as suggested in feedback.
@gperdomor already merged, you can rebase :) |
[CoreBundle] Add custom constraint for "enabled" entity fields and prevent delete all channels
Thanks a lot @gperdomor and guys for the review! 👍 Good job. |
This PR introduce a custom constraint to verify entities with enabled field, when at least one entity is required to be enabled.
Also this PR prevent the deletion of a channel, if the the channel is the unique enabled channel available.