schmittjoh / JMSTranslationBundle

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

Remove dependency on JMSDiExtraBundle #464

Closed tommygnr closed 6 years ago

tommygnr commented 7 years ago
Q A
Bug fix? no
New feature? no
BC breaks? yes (extremely minor, requires removing two lines from AppKernel for some users)
Deprecations? no
Tests pass? yes
Fixed tickets closes #355, closes #411, closes #457
License Apache2

Description

This is an updated version of https://github.com/schmittjoh/JMSTranslationBundle/pull/411. I have rebased in on to the latest master, updated it to get the tests passing and updated the docs and changelog as appropriate.

The BC break is only going to affect users that have added the DiExtraBundle and AopBundle to their AppKernel file. We can avoid it altogether by continuing to keep the DiExtraBundle as a dependency in composer.json until a 2.0 release. It's an extremely trivial BC break that will be notified to users when the post install scripts are executed during composer update so I'm inclined to say it can be merged as is.

Todos

tommygnr commented 6 years ago

ping @Nyholm

tristanbes commented 6 years ago

This would mean bump dependency for this bundle with Symfony >= 3.3

greg0ire commented 6 years ago

ping @gnat42 @Nyholm

mvrhov commented 6 years ago

This is long overdue. and also blocking the upgrade to 4.0

tommygnr commented 6 years ago

@tristanbes can you explain why this would require symfony >= 3.3? It passes all tests with symfony 2.7

tommygnr commented 6 years ago

@tristanbes have you seen ^ I don't see how this requires symfony 3.3

tristanbes commented 6 years ago

I can't remember why I wrote this, so you can ignore my comment :-)

tristanbes commented 6 years ago

get this merged for mental health please, I can't bear adding JMS AOP and JMS ExtraBundle dependencies just for translating stuff :D. Using this PR for other projects, and working well. 😋

gnat42 commented 6 years ago

look - I would have no problem removing the deps, however I'm not the owner of the project. I just signed up years ago to help deal with some requests for a limited time. The owner of the project wasn't interested in much refactoring/deep work. Thus a new group/project has been setup. I am uncomfortable making the requested change because of these reasons. I do fully understand the frustrations. At a minimum feel free to fork the project and continue development there if you don't want to change bundles. There are lots of PRs you could look at incorporating.

Apologies if I owned this bundle/code I'd feel more comfortable doing bigger PRs like this.

mvrhov commented 6 years ago

@gnat42 Now that that's clear. Let's be honest. The owner is clearly not interested anymore. But you have the power to merge this so IMO you should.

edit: I know about the translation project I've done a few PR there but it's still not comparable with JMS. At least setting default translations from Desc annotations/filters is missing. Then there is the thing that it writes the path from where the translations were extracted. We waited for the ability to disable this in JMS bundle for ages.

gnat42 commented 6 years ago

This is only partially true. Yes I can commit it, however I signed up to maintain this bundle for a sum total of 6 months over 2 years ago. I don't know the code that well, I barely use the bundle anymore. Committing PRs here and there without actually being a maintainer (in reality not just name) is irresponsible. This is the downside of OSS, the upside is if anyone wants to take over, they are welcome to by forking. Or you could request commit rights / ownership from @schmittjoh.

Without actually knowing the code like I do bundles I actually maintain I don't want to keep merging PRs here and there. Sorry about that.

schmittjoh commented 6 years ago

I think it makes sense to remove the dependency. The main part I was against, is making a lot of different sub-projects out of this one repository as it would yet even more increase the maintenance burden.