search

schmittjoh / JMSTranslationBundle

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

Drop symfony < 3.4 and < 4.3 for 4.x #520

Closed franmomu closed 4 years ago

franmomu commented 4 years ago
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets https://github.com/schmittjoh/JMSTranslationBundle/pull/496, https://github.com/schmittjoh/JMSTranslationBundle/pull/482, https://github.com/schmittjoh/JMSTranslationBundle/pull/495, https://github.com/schmittjoh/JMSTranslationBundle/pull/481, https://github.com/schmittjoh/JMSTranslationBundle/issues/511
License Apache2

Description

EDIT: I've removed some changes I had made (cleaning up) so there are fewer changes and those changes can be applied later with a CS Fixer.

goetas commented 4 years ago

@franmomu Thanks for your PR! Looks good, but will take some time to be reviewed.

@schmittjoh @gnat42 I'm fine with dropping support for old versions and doing some cleanup... what about you?

franmomu commented 4 years ago

@franmomu Thanks for your PR! Looks good, but will take some time to be reviewed.

Sure! Let me write some comments in some of the changes to make it easier.

gnat42 commented 4 years ago

It makes sense at this point to start dropping some of those. I haven't reviewed the PR closely though I did skim through the changes and it looked fine to me. However I'd suggest that if this is done you perhaps make it a major release number bump due to the break.

goetas commented 4 years ago

@gnat42 I do not see bc breaks, can you point me to it? (dropping supported version should not be considered as a BC break, see this blog post from the doctrine team https://www.doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html)

gnat42 commented 4 years ago

@goetas Interesting. I can see their point. The only reason I disagree (but ultimately don't care which way you go with this), is semantic version not only helps the computer know what version and allows you to stick to a particular one, but it also signals people that something has changed. True if you bump the minimum up it doesn't break and composer will ignore new versions but then people will be hmm why do I have x.y.z and and not x.y.z2... and have to dig a bit. A major version bump tells you something has changed without digging is all. Feel free to merge and release whatever version you like.

franmomu commented 4 years ago

If there are not BC breaks I would release a minor version. To be compatible with Symfony 5 there are some changes that won't be BC like removing ContainerAwareCommand from commands, so when that time comes it should be major version release.

goetas commented 4 years ago

@franmomu can you please rebase your pr?