CacheableDependencyInterface::getCacheTags should return a list<string> for compatibility with Cache::mergeTags

Assuming this will fail with a bunch of PHPStan issues but setting to NR to get an idea if this is something we can do.

I think we need level 3 before we see phpstan issues.

Level 2: https://phpstan.org/r/98f1fbe4-0a95-4f4e-bd29-d8869033c9ea
Level 3: https://phpstan.org/r/a58dfaa7-b313-43ce-8bff-8c60afde83e3

Awesome! That means this should be easy to get in :) Pipeline's green.

I set phpstan to level 3 and generated the baseline. Then applied this patch and re-generated again. There was only one difference and it was just rearranging the wording of one violation.

Then I repeated the same steps on level 8 to see if we're not creating and masking more issues.

The diff mostly shows places where we were calling Cache::mergeTags with an array instead of a list, mostly due to RefinableCacheableDependencyTrait::addCacheTags.

It does, however, show that we probably also need to update the param doc for CacheableMetadata::setCacheTags, as that allows setting the property to an array that is not a list.

It also doesn't like \Drupal\Core\Entity\EntityBase::getCacheTags returning ::getCacheTagsToInvalidate as once again that is not necessarily a list.

There are several more problems in various places, but I haven't combed through them all. Attaching the diff for perusal.

Adding related coding standards issue since it will inevitably come up. Don't think we should wait for it though, especially since we already use this in \Drupal\Core\Cache\Cache::mergeTags.

So I guess we have 2 options:

1. Smaller scope, accept more potential errors far down the line when we eventually get to level 8
2. Wider scope, try to fix these issues before they have the chance to appear.

I like the idea of going with 2, but then the scope of this issue might get a lot bigger and have even more flow on effects. Also finding those issues sounds a bit time consuming.

2 more spots to fix identified in #7:
- EntityInterface::getCacheTagsToInvalidate
- RefinableCacheableDependencyInterface::addCacheTags

There's also \Drupal\Core\Plugin\DefaultPluginManager::$cacheTags and \Drupal\Tests\Core\Render\TestCacheableDependency::$tags that are simple arrays.

For the record, the reason I wanted to test with a higher phpstan level is to determine if there is somewhere in core that is using something other than a list. But upon further consideration it probably doesn't matter, because the changes were initially made to satisfy phpunit, and these changes are to satisfy phpstan.

Most likely it will be contrib and custom projects that are affected here, as they're more likely to run on higher levels of phpstan.

when we eventually get to level 8

Pretty optimistic lol

Change seems pretty straightforward

Committed/pushed to 11.x, thanks!

catch → closed merge request !11160

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