schmittjoh / JMSTranslationBundle

Puts the Symfony2 Translation Component on steroids
http://jmsyst.com/bundles/JMSTranslationBundle
427 stars 291 forks source link

Allow to use translate controller without sensio/framework-extra-bundle #569

Closed deguif closed 1 year ago

deguif commented 1 year ago
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass?
Fixed tickets
License Apache2

Description

Allow to use the translate controller without the sensio/framework-extra-bundle package.

deguif commented 1 year ago

@goetas would you have some time to review this one? It would allow to get ride of sensio/framework-extra-bundle

deguif commented 1 year ago

@goetas this is going to be more relevant as sensio/framework-extra-bundle is going to be deprecated in a near future. See: https://github.com/symfony/symfony/issues/44705

goetas commented 1 year ago

looks good, thanks!

ryanmab commented 1 year ago

@deguif @goetas I've just been trying to remove the sensio/framework-extra-bundle dependency from one of my projects which is using the JMS bundle, and noticed that it seems like we can't make use of the Web UI without still having sensio/framework-extra-bundle available. Otherwise, we receive this exception:

I can see the annotations are still in use here, and here.

Is this by design, or should we also remove these annotations from the TranslateController entirely?

gnat42 commented 1 year ago

I would remove all use of @Template annotation method and just use a return in the controller to return the response rendered. Its a very old way to deal with templates in Symfony projects and there's no real benefit.

deguif commented 1 year ago

@ryanmab in the meantime you can probably use Doctrine\Common\Annotations\AnnotationReader::addGlobalIgnoredName(className) in your bootstrap file to ignore the @Template annotation

deguif commented 1 year ago

@gnat42 I'm not sure about the BC breaks it would introduce by removing the annotation

deguif commented 1 year ago

@Template was not designed for being used in bundles ;)

gnat42 commented 1 year ago

@deguif Fair enough about the BC break.

However, a couple of thoughts.

  1. Since this was merged Dec 19, 2022 I would consider it a bug of the PR.
  2. Additionally, I would consider using the @Template annotation an internal implementation. For examplesomeone including this bundle shouldn't care or depend on the annotation existing. Removing it and return the appropriate Response object makes the controller function the same in both ways.
  3. If you only add it to the global ignore list, the controller will throw an exception if it doesn't return a Response object. If I recall, when using the annotation it would just return an array of variables.
deguif commented 1 year ago

Maybe I misunderstood your reply but @Template was not merged recently. What was merged here is the use of the twig engine by default, but it has left the use of the annotation untouched

deguif commented 1 year ago

@gnat42 the controller is indeed returning a response since this pr, so you can ignore the annotation globally safely and the response should be returned

gnat42 commented 1 year ago

Right, so this pr removed the requirement for the bundle that provides the template annotation, and the controllers were changed to properly return a response. As such, removing the inclusion of the template annotation in the controller IMO is safe and not a BC, its a broken artifact of the incomplete PR that was merged that didn't remove the annotation.