Unknown placeholder: entityViewPlaceholder-entitytermsview-entitytermsforlanguagelistview-class
Closed, ResolvedPublicPRODUCTION ERROR
Actions

Assigned To

Authored By

	Michael
	Jul 25 2019, 10:31 AM

Description

Error

Request URL: https://test.wikidata.org/wiki/Q10
Request ID: XTmAmApAMFAAALO0MdYAAACH

message

Unknown placeholder: entityViewPlaceholder-entitytermsview-entitytermsforlanguagelistview-class

trace

#0 [internal function]: Wikibase\Repo\ParserOutput\PlaceholderExpander\ExternallyRenderedEntityViewPlaceholderExpander->getHtmlForPlaceholder(string)
#1 /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/ParserOutput/TextInjector.php(80): call_user_func_array(array, array)
#2 /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/Hooks/OutputPageBeforeHTMLHookHandler.php(187): Wikibase\Repo\ParserOutput\TextInjector->inject(string, array)
#3 /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/Hooks/OutputPageBeforeHTMLHookHandler.php(160): Wikibase\Repo\Hooks\OutputPageBeforeHTMLHookHandler->replacePlaceholders(OutputPage, string)
#4 /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/Hooks/OutputPageBeforeHTMLHookHandler.php(148): Wikibase\Repo\Hooks\OutputPageBeforeHTMLHookHandler->doOutputPageBeforeHTML(OutputPage, string)
#5 /srv/mediawiki/php-1.34.0-wmf.15/includes/Hooks.php(174): Wikibase\Repo\Hooks\OutputPageBeforeHTMLHookHandler::onOutputPageBeforeHTML(OutputPage, string)
#6 /srv/mediawiki/php-1.34.0-wmf.15/includes/Hooks.php(234): Hooks::callHook(string, array, array, NULL, string)
#7 /srv/mediawiki/php-1.34.0-wmf.15/includes/OutputPage.php(1938): Hooks::runWithoutAbort(string, array)
#8 /srv/mediawiki/php-1.34.0-wmf.15/includes/OutputPage.php(1950): OutputPage->addParserOutputText(ParserOutput, array)
#9 /srv/mediawiki/php-1.34.0-wmf.15/includes/page/Article.php(700): OutputPage->addParserOutput(ParserOutput, array)
#10 /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/Actions/ViewEntityAction.php(79): Article->view()
#11 /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/Actions/ViewEntityAction.php(54): Wikibase\ViewEntityAction->showEntityPage()
#12 /srv/mediawiki/php-1.34.0-wmf.15/includes/MediaWiki.php(507): Wikibase\ViewEntityAction->show()
#13 /srv/mediawiki/php-1.34.0-wmf.15/includes/MediaWiki.php(302): MediaWiki->performAction(Article, Title)
#14 /srv/mediawiki/php-1.34.0-wmf.15/includes/MediaWiki.php(892): MediaWiki->performRequest()
#15 /srv/mediawiki/php-1.34.0-wmf.15/includes/MediaWiki.php(523): MediaWiki->main()
#16 /srv/mediawiki/php-1.34.0-wmf.15/index.php(42): MediaWiki->run()
#17 /srv/mediawiki/w/index.php(3): include(string)
#18 {main}

Impact

This seems to impact pages on test that still have cache entries from before new Termbox was enabled

Notes

Error on logstash: https://logstash.wikimedia.org/goto/a160398a2cfc9adfde427f1a425c343a

Details

Subject	Repo	Branch	Lines +/-
Add hook to invalidate cache entries missing TermboxOption	mediawiki/extensions/Wikibase	wmf/1.34.0-wmf.17	+137 -0
Add hook to invalidate cache entries missing TermboxOption	mediawiki/extensions/Wikibase	master	+137 -0
Use PageRenderingHash hook to avoid Termbox v1 and v2 cache collisions	mediawiki/extensions/Wikibase	master	+20 -0

Customize query in gerrit

Related Objects

Mentioned In: T348354: 1.42.0-wmf.1 deployment blockers

Event Timeline

Michael created this task.Jul 25 2019, 10:31 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 25 2019, 10:31 AM

Jakob_WMDE subscribed.Jul 25 2019, 10:34 AM

Lydia_Pintscher merged a task: T228850: Unknown placeholder: entityViewPlaceholder-entitytermsview-entitytermsforlanguagelistview-class.Jul 25 2019, 10:39 AM

Lydia_Pintscher added a project: Wikibase-Termbox-Iteration-20.

Lydia_Pintscher added a subscriber: Tarrow.

Copied information from the other task:
On page
[XTg6eQpAME0AAB7XveAAAAAC] 2019-07-24 11:01:13: Fatal exception of type "RuntimeException"

