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

[RFC] Deprecate bundle inheritance #24048

Closed
javiereguiluz opened this Issue Aug 31, 2017 · 25 comments

Comments

Projects
None yet
@javiereguiluz
Member

javiereguiluz commented Aug 31, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? yes
RFC? yes
Symfony version 3.4

After having upgraded several apps to Symfony Flex (both small and complex apps) the worst thing to me was the overriding of third-party elements. Templates for example where in app/Resources/AcmeBundle/views/ and now should be in src/Resources/AcmeBundle/views/..., etc.

I asked around in the Symfony Slack chat and the first responses confirmed that bundle inheritance is rarely used, except to customize some templates:

Do you use that in your real projects? Do you override controllers, templates, etc. from third-party bundles?

-> Rarely, but it has happened
-> I've seen that in ApiPlatform, people override the swagger html twig file
-> Apart error pages from twig, I almost never override resources from third-party bundle
-> I overrode ~70% of FosUserBundle, but without bundle inheritance
-> Only the error pages from twig.
-> Me too, only error pages (on real projects)
-> I used it along time ago for RollerworksMultiUserBundle. but that project is EOL (for good!)
-> I only override templates and assets within app/Resources/. Never did any overriding using an inheriting bundle.

Deprecating this feature in 3.4 and removing it in 4.0 is our last chance in several years to do that.

If you agree, we should provide simple alternatives to override all that is listed in this article: How to Override any Part of a Bundle

  • templates
  • routing
  • controllers
  • services
  • configuration
  • entities
  • forms
  • validation
  • translation
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 31, 2017

Member

Thus, if you want to override bundle templates in a project, you don't need to use bundle inheritance (as there is a kernel-level folder allowing this).
I would not remove the ability to override bundle templates at project-level (which is just about using the Twig built-in feature of registering multiple paths for the same namespace).

I'm all in favor of deprecating this feature. Overriding bundle controllers or imported bundle resources created WTF debugging times anytime I used them (and I stopped doing it at least 5 years ago due to that).

Member

stof commented Aug 31, 2017

Thus, if you want to override bundle templates in a project, you don't need to use bundle inheritance (as there is a kernel-level folder allowing this).
I would not remove the ability to override bundle templates at project-level (which is just about using the Twig built-in feature of registering multiple paths for the same namespace).

I'm all in favor of deprecating this feature. Overriding bundle controllers or imported bundle resources created WTF debugging times anytime I used them (and I stopped doing it at least 5 years ago due to that).

@Pierstoval

This comment has been minimized.

Show comment
Hide comment
@Pierstoval

Pierstoval Aug 31, 2017

Contributor

Same for me. Everytime I saw bundle inheritance OR service/controller/whatever override, it was a PITA to debug or maintain. Templates that are copy-pasted in app/Resources, when the parent bundle is updated, all logic is lost because copy/paste has to re-happen again, and plenty other problems when overriding stuff.

I'm 👍 for deprecating this feature, but not sure it's possible to remove it Symfony 4, as feature freeze is next month and we're very close (3-4 months) from SF4 release, I'm afraid too many people / OSS maintainers won't be able to follow this big upgrade, especially for bundles like FOSUserBundle which is still the most used.

Contributor

Pierstoval commented Aug 31, 2017

Same for me. Everytime I saw bundle inheritance OR service/controller/whatever override, it was a PITA to debug or maintain. Templates that are copy-pasted in app/Resources, when the parent bundle is updated, all logic is lost because copy/paste has to re-happen again, and plenty other problems when overriding stuff.

I'm 👍 for deprecating this feature, but not sure it's possible to remove it Symfony 4, as feature freeze is next month and we're very close (3-4 months) from SF4 release, I'm afraid too many people / OSS maintainers won't be able to follow this big upgrade, especially for bundles like FOSUserBundle which is still the most used.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 1, 2017

Member

👍 for me too

Member

xabbuh commented Sep 1, 2017

👍 for me too

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Sep 1, 2017

Member

