Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[EventDispatcher] add method getListenerPriority() to interface#16301

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@xabbuh
Copy link
Member

QA
Bug fix?no
New feature?yes
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets#16198
LicenseMIT
Doc PRtodo

@dupuchba
Copy link
Contributor

👍

@Tobion
Copy link
Contributor

👍

Status: Reviewed

@Tobion
Copy link
Contributor

Thank you@xabbuh.

@TobionTobion merged commitf8019c8 intosymfony:masterOct 23, 2015
Tobion added a commit that referenced this pull requestOct 23, 2015
…interface (xabbuh)This PR was merged into the 3.0-dev branch.Discussion----------[EventDispatcher] add method getListenerPriority() to interface| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | yes| BC breaks?    | yes| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#16198| License       | MIT| Doc PR        | todoCommits-------f8019c8 add method getListenerPriority() to interface
@xabbuhxabbuh deleted the event-dispatcher-interface-priority branchOctober 23, 2015 13:32
@fabpotfabpot mentioned this pull requestNov 16, 2015
@pfrenssen
Copy link

Is there a possibility to backport this to Symfony 2.8? ThegetListenerPriority() method is called inContainerAwareEventDispatcher::getListenerPriority() but since it is not defined on the interface the developers are not made aware that this method is now required.

@xabbuh
Copy link
MemberAuthor

@pfrenssen Adding a method to an interface is a BC break (all implementations of the interface must be changed). That's why we had to make this change in 3.0.

@pfrenssen
Copy link

Ok that sounds like a good reason. Pity is that BC is already broken, ContainerAwareEventDispatcher is calling this method in 2.8.

@xabbuh
Copy link
MemberAuthor

@pfrenssen I do not see where this is happening. But can you please create a new issue if you think that there actually is a bug (always calling a method that is not given by the interface would be indeed a mistake). Best would be if you could come up with a failing test case or code example.

@donquixote
Copy link
Contributor

@xabbuh@pfrenssen here,https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php#L102

backport this to Symfony 2.8

This could be done by adding a separate interface, extending the basic one.
Then either change the constructor signature for TraceableEventDispatcher, or do some instanceof.
Or maybe just method_exists(), without any extra interface?

The question is: Should TraceableEventDispatcher always require a dispatcher with getListenerPriority() ? Or is this called only sometimes? Or could it return a default value, if the method does not exist?

create a new issue

I leave this to others (e.g.@pfrenssen) For now I am simply having a short visit in this issue queue.

@xabbuh
Copy link
MemberAuthor

@pfrenssen@donquixote Thank you for the details given. I checked the code again and proposed a fix in#18388 to provide a meaningful exception in case you call thegetListenerPriority() method in case the method does not exist in the wrapped dispatcher.

@pfrenssen
Copy link

I guess this is a good solution if we cannot add the interface for BC reasons. Thanks!

fabpot added a commit that referenced this pull requestMay 3, 2016
This PR was merged into the 2.8 branch.Discussion----------[EventDispatcher] check for method to exist| Q             | A| ------------- | ---| Branch?       | 2.8| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#16301 (comment)| License       | MIT| Doc PR        |This change must be reverted after being merged into the `3.0` branch (the `getListenerPriority()` method was added to the interface in Symfony 3.0).Commits-------78ae2ad [EventDispatcher] check for method to exist
symfony-splitter pushed a commit to symfony/event-dispatcher that referenced this pull requestMay 3, 2016
This PR was merged into the 2.8 branch.Discussion----------[EventDispatcher] check for method to exist| Q             | A| ------------- | ---| Branch?       | 2.8| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony/symfony#16301 (comment)| License       | MIT| Doc PR        |This change must be reverted after being merged into the `3.0` branch (the `getListenerPriority()` method was added to the interface in Symfony 3.0).Commits-------78ae2ad [EventDispatcher] check for method to exist
athlan added a commit to athlan/pimple-aware-event-dispatcher that referenced this pull requestFeb 24, 2018
Implement `getListenerPriority()` that was introduced in Symfony 2.8.symfony/symfony#16301
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@xabbuh@dupuchba@Tobion@pfrenssen@donquixote@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp