search

schmittjoh / JMSTranslationBundle

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

Remove extending from ContainerAwareCommand #529

Closed franmomu closed 4 years ago

franmomu commented 4 years ago
Q A
Bug fix? no
New feature? no
BC breaks? I guess so
Deprecations? no
Tests pass? yes
Fixed tickets https://github.com/schmittjoh/JMSTranslationBundle/pull/515
License Apache2

Description

I removed extending from ContainerAwareCommand by extending from Command and adding getContainer and setContainer methods and also calling setContainer from the service declaration. I think it is BC, but maybe I'm missing something.

goetas commented 4 years ago

I think that we can consider this as not-BC. People should not have extended those command IMO. @gnat42 what do you think?

Can we remove completly the setContainer/getContainer methods?

goetas commented 4 years ago

i've forgot about it, but did you check https://github.com/schmittjoh/JMSTranslationBundle/pull/515/files ?

franmomu commented 4 years ago

i've forgot about it, but did you check https://github.com/schmittjoh/JMSTranslationBundle/pull/515/files ?

Yes, It does the same thing but with the container and using kernel.project_dir

gnat42 commented 4 years ago

Hey @goetas, I think its fine to remove that. Extending the commands makes no sense so even if it was a break, I kinda feel like its one that is ok.

goetas commented 4 years ago

Thanks for your work