👍

Member

chalasr commented Sep 1, 2017

👍

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Sep 2, 2017

Contributor

Controllers and Form types can be overwritten by registering them services 😛 services take precedence over initialization by FQCN.

For entities you can use the mapping configuration, to point to a another directory (with a different mapping type, IMO this is actually something the bundle maintainer should handle themselves).

Contributor

sstok commented Sep 2, 2017

Controllers and Form types can be overwritten by registering them services 😛 services take precedence over initialization by FQCN.

For entities you can use the mapping configuration, to point to a another directory (with a different mapping type, IMO this is actually something the bundle maintainer should handle themselves).

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 5, 2017

Contributor

For 3.4 we would simply check if you defined getParent in a bundle, if so, trigger a deprecation. Right?

I wondered.. where will that happen. From the kernel?

Contributor

ro0NL commented Sep 5, 2017

For 3.4 we would simply check if you defined getParent in a bundle, if so, trigger a deprecation. Right?

I wondered.. where will that happen. From the kernel?

@Pierstoval

This comment has been minimized.

Show comment
Hide comment
@Pierstoval

Pierstoval Sep 5, 2017

Contributor

@ro0NL yep, this can be done in Kernel::initializeBundles()

Contributor

Pierstoval commented Sep 5, 2017

@ro0NL yep, this can be done in Kernel::initializeBundles()

@NicoHaase

This comment has been minimized.

Show comment
Hide comment
@NicoHaase

NicoHaase Sep 5, 2017

Just recently, I used the getParent logic to override a bundled template in another bundle which we use in multiple projects. Will there be any alternative? Even if you determine that you do not use it anymore and debugging is hard, I'd love to see good arguments for removing this and not just leaving it as it is and adding huge warnings that using this is a really bad idea, but you can use it if you take all risks

NicoHaase commented Sep 5, 2017

Just recently, I used the getParent logic to override a bundled template in another bundle which we use in multiple projects. Will there be any alternative? Even if you determine that you do not use it anymore and debugging is hard, I'd love to see good arguments for removing this and not just leaving it as it is and adding huge warnings that using this is a really bad idea, but you can use it if you take all risks

@Pierstoval

This comment has been minimized.

Show comment
Hide comment
@Pierstoval

Pierstoval Sep 5, 2017

Contributor

Just recently, I used the getParent logic to override a bundled template in another bundle which we use in multiple projects.

You actually don't need to use getParent() for this. Just create the app/Resources/{ExternalBundleName}/views/{path to your view}.twig and you're set.

Contributor

Pierstoval commented Sep 5, 2017

Just recently, I used the getParent logic to override a bundled template in another bundle which we use in multiple projects.

You actually don't need to use getParent() for this. Just create the app/Resources/{ExternalBundleName}/views/{path to your view}.twig and you're set.

@NicoHaase

This comment has been minimized.

Show comment
Hide comment
@NicoHaase

NicoHaase Sep 5, 2017

@Pierstoval: I know about that. But as I use the bundle which overwrites some core template in multiple projects, I explicitly don't want to override these templates in app/Resources in each of these projects, but do it centralized in the very bundle that each of these projects uses

NicoHaase commented Sep 5, 2017

@Pierstoval: I know about that. But as I use the bundle which overwrites some core template in multiple projects, I explicitly don't want to override these templates in app/Resources in each of these projects, but do it centralized in the very bundle that each of these projects uses

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 5, 2017

Member

You can explicitly register your bundle's template path as a path for the namespace of the parent bundle (that's what happens in the background when using bundle inheritance).

Member

xabbuh commented Sep 5, 2017

You can explicitly register your bundle's template path as a path for the namespace of the parent bundle (that's what happens in the background when using bundle inheritance).

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 5, 2017

Member

To be exact, the solution given by @xabbuh works when using the @Foo/baz.html.twig notation. When using the FooBundle::baz.html.twig notation, only bundle inheritance can allow a bundle to provide template overrides. But this syntax should not be used anymore (in Symfony 4, it won't be available by default, as this requires the templating component, which won't be included by default anymore)

