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

Mark all dispatched event classes as final #33152

Merged
merged 1 commit into from Aug 21, 2019

Conversation

@Tobion
Copy link
Member

commented Aug 13, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

I think we should mark all our Event classes as final. There is no point in people extending them as the libraries that use the event, will only dispatch this event. So extending events in user-land achieves nothing as the subclasses won't be dispatched.
I'm not talking about the base events that are meant to be extended like KernelEvent, but the leaf events like ExceptionEvent, ResponseEvent etc.
Then we can also make them real final in 5.0 as the events are value objects that should not be mocked.

@@ -20,7 +20,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class MessageEvent extends Event
final class MessageEvent extends Event

This comment has been minimized.

Copy link
@Tobion

Tobion Aug 13, 2019

Author Member

@fabpot there is a MessageEvents class in the same mailer namespace. This can easily be confused with the event class and should maybe be renamed or relocated or even made internal.

This comment has been minimized.

Copy link
@stof

stof Aug 21, 2019

Member

isn't MessageEvents the class holding all event names as constants ? This is not internal

This comment has been minimized.

Copy link
@Tobion

Tobion Aug 21, 2019

Author Member

The Mailer MessageEvents is a class used for logging and data collecting. It collects events but is not an event itself.

@Tobion Tobion force-pushed the Tobion:final-events branch from 70640d0 to 0d135ee Aug 13, 2019

@chalasr chalasr added this to the next milestone Aug 13, 2019

@fabpot

fabpot approved these changes Aug 14, 2019

@lyrixx

lyrixx approved these changes Aug 14, 2019

@Tobion Tobion referenced this pull request Aug 19, 2019
39 of 59 tasks complete

@Tobion Tobion force-pushed the Tobion:final-events branch from 0d135ee to 7052758 Aug 19, 2019

nicolas-grekas added a commit that referenced this pull request Aug 21, 2019

bug #33282 [HttpKernel] Do not extend the new SF 4.3 ControllerEvent …
…so we can make it final (Tobion)

This PR was merged into the 4.3 branch.

Discussion
----------

[HttpKernel] Do not extend the new SF 4.3 ControllerEvent so we can make it final

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | unlikely
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |
| License       | MIT
| Doc PR        |

See #33152 (comment)
Remember the ControllerEvent is new in SF 4.3 so we just go back to what it was before 4.3

Commits
-------

00140b6 Do not extend the new SF 4.3 ControllerEvent so we can make it final

@Tobion Tobion force-pushed the Tobion:final-events branch from 7052758 to 4bb38ee Aug 21, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Thank you @Tobion.

nicolas-grekas added a commit that referenced this pull request Aug 21, 2019

feature #33152 Mark all dispatched event classes as final (Tobion)
This PR was merged into the 4.4 branch.

Discussion
----------

Mark all dispatched event classes as final

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |
| License       | MIT
| Doc PR        |

I think we should mark all our Event classes as final. There is no point in people extending them as the libraries that use the event, will only dispatch this event. So extending events in user-land achieves nothing as the subclasses won't be dispatched.
I'm not talking about the base events that are meant to be extended like KernelEvent, but the leaf events like ExceptionEvent, ResponseEvent etc.
Then we can also make them real final in 5.0 as the events are value objects that should not be mocked.

Commits
-------

4bb38ee Mark all dispatched event classes as final

@nicolas-grekas nicolas-grekas merged commit 4bb38ee into symfony:4.4 Aug 21, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

@Tobion Tobion deleted the Tobion:final-events branch Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.