From Logstash:
RuntimeException from line 62 of /srv/mediawiki/php-1.34.0-wmf.15/extensions/Wikibase/repo/includes/ParserOutput/PlaceholderExpander/ExternallyRenderedEntityViewPlaceholderExpander.php: Unknown placeholder: entityViewPlaceholder-entitytermsview-entitytermsforlanguagelistview-class

Entities we found it on:
Q7, Q100 and Q173121

No longer on Q7 after purging the cache. We are deliberately not purging the other to keep as test subjects

@Jakob_WMDE was unable to reproduce locally; toggling the UseSSR variable always seemed to result in refreshing the cache

Tarrow updated the task description. (Show Details)Jul 25 2019, 11:03 AM

Krinkle moved this task from Untriaged to Jul2019/1.34.wmf.13+ on the Wikimedia-production-error board.Jul 25 2019, 9:06 PM

Tarrow claimed this task.Jul 26 2019, 8:51 AM

Tarrow moved this task from To Do to Doing on the Wikibase-Termbox-Iteration-20 board.

This is now perfectly reproducible locally. ( We were caught out by $wgInvalidateCacheOnLocalSettingsChange )

It appear to happen for the reasons we all expected. There is still an "old" version of termbox in the ParserCache which ovbiously contains place holder that ExternallyRenderedEntityViewPlaceholderExpander doesn't know how to expand.

Addshore subscribed.Jul 28 2019, 11:41 PM

@Jakob_WMDE and I spent quite some time looking at it today. We came to the following discoveries.

We attempted to add a new cache key:

We tried registering it in \Wikibase\RepoHooks::onParserOptionsRegister
and both adding the extra key to to ParserOptions ($options->addExtraKey( 'wbNewTermbox' );) and recording the option ParserOptions ($output->recordOption( 'wbNewTermbox' );) in extensions/Wikibase/repo/includes/Content/EntityContent.php

This was unsuccessful because the parser option we created ('wbNewTermbox') was not used in generating the ParserCache key until after a new parser cache entry was made:
This can be seen in \ParserCache::getKey (includes/parser/ParserCache.php:194 and includes/parser/ParserCache.php:216) where the options keys themselves are read from the cache.

Perhaps this wouldn't be an issue if we recomputed the ParserCache options at this point?

This can be seen in: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/526117/2

It works as we intend in the case that you purge the parser cache first. Then the key is taken into account and the cache is correctly split for any future cache reads

Change 526174 had a related patch set uploaded (by Jakob; owner: Jakob):
[mediawiki/extensions/Wikibase@master] Use PageRenderingHash hook to avoid Termbox v1 and v2 cache collisions

https://gerrit.wikimedia.org/r/526174

gerritbot added a project: Patch-For-Review.Jul 29 2019, 3:58 PM

@Addshore do you feel like having a look at the above considerations and those patches, i.e. https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/526117/3 and https://gerrit.wikimedia.org/r/526174, and providing some guidance what would be the right (tm) way to achieve what we want to achieve?
It looks like both hooks would lead to a success, but there must be some reasons why two separate hooks exist. We've come up with some interpretations for our use, but still are not sure which direction we should continue in.
Thanks sir!

Just to clarify the two options we thoughts of are https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/526117/3 (not 2) and https://gerrit.wikimedia.org/r/526174

I.e
1 https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/526117/3
Overload the wb ParserOption and concatenate to the end of the parser version when termbox is loaded.

This uses a newer hook (ParserOptionsRegister) I think first used https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/444950

https://github.com/wikimedia/mediawiki/blob/master/docs/hooks.txt#L2502 (thanks @WMDE-leszek) hints maybe this is the better newer way to do it? but has the disadvantage that we are reusing an existing option.

2
https://gerrit.wikimedia.org/r/526174
Using (PageRenderingHash) and therefore not altering the existing parser options but also not having it available in the parser options.

It sounds like when old parser cache entries are encountered the old behaviour should continue to happen, and the new SSR service should not be called.
Otherwise turning on the feature will essentially purge / reject / ignore all previously cached pages, which is not a best situation.

Instead including something new in the parser cache output and being able to detect old Vs new parser cache outputs would be best. Allowing conditional running of the SSR code Vs legacy code.

This would mean that the SSR termbox is slowly introduced and cached for wikidata.

Thoughts?

I think that would probably not work that well. We don't really want to provide editors with a mixed old/new experience where they can edit some items but not others.

I think we aren't too worried about the load from mobile pageviews so the fact that after deployment we'd have just cache misses is probably fine.

I think we're settled that we want to split the cache by new / old termbox. This means we'll avoid the above error; we'll also avoid the inverse error if we have to revert the deployment and there are now some.new termboxes in the cache.

I think the question now is simply where is it best to do the splitting the cache?

