Skip to content

[RFC] Abandon FrameworkExtraBundle #44705

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

Closed
derrabus opened this issue Dec 18, 2021 · 16 comments
Closed

[RFC] Abandon FrameworkExtraBundle #44705

derrabus opened this issue Dec 18, 2021 · 16 comments
Labels
Keep open RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@derrabus
Copy link
Member

Description

Since Symfony 2 sensio/framework-extra-bundle is the solution for adding annotation-magic to the application. Since then, some functionality has been moved to the core (e.g. annotation-based routing) and other functionality has been obsoleted by a core feature (param converters Vs. argument value providers).

FrameworkExtraBundle feels like a core-bundle but it's not as well maintained as we would expect a core bundle to be.

I think, it would be a good idea to review the feature set of the bundle and move commonly used functionality to the monorepo. Speaking for myself, that would be the security annotations like IsGranted as well as the Doctrine entity param converter (see #43854).

What do you think?

Example

No response

@carsonbot carsonbot added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Dec 18, 2021
@fabpot
Copy link
Member

fabpot commented Dec 18, 2021

I agree.

@garak
Copy link
Contributor

garak commented Dec 19, 2021

I don't get the logic. There's a set of optional features in a bundle that is not well maintained. The solution should be maintaining that bundle, not putting optional features in the core.

I think a strong point of Symfony is having a slim base and modularity, I hope it's not going to transform in a copy of Laravel.

@wouterj
Copy link
Member

wouterj commented Dec 19, 2021

Let's not act as if moving 3 annotations as native PHP attributes to core will upset anyone please.

That being said, "the solution should be maintaining the bundle" is nicely said but not the practice. Apparently nobody is interest in maintaining the bundle (for years now).
So either it will be dead, we'll remove them from the official docs and that's it. Or we see value in some features and move the few attributes to core.

I'm all +1 for the latter.

@javiereguiluz
Copy link
Member

javiereguiluz commented Dec 19, 2021

@garak generally speaking I agree with you ... but in this particular case, I think the bundle was "wrong" since day one. I mean, these were so useful and common features that I never felt them as "extra features". In my opinion, they should've been "core/built-in features" since day one.

@fabpot
Copy link
Member

fabpot commented Dec 19, 2021

These feature were in a bundle as annotations in phpdocs were controversial back then. Now that we have native annotations, it makes sense to move them to core. Add simple as that.

@Nyholm
Copy link
Member

Nyholm commented Dec 20, 2021

I agree with this as well.

@derrabus will you do/coordinate the work?

@stof
Copy link
Member

stof commented Dec 20, 2021

As several features have not been migrated yet to the core, we cannot abandon the bundle yet (we probably cannot abandon it fully before the end of the 5.4 LTS, as the 5.4 branch won't have replacements).

However, I'm also in favor of bringing the remaining features to the core to abandon the bundle in the future

fabpot added a commit that referenced this issue Mar 25, 2022
This PR was merged into the 6.1 branch.

Discussion
----------

[HttpKernel] Add DateTimeValueResolver

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #44705
| License       | MIT
| Doc PR        | symfony/symfony-docs#16562

This PR replaces the [DateTimeParamConverter](https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/master/src/Request/ParamConverter/DateTimeParamConverter.php) from the SensioFrameworkExtraBundle with a value resolver in the Symfony Core.

Note that the behavior of the resolver is enabled by default when using the Symfony framework.

Commits
-------

dcc1228 [HttpKernel] Add DateTimeValueResolver
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@stof
Copy link
Member

stof commented Jun 21, 2022

@carsonbot keep this opened. The work is in progress in various places

@carsonbot carsonbot removed the Stalled label Jun 21, 2022
chalasr added a commit that referenced this issue Jul 7, 2022
… handle attributes on controllers (nicolas-grekas)

This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

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

| 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

Commits
-------

0f2293c [HttpKernel] Add `ControllerEvent::getAttributes()` to handle attributes on controllers
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 11, 2022

nicolas-grekas added a commit that referenced this issue Jul 12, 2022
This PR was merged into the 6.2 branch.

Discussion
----------

[Security] Add `#[IsGranted()]`

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

Extracted from #45415 (and modernized a lot).

I did not implement the proposals from Stof to keep this first iteration simple. I'd appreciate help to improve the attribute in a follow up PR 🙏

Commits
-------

bf8d75e [Security] Add `#[IsGranted()]`
nicolas-grekas added a commit that referenced this issue Jul 12, 2022
…nder arrays returned by controllers (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[TwigBridge] Add `#[Template()]` to describe how to render arrays returned by controllers

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

Extracted from #45415 (and modernized a lot).

Unlike the``@Template`` annotation, this attribute mandates a template name as argument. This removes the need for any template-guesser logic, making things explicit.

Using the attribute also turns `FormInterface` instances into `FormView`, adjusting the status code to 422 when a non-valid form is found in the parameters, similarly to what `AbstractController::render()` does.

Commits
-------

c252606 [TwigBridge] Add `#[Template()]` to describe how to render arrays returned by controllers
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 12, 2022

ParamConverter ➡️ Symfony\Component\HttpKernel\Attribute\ParamConverter

We don't need this one, do we?

@stof
Copy link
Member

stof commented Jul 12, 2022

indeed, I don't think we need it

chalasr added a commit that referenced this issue Jul 12, 2022
…HTTP cache headers on controllers (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[HttpKernel] Add `#[Cache()]` to describe the default HTTP cache headers on controllers

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

Extracted from #45415 (and modernized a lot).

I'd appreciate any help for porting the other attributes following this leading PR 🙏

Commits
-------

acd4fa7 [HttpKernel] Add `#[Cache]` to describe the default HTTP cache headers on controllers
@chalasr
Copy link
Member

chalasr commented Jul 21, 2022

The main features have been migrated to core. Let's iterate and deprecate FrameworkExtraBundle :)

@chalasr chalasr closed this as completed Jul 21, 2022
@stof
Copy link
Member

stof commented Jul 21, 2022

Well, let's wait for the release of the core feature first before deprecating FrameworkExtraBundle

@arderyp
Copy link
Contributor

arderyp commented Feb 29, 2024

@stof I know this is an old closed issue but I have a relevant question.

I am on the latest 5.4.x. I just migrated from annotations to attributes. Everything is working great. grep is returning no instances of Sensio or sensio anywhere in my code. I dropped by sensio_framework_extra_bundle config. Everything is working.

And yet... when I remove the SensioFrameworkExtraBundle entry from config/bundles.php, my app breaks many routes with errors about being unable to map entity arguments in controller route parameters (like this).

It looks like ParamConverterListener is the source of my problem. Does Symfony not have a native replacement for that until 6.2?

@derrabus
Copy link
Member Author

Please use our discussions board for support questions: https://github.com/symfony/symfony/discussions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Keep open RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.