Skip to content

[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.

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
Merged
merged 1 commit into from
May 2, 2023
Merged

[HttpKernel] Fix handling of MapRequest* attributes #50125

merged 1 commit into from
May 2, 2023

Conversation

nicolas-grekas
Copy link
Member

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

@carsonbot carsonbot added this to the 6.3 milestone Apr 23, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 23, 2023
Closed
@artyuum
Copy link
Contributor

artyuum commented Apr 23, 2023

@nicolas-grekas I cloned your repo, checked out your branch and used the link 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
Copy link
Member Author

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

@artyuum
Copy link
Contributor

artyuum commented Apr 23, 2023

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

@Koc
Copy link
Contributor

Koc commented Apr 23, 2023

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

@Koc Koc mentioned this pull request Apr 23, 2023
Merged
nicolas-grekas
nicolas-grekas commented Apr 24, 2023
View reviewed changes
Copy link
Member Author

@nicolas-grekas 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?

@nicolas-grekas nicolas-grekas force-pushed the hk-map-req branch 2 times, most recently from 791c5f3 to 6a82314 Compare April 24, 2023 12:26
@nicolas-grekas
Copy link
Member Author

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.

@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Apr 24, 2023
@nicolas-grekas
Copy link
Member Author

PR rebased on top of #50158 to split the concern of decoration apart.

@nicolas-grekas
Copy link
Member Author

PR rebased and ready.

@chalasr
Copy link
Member

chalasr commented May 2, 2023

Thanks @nicolas-grekas.

@chalasr chalasr merged commit 0d25263 into symfony:6.3 May 2, 2023
@nicolas-grekas nicolas-grekas deleted the hk-map-req branch May 2, 2023 21:02
@fabpot fabpot mentioned this pull request May 7, 2023
Merged
@DjordyKoert DjordyKoert mentioned this pull request Jul 14, 2023
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 17, 2024
Closed
@stof stof mentioned this pull request Apr 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Koc Koc Koc left review comments

@derrabus derrabus derrabus approved these changes

@chalasr chalasr chalasr approved these changes

@wouterj wouterj Awaiting requested review from wouterj

@dunglas dunglas Awaiting requested review from dunglas

Assignees
No one assigned
Labels
Bug HttpKernel ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

[HttpKernel] #[MapRequestPayload] is "handled" before #[IsGranted]
6 participants
@nicolas-grekas @artyuum @Koc @chalasr @derrabus @carsonbot