WMDE-leszek added a comment.
As I was explicitly called out, a few answers, plus an opinion at the end. > The weird code was added in patchset 20 in July 2018, apparently by @WMDE-leszek: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/441203/19..20/lib/includes/SimpleCacheWithBagOStuff.php. The commit message was edited in the same patchset. It mentions 3 sources, but I can't find `utf8_…` mentioned on these pages. > > I tried to read the discussion, but it's not very helpful. Unfortunately I wasn't involved back then. There is also no Phabricator task linked. > > What's done in the code directly after the weird `utf8_encode` is a `hash_hmac` call. Could it be that this is not able to work with UTF-8 strings, but expects something else? Looking at the change history I can recall I pair-programmed parts of this change with Aleksey. I didn't remember the exact reason for adding utf8_encode and _decode. But thankfully there are tests which help to understand it. Test in question is seemingly `Wikibase\Lib\Tests\SimpleCacheTestCase::testBinaryData` Role of `utf8_encode` is apparently not about dealing with UTF-8 strings, but to ensure that `$dataToStore` in `return json_encode( [ $signature, $dataToStore ] );` (`Wikibase\Lib\SimpleCacheWithBagOStuff::serialize`) is UTF-8 encoded. As per PHP documentation in case it was not, json_encode would return false, leading to incorrect value being stored in cache. It might be, which maybe Lucas implies, that given the current usage of this cache class (I recall it was introduced to cache labels and descriptions of items, those could be maybe assumed to be always UTF-8-encoded) this situation might be impossible in practice. Whether this warrants removal of the encoding in the cache class, I am not sure. > What is `SimpleCacheWithBagOStuff` even used for? It is a PSR-16-compliant implementation `CacheInterface` implementation wrapping Mediawiki's "BagOStuff", as per consquence of Decision to "Use PSR-16 PHP Cache Interface in Wikibase" <https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Wikibase/+/refs/heads/master/docs/adr/0001-use-psr-16-cache-interface.md> (instead of binding to Mediawiki's less univeral interfaces or other abstractions) Originally, `SimpleCacheWithBagOStuff` was introduced to use cache instead of secondary SQL table for efficient fetching of item label and description data to be used when displaying "references" to an item <https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Wikibase/+/refs/heads/master/docs/adr/0000-use-cache-instead-wb_terms-for-data-needed-for-display.md>. It could be it has been also adopted for other use cases. > My suggestion is to not touch this class, but mark it as deprecated and replace all usages with something reliable from core. I note that the latter part of the suggestion goes against the quoted above decision to use more common cache abstraction. Finally, I'd like to address the above remarks of possibly introducing some fallback cache key generation algorithm to not change keys while changing the logic in the cache class. I'd think it would be sensible to provide some data, if it was actually intended to introduce some "smart" logic to keep cache keys unchanged. Without understanding the suggested impact it does seem like unnecessary complexity to me. I'd naively think if such a detail like changing the cache key generation algorithm (I guess equaling temporary dropping of cache) can take Wikidata down, we might have bigger problems than what is being discussed here. I admit I don't remember details of rollout of the cache approach (instead of the secondary SQL table) but I don't recall any spectacular outages on the way. TASK DETAIL https://phabricator.wikimedia.org/T324202 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: WMDE-leszek Cc: WMDE-leszek, thiemowmde, Paladox, Michael, ItamarWMDE, Aklapper, Lucas_Werkmeister_WMDE, Danny_Benjafield_WMDE, Isabelladantes1983, Themindcoder, Adamm71, Jersione, Hellket777, LisafBia6531, Astuthiodit_1, malberts, 786, Biggs657, karapayneWMDE, Invadibot, maantietaja, Juan90264, Alter-paule, Beast1978, Un1tY, Akuckartz, Hook696, darthmon_wmde, Kent7301, joker88john, CucyNoiD, Nandana, Gaboe420, Giuliamocci, Cpaulf30, Lahi, Gq86, Af420, Bsandipan, GoranSMilovanovic, TK-999, QZanden, LawExplorer, Lewizho99, Maathavan, _jensen, rosalieper, Neuronton, Scott_WUaS, Wikidata-bugs, aude, Lydia_Pintscher, Jdforrester-WMF, Mbch331
_______________________________________________ Wikidata-bugs mailing list -- [email protected] To unsubscribe send an email to [email protected]
