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

[EventDispatcher] Throw exception is subscriber is miss configured #10065

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@lyrixx
Copy link
Member

commented Jan 18, 2014

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

Lot of new comers (during trainings) made the same typo:

class TestEventSubscriberTypo implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return array('pre.foo', 'preFoo');
    }
}

instead of

class TestEventSubscriberTypo implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return array('pre.foo' => 'preFoo');
    }
}

So I added a check.

This can be a BC break as the event name can be an int. But I think (I hope?)
nobody do that.

@sstok

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2014

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2014

@sstok Thanks. Fixed ;)

{
public static function getSubscribedEvents()
{
return array('pre.foo', 'preFoo');

This comment has been minimized.

Copy link
@staabm

staabm Jan 19, 2014

Contributor

I would add a comment here so one immediately sees where the typo is in here

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jan 19, 2014

Author Member

Fixed, Thanks.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 20, 2014

Not sure about this one. Is it really that frequent a mistake?

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2014

I am not sure too.

But this is a very frequent mistake. At least one person per training session. More over, it's not so easy to detect. Now, I know the typo and I can help faster. But for a new comer, it's almost impossible to spot the issue.

@Tobion

This comment has been minimized.

Copy link
Member

commented Jan 20, 2014

👎 We could add hundrets of those checks. Why just here? Also it's a bc break.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2014

@Tobion I agree with you. I already answered ... just above and in the PR header.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 20, 2014

Closing it as it seems that such safeguards could be added in so many places.

@fabpot fabpot closed this Jan 20, 2014

@lyrixx lyrixx deleted the lyrixx:ed-debug branch Jan 20, 2014

@stof

This comment has been minimized.

Copy link
Member

commented Jan 20, 2014

@fabpot this might become a check on SensioLabs Insight though. Such mistake can be spotted by a static analysis

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2014

very good point ;)

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.