Member

stof commented Sep 5, 2017

To be exact, the solution given by @xabbuh works when using the @Foo/baz.html.twig notation. When using the FooBundle::baz.html.twig notation, only bundle inheritance can allow a bundle to provide template overrides. But this syntax should not be used anymore (in Symfony 4, it won't be available by default, as this requires the templating component, which won't be included by default anymore)

@NicoHaase

This comment has been minimized.

Show comment
Hide comment
@NicoHaase

NicoHaase Sep 5, 2017

@xabbuh: can you give an example about how to do this? It would be nice to include this to the upgrade notes if you finally decide to remove bundle inheritance :)

NicoHaase commented Sep 5, 2017

@xabbuh: can you give an example about how to do this? It would be nice to include this to the upgrade notes if you finally decide to remove bundle inheritance :)

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 5, 2017

Member

@NicoHaase this is a real example in a Symfony app:

twig:
    debug:            '%kernel.debug%'
    strict_variables: '%kernel.debug%'
    paths:
        '%kernel.project_dir%/templates': ~
        '%kernel.project_dir%/templates/error': Twig

Now I can create templates/error/Exception/error.html.twig to override the error pages from TwigBundle.

The paths config expects an array of dir paths and optionally one Twig namespace for each of them. In the case of bundles, the namespace is the name of the bundle without Bundle suffix: Twig = TwigBundle, FOSUser = FOSUserBundle, etc.

And if we finally deprecate this, we'll upgrade all our docs to explain the alternatives.

Member

javiereguiluz commented Sep 5, 2017

@NicoHaase this is a real example in a Symfony app:

twig:
    debug:            '%kernel.debug%'
    strict_variables: '%kernel.debug%'
    paths:
        '%kernel.project_dir%/templates': ~
        '%kernel.project_dir%/templates/error': Twig

Now I can create templates/error/Exception/error.html.twig to override the error pages from TwigBundle.

The paths config expects an array of dir paths and optionally one Twig namespace for each of them. In the case of bundles, the namespace is the name of the bundle without Bundle suffix: Twig = TwigBundle, FOSUser = FOSUserBundle, etc.

And if we finally deprecate this, we'll upgrade all our docs to explain the alternatives.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 5, 2017

Contributor

The @javiereguiluz's example should be the right solution regarding to project-level (App-to-Bundle), BUT inside a bundle that needs to override templates from another one (@NicoHaase's case, Bundle-to-Bundle) I might need to implement PrependExtensionInterface in my DI extension to be able to add the new path before the current one in the same namespace:

class AcmeHelloExtension extends Extension implements PrependExtensionInterface
{
    public function prepend(ContainerBuilder $container)
    {
        $container->prependExtensionConfig('twig', array(
            'paths' => array(
                __DIR__.'/../Resources/views' => 'Twig',
            ),
        ));
    }
}

Now I can create AcmeHelloBundle/Resources/views/Exception/error.html.twig to override the error pages from TwigBundle.

Btw, the %kernel.project_dir% parameter can be omitted from path value as it is already the default root path for each relative path.

Contributor

yceruto commented Sep 5, 2017

The @javiereguiluz's example should be the right solution regarding to project-level (App-to-Bundle), BUT inside a bundle that needs to override templates from another one (@NicoHaase's case, Bundle-to-Bundle) I might need to implement PrependExtensionInterface in my DI extension to be able to add the new path before the current one in the same namespace:

class AcmeHelloExtension extends Extension implements PrependExtensionInterface
{
    public function prepend(ContainerBuilder $container)
    {
        $container->prependExtensionConfig('twig', array(
            'paths' => array(
                __DIR__.'/../Resources/views' => 'Twig',
            ),
        ));
    }
}

Now I can create AcmeHelloBundle/Resources/views/Exception/error.html.twig to override the error pages from TwigBundle.

