NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[EventDispatcher] Correct the called event listener method case#41905

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:4.4fromjjsty1e:patch1

Jul 4, 2021

Merged

[EventDispatcher] Correct the called event listener method case#41905

fabpot merged 1 commit intosymfony:4.4fromjjsty1e:patch1

Jul 4, 2021

Conversation

Copy link

Contributor

jjsty1e commentedJun 29, 2021

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	none
Tickets	none
License	MIT
Doc PR	none

when define an event listener, if you don't specify a method, then the word case of the actual method maybe wrong, for example :

services:my_kernel_envent_listeners:class:App\EventListener\KernelListenertags:            -{ name: kernel.event_listener, event: kernel.controller_arguments }

nomethod key at this tag, then actual method called isonKernelControllerarguments, actually it should beonKernelControllerArguments, the case of word 'arguments' should be upper.

ps: only event name that has dash(_) will be affected.

carsonbot added Status: Needs Review Bug EventDispatcher labels

Jun 29, 2021

Copy link

ContributorAuthor

jjsty1e commentedJun 29, 2021

sorry, it seems this PR should be filed to branch 4.4

Copy link

ContributorAuthor

jjsty1e commentedJun 30, 2021

failed tests😭, so hard to make a pull request, but problem indeed exists

derrabus added this to the4.4 milestone

Jun 30, 2021

Copy link

Member

derrabus commentedJun 30, 2021

The current behavior might be somewhat unexpected. But if we just change it, we will break existing applications. Can we make it so that both ways work?

Copy link

Member

fabpot commentedJun 30, 2021

Method names are case insentitive in PHP, right? So, is there a bug that needs to fix here or is it just for a better looking method name?

Copy link

Member

derrabus commentedJun 30, 2021

Method names are case insentitive in PHP, right?

If we don't configure a method name,RegisterListenersPass will guess it. And that logic is case-sensitive.

So, is there a bug that needs to fix here or is it just for a better looking method name?

It's about allowing a less surprising method name, I'd say.

derrabus added the DXDX = Developer eXperience (anything that improves the experience of using Symfony) label

Jun 30, 2021

Copy link

Contributor

ro0NL commentedJun 30, 2021

Ref#41876

jjsty1e mentioned this pull request

Jul 1, 2021

correct the called event listener method case#41876

Closed

ro0NL reviewed

Jul 1, 2021

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

https://regex101.com/r/LGhvms/1

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php OutdatedShow resolvedHide resolved

src/Symfony/Component/EventDispatcher/Tests/DependencyInjection/RegisterListenersPassTest.php OutdatedShow resolvedHide resolved

ro0NL approved these changes

Jul 2, 2021

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

i tend to believe it's a bugfix (as method calls are case-insensitive). Camel casing atevery word boundary, but not_, feels odd

ppl relying on case-sensitive method calls in DI config (which now changes) rely on implementation details IMHO. But 5.x would be as fine i guess 👍

@jjsty1e how did you discovered this? :D

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jul 2, 2021

ro0NL reviewed

Jul 2, 2021

View reviewed changes

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

		$event['method'] ='on'.preg_replace_callback([
		'/(?<=\b)[a-z]/i',
		'/(?<=\b\|_)[a-z]/i',
		'/[^a-z0-9]/i',

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

btw this 2nd regex is a leftover fromhttps://github.com/symfony/symfony/pull/1162/files it seems, where it was replaced with empty string which now happens at L87, this happened with refactoring likely.

AFAIK it's redundant 👼