-
Notifications
You must be signed in to change notification settings - Fork 326
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 page resource segment to PageRemovedEvent #6906
Conversation
09892d7
to
6acf3de
Compare
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.
@vvasiloi Thank you for working on this. Just some small comments.
src/Sulu/Bundle/PageBundle/Tests/Unit/EventListener/DomainEventSubscriberTest.php
Outdated
Show resolved
Hide resolved
src/Sulu/Bundle/PageBundle/Tests/Unit/EventListener/DomainEventSubscriberTest.php
Outdated
Show resolved
Hide resolved
src/Sulu/Bundle/PageBundle/Tests/Unit/EventListener/DomainEventSubscriberTest.php
Outdated
Show resolved
Hide resolved
…ared in parent classes
ea4f5f0
to
7280f7e
Compare
7280f7e
to
e34c7f0
Compare
@alexander-schranz I think it's ready. I looked to add some docs, but I didn't find any for spefic events, so I don't know where to start. 😅 |
@vvasiloi thx for the pull request. great work 👍 Yeah the events are really leaking. We shoul ddefinitly add a Events section to all the bundles to show and describe them. |
Hi @vvasiloi, just wanted to let you know how impressed I am about your methodical approach on this and the quality of the entire pull request, even if you have never worked with Sulu before. Thanks for your contribution! 🙂 |
To Do
Use mocking in test (phpspec/prophecy)Not necessaryCreate a documentation PRThere's no documentation for particular events