Indeed we are only talking about the mobile term box right now.
Looking at the 2 Gerrit changes I think something like #1 would be best (I haven't seen the hook in #2 used at all anywhere).
Although #1 could en changed to register a new parser option called wb-termbox for example, default value null, used in cache key and when termbox is rendered set it to 2.

I believe that would leave the current cache keys as they are / only change the cache key for renders with the termbox, but it would be worth checking that.

I haven't dug into all of the code, but is shouldRenderTermbox always the right thing to be checking?
I guess the cache avoidence when the SSR is down etc happens somewhere else?

The future should also be considered, and what is the behaviour is we complete termbox for desktop, roll it out everywhere, but then need to turn the feature off / shouldRenderTermbox = false.
Is turning the feature off going to result in all pcache entities essentially being lost, as it probably shouldn't.
But maybe this should be something we think about for the desktop release rather than just mobile, but we don't want to forget.

In T228978#5375173, @Addshore wrote:

I haven't seen the hook in #2 used at all anywhere).

FWIW, this hook is for example used by the MobileFrontend extension: https://gerrit.wikimedia.org/g/mediawiki/extensions/MobileFrontend/+/88faf91e20e097550c8c45b35121e4b58ba1816b/includes/MobileFrontendHooks.php#359 and Math extension: https://gerrit.wikimedia.org/g/mediawiki/extensions/Math/+/e2dd4949832adddab9cbd6ea010942d4d72f48cf/src/MathHooks.php#111

In T228978#5375173, @Addshore wrote:

Although #1 could en changed to register a new parser option called wb-termbox for example, default value null, used in cache key and when termbox is rendered set it to 2.

This would definitely be the nicest solution. Sadly, we discovered that adding a new parser option has the issue that it isn't used in the key until the cache entry for a page is purged. This puts us in a catch-22 where our cache invalidation won't be seen until the cache is purged. This could be a bug or it could be deliberate. It's a bit confusing but hopefully explained by T228978#5373176.

Tarrow moved this task from Doing to To Do on the Wikibase-Termbox-Iteration-20 board.Jul 30 2019, 8:48 AM

You could include the same logic checking shouldRenderTermbox and checking the current key / options for pcached content in the parsercacherejectvalur hook.
This would allow you to reject values where you want to use SSR and the caged Verizon doesn't already have an SSR term box, then re parsing and using a key with the new value.

Tarrow moved this task from To Do to Doing on the Wikibase-Termbox-Iteration-20 board.Jul 30 2019, 2:02 PM

@Addshore: can you take a look at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/526117/4 in comparison to https://gerrit.wikimedia.org/r/526174

Do you have a preference? Specifically:

I was a little unsure of all the static calls to the termbox flags.
Do do you think it is good or bad to have the additional ParserOption?

Had a call with @Addshore:

Conclusion is we should add it as a ParserOption because termbox being enabled or not is an option that we set the affect the output of the parsed page. We'd rather not just change the cache key because down the line we might want to actually use the ParserOptions of the termbox version to do something.

The reject hook is the right way to go about introducing the new key; we can get rid of it once all the old entries are gone though.

Tarrow removed Tarrow as the assignee of this task.Aug 1 2019, 8:12 AM

Tarrow moved this task from Doing to To Do on the Wikibase-Termbox-Iteration-20 board.

Picking this up again. Will work on the existing patch;

Change 526174 abandoned by Jakob:
Use PageRenderingHash hook to avoid Termbox v1 and v2 cache collisions

https://gerrit.wikimedia.org/r/526174

Change 528073 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/Wikibase@master] Add hook to invalidate cache entries missing TermboxOption

https://gerrit.wikimedia.org/r/528073

Tarrow moved this task from Doing to Peer Review on the Wikibase-Termbox-Iteration-20 board.Aug 5 2019, 2:13 PM

Change 529059 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/Wikibase@wmf/1.34.0-wmf.17] Add hook to invalidate cache entries missing TermboxOption

https://gerrit.wikimedia.org/r/529059

Change 528073 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add hook to invalidate cache entries missing TermboxOption

https://gerrit.wikimedia.org/r/528073

Mentioned in SAL (#wikimedia-operations) [2019-08-08T12:01:38Z] <tarrow@deploy1001> Synchronized php-1.34.0-wmf.17/extensions/Wikibase/: SWAT: [[gerrit:529055|Split ParserCache on Termbox (T228978)]] (duration: 01m 21s)

Change 529059 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@wmf/1.34.0-wmf.17] Add hook to invalidate cache entries missing TermboxOption

https://gerrit.wikimedia.org/r/529059

Mentioned in SAL (#wikimedia-operations) [2019-08-08T12:15:14Z] <tarrow@deploy1001> Synchronized php-1.34.0-wmf.17/extensions/Wikibase/: SWAT: [[gerrit:529059|Add hook to invalidate cache entries missing TermboxOption (T228978)]] (duration: 01m 14s)