KnpLabs / DictionaryBundle

Are you often tired to repeat static choices like gender or civility in your apps ?
MIT License
86 stars 24 forks source link

Allow Symfony 6.3 #192

Closed tacman closed 1 year ago

tacman commented 1 year ago

Symfony 6.3 is in beta. I've submitted a PR here, can someone approve it please?

tacman commented 1 year ago

@PedroTroller can you please approve the PR so that this can be installed with Symfony 6.3? Thanks.

tacman commented 1 year ago

@AntoineLelaisant or @PedroTroller , can you allow Symfony 6.3 please?

tacman commented 1 year ago

hello, @AntoineLelaisant or @PedroTroller ? Symfony 6.3 is in beta 2 now. It's a one-line change. Please?

There's a lot of code to handle an old version of the symfony validator, and should probably be refactored. But in this case, it's simple not blocking Symfony 6.3. I'm wondering if I should add Symfony 6.4 and 7.0 now too, as it's likely they'll work out of the box. And if not, then we can add something to block people from installing this. But it seems a bit backwards to by default block all new versions of Symfony without an explicit case statement.

Do you mind if I submit my fork to packagist with this change?

alexpozzi commented 1 year ago

Hello, I'm not a maintainer of this library but, in my opinion, supporting a beta version doesn't make sense and it would be better to wait for it to be released before officially supporting it (even more with a major version). You are free to do what you want on your side, you can also just use your fork as a composer's VCS repository temporarily and switch back to the original package once 6.3 will be supported.

tacman commented 1 year ago

Supporting a beta version doesn't make sense? The whole purpose of beta is to allow library developers to test their libraries against newer versions. This bundle has one of the most restrictive policies on beta I've ever worked with -- by default, new versions are excluded from working.

Please reconsider this strategy.

Yes, I realize I can fork the bundle, add my one-line change that simply allows 6.3, then change all my projects that use this bundle to point to my fork, then revert back. But that's a lot of work for a trivial change.

If you're going to keep this restrictive policy, why not lock it in composer.json, e.g. get rid of ^6.0 and make it <6.3? The answer, I assume, is because that would be unnecessarily restrictive. But the code is similarly unnecessarily restrictive.

switch ($version = substr((string) InstalledVersions::getVersion('symfony/validator'), 0, 3)) {
    default:
        throw new Exception('knplabs/dictionary-bundle is not compatible with the current version of symfony/validator: '.$version);

    case '6.2':
    case '6.1':
    case '6.0':
        trait SymfonyCompatibilityTrait
alexpozzi commented 1 year ago

Again, I'm not the library maintainer and I don't know why that check has been added but, I can agree with you that rather the composer.json, rather this switch, should be adapted to be consistent with each others. Again (x2) you are free to do whatever you want with your own code, in my opinion, as a library maintainer, better being restrictive and open up when sure of being able to support 100% an external dependency rather than opening up by default and hope that things will work out of the box. I guess we have different opinion on the topic.

PedroTroller commented 1 year ago

@tacman sorry, I've been off for a few weeks

PedroTroller commented 1 year ago

@tacman I'll integrate your PR to the next release and try to release it this week.