Update method signature for CacheInvalidation.php

[netwarex]

Fixes #528

[dbu]

hi @netwarex , thanks for looking into this!

looks like this is quite the BC break that symfony dropped onto us here... i think we can not be compatible with both symfony < 6 and >= 6 at the same time. and changing the signature on our side also is a BC break.

probably we need to do an ugly thing again: wrap the whole class in an if/else checking for Kernel::MAJOR_VERSION, like we do here: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/91f5e625e0b8c5e667a43849a44a7f34042b6aab/src/Security/Http/Logout/ContextInvalidationSessionLogoutHandler.php (but we should also check for class_exists Kernel and if it does not exist go with the old version, to avoid messing up things that scan the code. the symfony kernel is an optional dependency for the FOSHttpCache)

we should then note in the changelog that for symfony 6, you need to adjust to the symfony BC break.

i think for the implementations of the interface that we have in the tests, we can go with the more restrictive thing, as that is allowed for extending classes.

can you try to adjust the PR accordingly?

[netwarex]

can you try to adjust the PR accordingly?

Hi @dbu ,
Sure! I have changed to make it backwards compatible. Can you please review my changes, and release if it's passed? :) Thank you!

[dbu]

thanks! i only have a few nitpicks, but i think this is correct. i will have a look whats up with the build, quite a few jobs fail but it does not look related to your changes.

could you also write the changelog entry to explain that when implementing the CacheInvalidation interface, one will need to be aware of the Symfony BC break?

[dbu]

i fixed the builds except for legacy varnish 5 in #530, can you please rebase on master?

[dbu]

dang, i messed up the phpstan src job. can you please rebase again?

and can you please check the failures other than " CI / PHP 7.4 Legacy Varnish 5"? varnish 5 has a known problem to install.

there are some cs fixes necessary, and somehow phpstan seems to choke on the class_alias construction - if you find no solution for that, i am ok to add it to phpstan ignore rule or if that is not possible to have the ci job copy the legacy interface over the selection file and remove the new interface for now.

[netwarex]

Okay, I force pushed my master for now, doesn't matter for the fork, but it's properly rebased then :) Please review

PS: Please add ignore for those errors, as I have no idea currently to fix that, and can be done in a new PR (would be nice to have this released :))

[dbu]

thanks. i will quickly check with phpstan if they have an idea how to handle this.

the codestyle check is broken by your changes, can you please adjust these things:
https://github.com/FriendsOfSymfony/FOSHttpCache/runs/7198550603?check_suite_focus=true

[dbu]

phpstan/phpstan#7582

[dbu]

the varnish 5 build is fixed in master now, thanks to quick help from the varnish module maintainers.

[dbu]

for phpstan: we need to configure the alias in a bootstrap file: https://phpstan.org/user-guide/discovering-symbols#class-aliases

i suggest that we alias to legacy if php < 8.1 and the new one otherwise. that way the build will stay ok when the phpstan task updates to php 8.1 and installs symfony 6.

can you do that or do you want me to take over?

[dbu]

yay, its getting green!

can we please move the config file into a subfolder (also to make the context of the file clear)? i think then we are ready to merge.

[netwarex]

can we please move the config file into a subfolder (also to make the context of the file clear)? i think then we are ready to merge.

Sure thing 👍 Can we release the changes today, please?

[dbu]

yay, looks good now. thanks a lot. i will tag a release right after merging.

[dbu]

https://github.com/FriendsOfSymfony/FOSHttpCache/releases/tag/2.14.0