Skip to content

[EventDispatcher] Add missing checks to RegisterListenersPass #13293

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[EventDispatcher] Add missing checks to RegisterListenersPass #13293

wants to merge 1 commit into from

Conversation

znerol
Copy link
Contributor

@znerol znerol commented Jan 6, 2015

  • Support services using a parameter for their class name.
  • Prevent abstract services as event subscribers.
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@znerol
Copy link
Contributor Author

znerol commented Jan 6, 2015

Fixes some issues found by @stof in existing code while reviewing #12521

@stof
Copy link
Member

stof commented Jan 7, 2015

This should actually be fixed in older branches than 2.7

@stof
Copy link
Member

stof commented Jan 7, 2015

Can you also add a test using a parameter for the service class ?

@znerol
[EventDispatcher] Add missing checks to RegisterListenersPass
195893a 
* Support services using a parameter for their class name.
* Prevent abstract services as event subscribers.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |
@znerol znerol force-pushed the register-listeners-pass branch from 3ea8580 to 195893a Compare January 7, 2015 21:25
@znerol
Copy link
Contributor Author

znerol commented Jan 7, 2015

Can you also add a test using a parameter for the service class ?

Done. Did not bother adding a new commit, just amended the existing one. Please correct me if this is not something commonly done in Symfony with small patches.

@stof you mentioned in the other issue that ...

in older Symfony versions, the RegisterListenersPass was registered late in the container compilation, ensuring it would always run after the class resolution in definitions

Do you remember how far this can be safely backported? Cherry-picking works down to the 2.5 branch. The patch also applies down to the 2.3 branch when catering for #6643 by simply finding/replacing most occurrences of EventDispatcher with HttpKernel.

@fabpot
Copy link
Member

fabpot commented Jan 16, 2015

👍 I will take care of the changes to be able to merge it in 2.3.

@fabpot
Copy link
Member

fabpot commented Jan 16, 2015

Thanks @znerol

@fabpot fabpot closed this in 8a870c2 Jan 16, 2015
@znerol znerol deleted the register-listeners-pass branch January 16, 2015 15:25
znerol added a commit to znerol-forks/symfony that referenced this pull request Jan 18, 2015
@znerol
Replicate symfony#13293
9a9b31c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
EventDispatcher
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@znerol @stof @fabpot