Btw, the %kernel.project_dir% parameter can be omitted from path value as it is already the default root path for each relative path.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 5, 2017

Member

you probably don't need to use $bundlesMetadata['AcmeHelloBundle']['path'], as the path to the root of your own bundle is just __DIR__.'/..'

Member

stof commented Sep 5, 2017

you probably don't need to use $bundlesMetadata['AcmeHelloBundle']['path'], as the path to the root of your own bundle is just __DIR__.'/..'

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 5, 2017

Contributor

@stof yes I know (unrelated) but __DIR__ constant is considered a bugrisk for SensioLabsInsight so whenever I can, I try not to use it ;)

EDIT: yep I guess the risk here is minimal as core bundles use it, sample updated, thanks.

Contributor

yceruto commented Sep 5, 2017

@stof yes I know (unrelated) but __DIR__ constant is considered a bugrisk for SensioLabsInsight so whenever I can, I try not to use it ;)

EDIT: yep I guess the risk here is minimal as core bundles use it, sample updated, thanks.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 6, 2017

Member

👍

Member

lyrixx commented Sep 6, 2017

👍

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 11, 2017

Member

So, who would like to try a PR for this? Feat. freeze is coming.

Member

nicolas-grekas commented Sep 11, 2017

So, who would like to try a PR for this? Feat. freeze is coming.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 11, 2017

Member

I will submit a PR soon.

Member

fabpot commented Sep 11, 2017

I will submit a PR soon.

fabpot added a commit that referenced this issue Sep 15, 2017

feature #24160 [HttpKernel] Deprecate bundle inheritance (fabpot)
This PR was squashed before being merged into the 3.4 branch (closes #24160).

Discussion
----------

[HttpKernel] Deprecate bundle inheritance

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #24048
| License       | MIT
| Doc PR        | symfony/symfony-docs#8389

Commits
-------

89893c1 [HttpKernel] deprecated bundle inheritance
ee9f4c3 fixed CS

@fabpot fabpot closed this Sep 15, 2017

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 15, 2017

Member

@yceruto SensioLabsInsight considers it a bugrisk because it might break the possibility to override through bundle inheritance in some cases. This rule should disappear from Insight (even in 2.x or 3.x, this rule is bad because it is applied to lots of places where bundle inheritance does not play any role)

Member

stof commented Sep 15, 2017

@yceruto SensioLabsInsight considers it a bugrisk because it might break the possibility to override through bundle inheritance in some cases. This rule should disappear from Insight (even in 2.x or 3.x, this rule is bad because it is applied to lots of places where bundle inheritance does not play any role)

@MustaphaGhlissi

This comment has been minimized.

Show comment
Hide comment
@MustaphaGhlissi

MustaphaGhlissi Jan 5, 2018

Hi every body ,

I got started recently a new project using Symfony 4

I have a problem ,

I am using FOSUserBundle works fine but I need to override the FOS SecurityController
Previously we use getParent() function but this feature was deprecated and which alternative solution can use in my case ??

Regards

MustaphaGhlissi commented Jan 5, 2018

Hi every body ,

I got started recently a new project using Symfony 4

I have a problem ,

I am using FOSUserBundle works fine but I need to override the FOS SecurityController
Previously we use getParent() function but this feature was deprecated and which alternative solution can use in my case ??

Regards

@Pierstoval

This comment has been minimized.

Show comment
Hide comment
@Pierstoval

Pierstoval Jan 5, 2018

Contributor

You have to create your own controller and recreate the route you want to override, I think it's the only way.

Contributor

Pierstoval commented Jan 5, 2018

You have to create your own controller and recreate the route you want to override, I think it's the only way.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz
Member

javiereguiluz commented Jan 5, 2018

@MustaphaGhlissi the solution is what @Pierstoval said and it's explained here: https://symfony.com/doc/current/bundles/override.html

@MustaphaGhlissi

This comment has been minimized.

Show comment
Hide comment
@MustaphaGhlissi

MustaphaGhlissi commented Jan 5, 2018

Thanks guys,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment