Skip to content

Fix phpstan variable visibility (with package update for Symfony 8)#190

Open
victormacko wants to merge 5 commits intonorkunas:masterfrom
victormacko:fix-phpstan
Open

Fix phpstan variable visibility (with package update for Symfony 8)#190
victormacko wants to merge 5 commits intonorkunas:masterfrom
victormacko:fix-phpstan

Conversation

@victormacko
Copy link

This PR is in addition to #189 to address PHPStan's issue with the variable visibility in a few of the classes which are marked as 'final' - it shouldn't have any effect.
I've done a basic test (sending a notification) which worked without issue.

@victormacko
Copy link
Author

@norkunas This is the PR i've no idea on how to fix up ... do you have any suggestions so I can sort it?

@norkunas
Copy link
Owner

norkunas commented Mar 3, 2026

@victormacko based on:

Problem 2
- Root composer.json requires symfony/http-client ^5.0|^6.2|^7.0|^8, found symfony/http-client[v5.0.0, ..., v5.4.49, v6.2.0, ..., v6.4.34, v7.0.0, ..., v7.4.6, v8.0.0, ..., v8.0.6] but these were not loaded, because they are affected by security advisories ("�]8;;[https://symfony.com/cve-2024-50342�\PKSA-4k7v-pfvw-nqvp�]8;;�](https://symfony.com/cve-2024-50342%1B/PKSA-4k7v-pfvw-nqvp%1B]8;;%1B/)"). Go to https://packagist.org/security-advisories/ to find advisory details. To ignore the advisories, add them to the audit "ignore" config. To turn the feature off entirely, you can set "block-insecure" to false in your "audit" config.

i guess we need to bump dependencies to non-vulnerable versions

@victormacko
Copy link
Author

@norkunas i've tried adding a conflict to avoid composer installing it, but it's still failing here...

Are you able to assist? ... i've always been under the impression that it's better to have developers manage CVS's themselves - ie. by installing roave/security-advisories / some other security checker, so this is all new to me to manage it within packages like this.

@norkunas
Copy link
Owner

norkunas commented Mar 3, 2026

@norkunas i've tried adding a conflict to avoid composer installing it, but it's still failing here...

Are you able to assist? ... i've always been under the impression that it's better to have developers manage CVS's themselves - ie. by installing roave/security-advisories / some other security checker, so this is all new to me to manage it within packages like this.

Well it's a composer feature now :) For the future - we should add to the composer options in CI this flag: --no-security-blocking.

But still, I think we should bump at least to non-vulnerable dependencies.

symfony/http-client -> ^5.4.47|^6.4.15|^7.1.8|^8.0

as for the symfony/options-resolver, it's not vulnerable, so ^8.0 can be added and that's all.

And then there is phpunit-bridge ... you can see I already tried #183
But the phpunit itself got better in the newer versions, so I think it's time to remove bridge and use phpunit directly

@victormacko
Copy link
Author

victormacko commented Mar 3, 2026

@norkunas ok thanks, i've added that into composer (the versions). I've no idea how the CI side of things works to add that flag in, or change which phpunit is used -- is that something set within the code, or elsewhere in GitHub?

it's just editing the workflow in ./github/workflows

@victormacko
Copy link
Author

@norkunas Has that ever worked in the past?
I've spent a chunk of time on it but still scratching my head on how it's something which can work... any insight you have would be ace on what needs to be fixed on it.

@norkunas
Copy link
Owner

norkunas commented Mar 3, 2026

@norkunas Has that ever worked in the past? I've spent a chunk of time on it but still scratching my head on how it's something which can work... any insight you have would be ace on what needs to be fixed on it.

of course it was :) you can see https://github.com/geocoder-php/BazingaGeocoderBundle/ for inspiration, where compatibility is made with many symfony versions. but i think it's time to retire 5.4 here. it simplifies maintenance a lot, and then of course php would need to be bumped up to 8.1

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.

2 participants