CacheKeyHelper::getKeyForPage (only method in the class) is not wiki-aware:
public static function getKeyForPage( $page ): string { return 'ns' . $page->getNamespace() . ':' . $page->getDBkey(); }
Two pages with the same namespace and DB key but on different wikis will share the same cache key.
Ah, now that I read the doc comment more carefully:
We type against interfaces like PageReference and LinkTarget, and for caching to work, all implementations would have to derive cache keys in the exact same way.
This requirement is okay to have, but then the method should forbid passing LinkTarget with an interwiki prefix, or PageReference with a non-local page, for example by throwing an exception. Otherwise there is no safe way to use the same key for LinkTarget and PageReference. Still though, I imagine this would cause issues if the cache key is used with makeGlobalKey, as the concept of "local" depends on the wiki where the expression is evaluated. I don't think there's an easy solution to that though, if we can't add further disambiguators to the cache key.
I'm not sure what the best course of action would be here, but a breaking change is needed either way. The method in its current form seems dangerous. Tentatively tagging MW Platform as this class was introduced for T278459.
@JTweed-WMF could you elaborate on the reasoning to move this to MediaWiki-Engineering?
It seems that the component belongs to the Page/Revision store components which are owned by the Platform Team, if I'm not mistaken.
When we triaged it as a team, we wondered if it was actually the Content Transform team. We don't own those components, I think they are officially un-owned.
CTT would be a good choice if this was about the key used by the parser cache. But this is a generic utility class used across the code base for various things that need a cache key that depends on the page.
@Daimona can you shed some light on why you filed this ticket? What's the problem? Did the fact that the helper class is not wiki-aware cause a problem somewhere?
What's the ask? Replace the static helper with a service class that is wiki-aware? Or should the static helper code call getWikiId() and include the result in the cache key?
The latter would be trivial to add. The former would be more tricky - in that case, I'd probably rather use BagOStuff::makeKey to make the key unique across wikis.
It didn't cause a problem because I read the implementation before using the helper. I was going to use it as a cache key generator of PageIdentity object, as I remembered that such a class existed. Said PageIdentity's can belong to a different wiki in my code, so the helper would have caused potential collisions (two homonym pages on different wikis ending under the same cache key). I had no reason to suspect that this would've been the case by just reading the docs, I had to actually double-check the implementation. (For completeness: eventually I went for simply PageIdentity::__toString())
What's the ask? Replace the static helper with a service class that is wiki-aware? Or should the static helper code call getWikiId() and include the result in the cache key?
I don't have a specific ask because I don't have enough context to know why this helper exists and how it was meant to be used. My first thought was that yes, it could just add getWikiId to the cache key, but as noted in T405920#11225009, I'm not sure how that would work as to interoperability with LinkTarget objects. Likewise if the LinkTarget has an interwiki prefix. Maybe it's fine, as it's "just" a cache split. But then again, I'm not familiar with the intended usage of the helper so it's hard to say. Maybe forbidding non-local pages is also fine, and would also prevent the subtle failure mode.
in that case, I'd probably rather use BagOStuff::makeKey to make the key unique across wikis.
The other remark I had above is that the caller can decide how to generate the cache key, i.e., whether to use makeKey or makeGlobalKey. This is not something the helper has control over. However, the helper makes no guarantee that the cache keys it generates are (or aren't) suitable for makeGlobalKey. In fact, they aren't, because the wiki ID is not part of the cache key. This could also be resolved via documentation only.
Change #1193077 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):
[mediawiki/core@master] CacheKeyHelper: Use wiki ID and interwiki prefix if present
Change #1193798 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] Fix and compact code and PHPDocs in SearchHandlerTest
Change #1193798 merged by jenkins-bot:
[mediawiki/core@master] Fix and compact code and PHPDocs in SearchHandlerTest
Change #1193077 merged by jenkins-bot:
[mediawiki/core@master] CacheKeyHelper: Use wiki ID and interwiki prefix if present