Redirect response should get cache tags from overridden config

Related but not same: #3196381: Forced login missing cache metadata → .

Let's see if anything wrong with this direction, then I can add tests.

donquixote → closed merge request !47

The fix and tests look good.

I have a question, see the MR

Either we implement a full deprecation path, or we keep it as it is and remove the @todo

A full deprecation path would look like:

1. Remove the initial property value assignation.
2. CasRedirectData to implement RefinableCacheableDependencyInterface and use RefinableCacheableDependencyTrait
3. Remove all properties and methods already inherited from trait
4. Add config object as 3rd param in CasRedirectData::__construct() which will default to NULL
5. In CasRedirectData::__construct() check if config object has been passed. If not set it to \Drupal::configFactory()->get('cas.settings') and trigger deprecation (see https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → )
6. In CasRedirectData::__construct(), add config as cache dependency of CasRedirectData
7. In redirector add $data (CasRedirectData) as cache dependency

But I'm OK also to keep the property as it is, but then remove the TODO

I updated the comment, remove the todo.

But to recap my point from our conversation:
We should add cache metadata for dependencies when we actually use them.

In some cases this means adding the same dependency twice in two different methods, so that if we remove one of them, the other is still there.

On the other hand, in general we should not add cache metadata for something that we think will be used elsewhere.
This does not cause much harm for a config object, but it breaks the pattern and can lead people astray, which now rely on that cache metadata being added elsewhere.
Here we are already in that scenario, where we now have to support those hypothetical people.

For this reason it seems wrong to add config cache metadata to CasRedirectData, if nothing in that object really depends on that config.
E.g. there is nothing you could update in the config that would lead to different outward behavior of CasRedirectData.
Of course this can change if an event subscriber adds some data to CasRedirectData that is dependent on that config. Also because the config is made available in the event object. Normally that event subscriber would then have to add the config cache metadata to the CasRedirectData.

CasRedirectData to implement RefinableCacheableDependencyInterface and use RefinableCacheableDependencyTrait

This actually seems like a good idea.
However, perhaps it is better to rethink the architecture for cas 4.x, maybe a lot of it becomes obsolete.
But if this can be done BC-friendly for 3.x, sure.

Fair enough. Thank you for catching & fixing.

Thank you all

Fixing credits

Automatically closed - issue fixed for 2 weeks with no activity.