BoltTranslate / Translate

Provides translation for contenttypes.
Other
43 stars 37 forks source link

[BUG] Overriding 'controller.frontend' can lead to race condition (with other extensions) #118

Closed mikemcgowan closed 7 years ago

mikemcgowan commented 7 years ago

The following code in TranslateExtension.php, replaces $app['controller.frontend'] with an instance of LocalizedFrontend (which extends Bolt's own Frontend).

            $app['controller.frontend'] = $app->share(
                function ($app) {
                    $frontend = new Frontend\LocalizedFrontend();
                    $frontend->connect($app);

                    return $frontend;
                }
            );

If another extension does the same as the above, the actual controller that gets created (and therefore the behaviour of the site) will be whichever extension happens to be the last to have its registerServices method invoked (and therefore replace $app['controller.frontend'] with its own implementation). This doesn't seem right to me.

Would it not be better to have some kind of decorator pattern here, allowing each extension its own chance to respond when a 'record' is requested? Or perhaps make use of Pimple's extend?

lenvanessen commented 7 years ago

The way the translate extension overrides the Front-end controllers is not ideal, but that's a limitation of the current way the Frontend controller in core works now.

I think we best move this to an RFC for Bolt 4.0, since in the 3.x there it would be near to impossible to prevent BC-breaks if we'd switch it over to a decorator pattern.

For now i'm closing and i'll bring up the suggestion for the core.

As always, feel free to re-open if you want to add something

lenvanessen commented 7 years ago

For reference: https://github.com/bolt/bolt/issues/7031

GwendolenLynch commented 7 years ago

As I mention in bolt/bolt#7031 this should be done via a view event, and extensions shouldn't override controllers (bundles are a different story)