SettingsBundle icon indicating copy to clipboard operation
SettingsBundle copied to clipboard

Issue #124 : Symfony6 feature

Open Alexandre-T opened this issue 2 years ago • 6 comments

Do NOT merge this PR into master!

@rvanlaak , I'm trying to prepare a new version of your awesome bundle. A version only for symfony 5.4.|^6.0 and Php 7.4.|^8.0 .

The bundle now works fine with my Symfony 6.1 application, but I still have problem with the functional test. I already tried to upgrade from nyholm/symfony-bundle-test:1 to nyholm/symfony-bundle-test:2 . But I'm not able to detect the services provided by your bundle.

Any help is welcomed!

Alexandre-T avatar Aug 04 '22 12:08 Alexandre-T

Hi @Alexandre-T thanks for your work on this. As the old version of this bundle works fine for lower versions, I think it's fine to release a v4 (or v3, as only betas were released?) that bumps all requirements to package versions that do not require us to keep the BC layers.

So, what about bumping to PHP >=8.0 and Symfony >= 5.4 ?

rvanlaak avatar Aug 04 '22 19:08 rvanlaak

I do agree. I'm already removing PHP7.4 and fixing some potential issues with PhpStan

Alexandre-T avatar Aug 05 '22 10:08 Alexandre-T

I'm currently fixing the errors detected by php-stan.

As we prepare a new version, what about to fix parameters and returns with no typehint specified?

The most impacted interface is the serializer itself.

interface SerializerInterface
{
    public function serialize(mixed $data): string;

    public function unserialize(string $serialized): mixed;
}

But I guess impacts won't be so important, because this bundle already provides two internal serializers.

Do you agree with such changes?

(Sorry, I'm not so fluent in English.)

Alexandre-T avatar Aug 05 '22 14:08 Alexandre-T

Hi @Alexandre-T , yes feel free to improve the internal interface, as this will be a major bump for everything anyways..

rvanlaak avatar Aug 17 '22 11:08 rvanlaak

I made a PR that could fix the remaining test problems

Chris53897 avatar Feb 07 '23 09:02 Chris53897

@Chris53897 can you also file a PR for this repo?

rvanlaak avatar Feb 07 '23 12:02 rvanlaak