As of #3468502: sebastianbergmann/comparator:5.0.2 Introduces (for Drupal) breaking changes Cache::mergeTags must be passed a list<string> of cache tags. This means if you're using a common pattern where you override getCacheTags in a block, entity type, etc, and call mergeTags from the parent, you need to run it through array_values. This can get very repetitive.
Implement an entity type or block plugin and override getCacheTags with the following code:
public function getCacheTags() {
return Cache::mergeTags(parent::getCacheTags(), ['some:tag']);
}
PHPStan will now complain:
Parameter #1 ...$cache_tags of static method Drupal\Core\Cache\Cache::mergeTags() expects list<string>,
array<string> given.
💡 array<string> might not be a list.
The fix is to wrap parent::getCacheTags in array_values()
Change return type of CacheableDependencyInterface::getCacheTags to list<string>
Agree
None
None
Changed return type of CacheableDependencyInterface::getCacheTags
None
TBC
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | baseline-diff.txt | 40.91 KB | mstrelan |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
acbramley commentedComment #4
acbramley commentedAssuming this will fail with a bunch of PHPStan issues but setting to NR to get an idea if this is something we can do.
Comment #5
mstrelan commentedI 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
Comment #6
acbramley commentedAwesome! That means this should be easy to get in :) Pipeline's green.
Comment #7
mstrelan commentedI 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::mergeTagswith an array instead of a list, mostly due toRefinableCacheableDependencyTrait::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::getCacheTagsreturning::getCacheTagsToInvalidateas 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.Comment #8
acbramley commentedSo 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
Comment #9
mstrelan commentedThere's also
\Drupal\Core\Plugin\DefaultPluginManager::$cacheTagsand\Drupal\Tests\Core\Render\TestCacheableDependency::$tagsthat 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.
Comment #10
acbramley commentedComment #11
smustgrave commentedPretty optimistic lol
Change seems pretty straightforward
Comment #13
catchCommitted/pushed to 11.x, thanks!
Comment #14
catch