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

[HttpKernel] Fix handling of`MapRequest*` attributes#50125

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

chalasr merged 1 commit intosymfony:6.3fromnicolas-grekas:hk-map-req

May 2, 2023

Merged

[HttpKernel] Fix handling of`MapRequest*` attributes#50125

chalasr merged 1 commit intosymfony:6.3fromnicolas-grekas:hk-map-req

May 2, 2023

Conversation

Copy link

Member

nicolas-grekas commentedApr 23, 2023

Q	A
Branch?	6.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#50120
License	MIT
Doc PR	-

carsonbot added Status: Needs Review Bug HttpKernel labels

Apr 23, 2023

carsonbot added this to the6.3 milestone

Apr 23, 2023

nicolas-grekas mentioned this pull request

Apr 23, 2023

[HttpKernel]#[MapRequestPayload] is "handled" before#[IsGranted]#50120

Closed

Copy link

Contributor

artyuum commentedApr 23, 2023

@nicolas-grekas I cloned your repo, checked out your branch and used thelink script to copy that specific version of Symfony into a working Sf project and I'm having the following error (even after nuking the cache):

Attempted to call an undefined method named "onKernelControllerArguments" of class "Symfony\Component\HttpKernel\Controller\ArgumentResolver\TraceableValueResolver".

Stacktrace

Symfony\Component\ErrorHandler\Error\UndefinedMethodError:Attempted to call an undefined method named "onKernelControllerArguments" of class "Symfony\Component\HttpKernel\Controller\ArgumentResolver\TraceableValueResolver".  at /var/www/html/test/vendor/symfony/event-dispatcher/Debug/WrappedListener.php:116  at Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke()     (/var/www/html/test/vendor/symfony/event-dispatcher/EventDispatcher.php:220)  at Symfony\Component\EventDispatcher\EventDispatcher->callListeners()     (/var/www/html/test/vendor/symfony/event-dispatcher/EventDispatcher.php:56)  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch()     (/var/www/html/test/vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php:139)  at Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch()     (/var/www/html/test/vendor/symfony/http-kernel/HttpKernel.php:161)  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()     (/var/www/html/test/vendor/symfony/http-kernel/HttpKernel.php:74)  at Symfony\Component\HttpKernel\HttpKernel->handle()     (/var/www/html/test/vendor/symfony/http-kernel/EventListener/ErrorListener.php:108)  at Symfony\Component\HttpKernel\EventListener\ErrorListener->onKernelException()     (/var/www/html/test/vendor/symfony/event-dispatcher/Debug/WrappedListener.php:116)  at Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke()     (/var/www/html/test/vendor/symfony/event-dispatcher/EventDispatcher.php:220)  at Symfony\Component\EventDispatcher\EventDispatcher->callListeners()     (/var/www/html/test/vendor/symfony/event-dispatcher/EventDispatcher.php:56)  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch()     (/var/www/html/test/vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php:139)  at Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch()     (/var/www/html/test/vendor/symfony/http-kernel/HttpKernel.php:224)  at Symfony\Component\HttpKernel\HttpKernel->handleThrowable()     (/var/www/html/test/vendor/symfony/http-kernel/HttpKernel.php:117)  at Symfony\Component\HttpKernel\HttpKernel->terminateWithException()     (/var/www/html/test/vendor/symfony/http-kernel/EventListener/DebugHandlersListener.php:74)  at Symfony\Component\HttpKernel\EventListener\DebugHandlersListener::Symfony\Component\HttpKernel\EventListener\{closure}()     (/var/www/html/test/vendor/symfony/error-handler/ErrorHandler.php:537)  at Symfony\Component\ErrorHandler\ErrorHandler->handleException()

Do you have the same error?

nicolas-grekas force-pushed thehk-map-req branch froma97336b to949a2e2Compare

April 23, 2023 16:55

Copy link

MemberAuthor

nicolas-grekas commentedApr 23, 2023

Yes, I do. This should now be fixed. Can you please try again?

Copy link

Contributor

artyuum commentedApr 23, 2023

@nicolas-grekas it works. I commented on the issue.

Copy link

Contributor

Koc commentedApr 23, 2023

We should be careful with usage of the payload as Subject from Controller Arguments in#[IsGranted Attribute

Koc mentioned this pull request

Apr 23, 2023

[HttpKernel] Introduce#[MapUploadedFile] controller argument attribute#49978

Merged

nicolas-grekas force-pushed thehk-map-req branch from949a2e2 to6cff3b1Compare

April 24, 2023 06:30

nicolas-grekas commented

Apr 24, 2023

View reviewed changes

Copy link

MemberAuthor

nicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

We should be careful with usage of the payload as Subject from Controller Arguments in [#IsGranted Attribute

Accessing unserialized payloads in#[IsGranted] directly contradicts the expectations described in#50120: the expectation there is that a payload shouldn't be parsed if we can rule out access to some route before said parsing happens, which makes sense to me.

One way to solve this would be to allow registering an IsGranted attribute for late evaluation, aka after other controller argument listeners. There might be others.

Let's merge this PR and solve late the case of accessing the parsed subject from IsGranted?

src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php OutdatedShow resolvedHide resolved

nicolas-grekas force-pushed thehk-map-req branch from6cff3b1 toc9855d0Compare

April 24, 2023 12:02

nicolas-grekas requested review fromchalasr,dunglas andwouterj ascode owners

April 24, 2023 12:02

nicolas-grekas force-pushed thehk-map-req branch 2 times, most recently from791c5f3 to6a82314Compare

April 24, 2023 12:26

Copy link

MemberAuthor

nicolas-grekas commentedApr 24, 2023

I updated the way traceable resolvers and serializers are wired so that we don't rely on service decoration, but use standard decoration instead. This preserves the definition of the original decorated service, so that we can e.g. use several tags on them without conflicts.