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

chore: add phpstan to CI #622

Merged
merged 3 commits into from
May 3, 2024
Merged

chore: add phpstan to CI #622

merged 3 commits into from
May 3, 2024

Conversation

usu
Copy link
Contributor

@usu usu commented May 2, 2024

  • Adds phpstan to CI (didn't test it - hope it goes through 😄). Yaml is copied from FOSHttpCache
  • Fixes all level-1 errors, except one remaining error which will be fixed by Remove deprecated code #618
  • After that, we can fix and increase higher levels by separate PRs

@usu
Copy link
Contributor Author

usu commented May 2, 2024

The new workflow "static" triggers quite a lot of errors "unknown class", which is due to references to dependencies which are dev-dependencies.
I could easily fix this by adding a REQUIRE_DEV: "true" to the phpstan-src job. But I wonder if this is correct, or whether all these dependencies should actually be non-dev dependencies...

@usu usu mentioned this pull request May 2, 2024
@dbu
Copy link
Contributor

dbu commented May 3, 2024

thanks for looking into this.

dang. good old "optional dependencies" saying hello :-|

i am not sure how optional they really are, a couple places looks to me like it would simply error if thats not available. lets move console and security core & http to the requirements. i would assume most symfony applications use those anyways. and require twig like you require jean-beru - twig really is an optional integration.

@usu
Copy link
Contributor Author

usu commented May 3, 2024

Fixed as suggested & ready to merge.

Actually console is optional, the services are only loaded when Symfony\Component\Console\Application is available:
https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/2.x/src/DependencyInjection/FOSHttpCacheExtension.php#L105
https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/2.x/src/DependencyInjection/FOSHttpCacheExtension.php#L541

However, twig is currently not treated optional. I will fix this in a separate PR.

@dbu
Copy link
Contributor

dbu commented May 3, 2024

the remaining failure is for ContextInvalidationLogoutHandler which was symfony 5 only and is being deleted in #618

i merge this as-is with the phpstan failure. thanks for the help!

@dbu dbu merged commit a850861 into FriendsOfSymfony:3.x May 3, 2024
11 of 12 checks passed
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