Skip to content

[HttpKernel] Add ControllerEvent::getAttributes() to handle attributes on controllers #46001

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
Jul 7, 2022
Merged

[HttpKernel] Add ControllerEvent::getAttributes() to handle attributes on controllers #46001

merged 1 commit into from
Jul 7, 2022

Conversation

nicolas-grekas
Copy link
Member

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

Replacing #45457. Paving the way toward #44705

@carsonbot carsonbot added this to the 6.1 milestone Apr 11, 2022
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 11, 2022
Closed
@lyrixx
Copy link
Member

lyrixx commented Apr 11, 2022

I wanted do that for ages! Thanks for doing it. But iirc we already had an instance of the reflection somewhere. Can't we reuse it?

@nicolas-grekas nicolas-grekas force-pushed the hk-controller-attr branch 3 times, most recently from 400b10a to d15dbbe Compare April 12, 2022 08:38
@nicolas-grekas
Copy link
Member Author

we already had an instance of the reflection somewhere. Can't we reuse it?

yes we can, updated!

@nicolas-grekas nicolas-grekas force-pushed the hk-controller-attr branch 2 times, most recently from 6f57daa to e2d28fe Compare April 12, 2022 08:53
@fabpot fabpot modified the milestones: 6.1, 6.2 Apr 27, 2022
@nicolas-grekas nicolas-grekas force-pushed the hk-controller-attr branch 2 times, most recently from 92919c9 to b7231ed Compare July 5, 2022 13:49
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 5, 2022 •
edited
Loading

PR is ready for another round of reviews.

@stof, I addressed your comment this way:

the attributes being added in the ControllerEvent need to be exposed (as read-only) in the ControllerArgumentsEvent.

the IsGrantedListener runs on ControllerArgumentsEvent, not on ControllerEvent. The goal is to run after argument resolvers, so that you can vote on the resolved arguments.

Understood and done. Just one thing: because there is ControllerArgumentsEvent::setController(), I think we should allow also updating the attributes. This is what the patch does.

should we have a addAttribute method instead, that allows adding additional attributes without loosing the ones coming from Reflection ?

well, the need to perform a recursive merge is easy to overlook. And I don't see any use case where you would want to remove the attributes coming from Reflection. This would be a big WTF for someone having to debug why their attributes have no effect.

I replaced setAttributes() by a new $attributes argument on setController() and a new addAttribute() method. Note that I think it's important to allow ppl to opt-out from reflection. This is the same principle that we're following everywhere we read attributes: they should remain declarative, which means optional.

@nicolas-grekas nicolas-grekas changed the title [HttpKernel] Add ControllerEvent::getAttributes() to allow reading/defining attributes on controllers [HttpKernel] Add ControllerEvent::getAttributes() to handle attributes on controllers Jul 5, 2022
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 5, 2022 •
edited
Loading

I missed this comment:

if setController is called before setAttributes (likely to be the case if a listener is the one calling setAttributes to set custom attributes), the assignment done in setController will not include those custom attributes.

Having $attributes on setController() binds controllers and their attributes. I think this solve this concern. But having a separate addAttribute() method reintroduces this concern. That's why I've removed it. I think we need to provide a way to override real attributes, but this way should not be a foot-gun. Since it's for advanced use cases, it's fine to me if it's a bit involving.

To me, the kernel should be the one doing that assignment after triggering the event, based on $event->getAttributes

I want everything to remain lazy, that's what the current implementation provides.

@nicolas-grekas nicolas-grekas force-pushed the hk-controller-attr branch 2 times, most recently from a61ee44 to 4b14fa9 Compare July 5, 2022 15:12
chalasr
chalasr approved these changes Jul 5, 2022
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍

src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
@@ -21,22 +21,15 @@ final class ArgumentMetadataFactory implements ArgumentMetadataFactoryInterface
/**
* {@inheritdoc}
*/
public function createArgumentMetadata(string|object|array $controller): array
public function createArgumentMetadata(string|object|array $controller, \ReflectionClass $class = null, \ReflectionFunction $reflection = null): array
Copy link
Member

Choose a reason for hiding this comment

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

final + interface ftw 💚

@nicolas-grekas nicolas-grekas mentioned this pull request Jul 6, 2022
Closed
7 tasks
@chalasr
Copy link
Member

chalasr commented Jul 7, 2022

Thank you @nicolas-grekas.

@chalasr chalasr force-pushed the hk-controller-attr branch from 946a6e5 to 0f2293c Compare July 7, 2022 09:51
@chalasr chalasr merged commit 2633877 into symfony:6.2 Jul 7, 2022
@chalasr chalasr mentioned this pull request Jul 7, 2022
Closed
@nicolas-grekas nicolas-grekas deleted the hk-controller-attr branch July 7, 2022 13:38
@catch56 catch56 mentioned this pull request Sep 2, 2022
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Sep 2, 2022
Merged
fabpot added a commit that referenced this pull request Sep 2, 2022
@fabpot
bug #47468 [HttpKernel] Don't cache controller's reflector inside the…
0e28848 
… request (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[HttpKernel] Don't cache controller's reflector inside the request

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #47461
| License       | MIT
| Doc PR        | -

Issue introduced by #46001

Commits
-------

c751bd0 [HttpKernel] Don't cache controller's reflector inside the request
@michaljusiega michaljusiega mentioned this pull request Oct 13, 2022
Closed
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
@mpdude mpdude mentioned this pull request Oct 28, 2024
Merged
mpdude added a commit to webfactory/WebfactoryWfdMetaBundle that referenced this pull request Oct 28, 2024
@mpdude
Read attributes as provided by the event object, and pass on attribut…
fcabc29 
…es to other event handlers (#53)

This PR uses the new API introduced in
symfony/symfony#46001 to read controller
attributes through the `ControllerEvent`, and to make them available to
other event handlers when replacing the controller.

This is necessary when using the ´Send304IfNotModified` attribute in
combination with `\Symfony\Component\HttpKernel\Attribute\Cache`.
Without this change,
`\Symfony\Component\HttpKernel\EventListener\CacheAttributeListener`
will not set `Cache` headers accordingly. The result is that you may get
`304 Not Modified` responses on conditional requests with
`If-Modified-Since`, but these are treated as `stale/cache` only in the
HttpCache and have a `Cache-Control: must-revalidate, private` header.
@MalteWunsch MalteWunsch mentioned this pull request Nov 19, 2024
Merged
MalteWunsch added a commit to webfactory/WebfactoryHttpCacheBundle that referenced this pull request Nov 19, 2024
@MalteWunsch
Read attributes as provided by the event object, and pass on attribut…
f613d44 
…es to other event handlers (Case 177753) (#15)

This adapts
webfactory/WebfactoryWfdMetaBundle#53:

> This PR uses the new API introduced in
symfony/symfony#46001 to read controller
attributes through the `ControllerEvent`, and to make them available to
other event handlers when replacing the controller.
>
> This is necessary when using the `Send304IfNotModified` attribute in
combination with `\Symfony\Component\HttpKernel\Attribute\Cache`.
Without this change,
`\Symfony\Component\HttpKernel\EventListener\CacheAttributeListener`
will not set `Cache` headers accordingly. The result is that you may get
`304 Not Modified` responses on conditional requests with
`If-Modified-Since`, but these are treated as `stale/cache` only in the
HttpCache and have a `Cache-Control: must-revalidate, private` header.

And adds some tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catch56 catch56 catch56 left review comments

@stof stof stof left review comments

@GromNaN GromNaN GromNaN approved these changes

@lyrixx lyrixx lyrixx approved these changes

@yceruto yceruto yceruto approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Feature HttpKernel Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
9 participants
@nicolas-grekas @lyrixx @chalasr @catch56 @GromNaN @stof @yceruto @fabpot @carsonbot