Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #124 : Symfony6 feature #126

Open
wants to merge 8 commits into
base: 3.x
Choose a base branch
from

Conversation

Alexandre-T
Copy link
Contributor

@Alexandre-T Alexandre-T commented Aug 4, 2022

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!

Symfony 3.4, 4.4, 5.3- removed
* Upgrading nyholm/symfony-bundle-test
* Fixing PhpStan configuration
@Alexandre-T Alexandre-T changed the title Symfony6 feature Symfony6 feature for #124 Aug 4, 2022
@Alexandre-T Alexandre-T changed the title Symfony6 feature for #124 Issue #124 : Symfony6 feature Aug 4, 2022
@rvanlaak
Copy link
Owner

rvanlaak commented Aug 4, 2022

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 ?

@Alexandre-T
Copy link
Contributor Author

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

Removing psr/cache:2 and less
@Alexandre-T
Copy link
Contributor Author

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.)

@rvanlaak
Copy link
Owner

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

$cachedSettingsManager->expects($this->at(1))
->method('invalidateCache')
->with($this->equalTo(null), $this->equalTo($owner))
->withConsecutive(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

->at() and ->withConsecutive() are both not supported in PhpUnit 10 ;(
Maybe there is a trait to solve this. Or later in a seperate PR

@Chris53897
Copy link

I made a PR that could fix the remaining test problems

@rvanlaak
Copy link
Owner

rvanlaak commented Feb 7, 2023

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

@Chris53897 Chris53897 mentioned this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants