As +2er

In T299352#7626069, @Esanders wrote:

Possibly caused by https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/753590/

Possibly also caused by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/752216. Reverting either of these patches seems to fix the problem.

Basically, in VisualEditor's API we want to render the list of categories on the page, to update it after saving changes. This now requires calling into the current skin's portlet generation code, and Vector's code doesn't expect to be called in such context. (Or something like that, it's still unclear to me what exactly goes wrong.)

Change 754143 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/core@master] Revert \"Categories are modelled as a portlet\"

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

The exception is also reproducible without involving VisualEditor, only using the action=parse API that it uses internally:
https://en.wikipedia.beta.wmflabs.org/wiki/Special:ApiSandbox#action=parse&format=json&title=test&text=%5B%5Bcategory%3Atest%5D%5D&prop=categorieshtml

This should make it easier to debug.

The same request works fine when using legacy Vector:

Change 754143 merged by jenkins-bot:

[mediawiki/core@master] Revert \"Categories are modelled as a portlet\"

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

It is likely related to Vector hook refactors, not the core category change. https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/753990
These two changes happened around the same time, and the Vector patch didn't handle it properly. I'll look when I'm back tomorrow.

(None of the suspicious changes are in wmf.18, they'd only be in this week's wmf.19 release. No backports are needed.)

In T299352#7626816, @matmarex wrote:

(None of the suspicious changes are in wmf.18, they'd only be in this week's wmf.19 release. No backports are needed.)

Last week was wmf.17, .18 will be deployed this week (and hasn't even been cut yet).

Oops, my bad, I got the wrong numbers.

Change 753990 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Don't run Vector hook when menu absent from page

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

Change 754586 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Revert \"Revert \"Categories are modelled as a portlet\"\"

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

Change 753990 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Don't run Vector hook when menu absent from page

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

Hi, train conductor here. Can anyone confirm whether this is no longer UBN?

Hi @jeena, I'm currently waiting on someone (@matmarex or @Esanders perhaps?) to restore https://gerrit.wikimedia.org/r/754586
That will give us more confidence going into the train that the faulty hook is working now as expected.

Okay, thanks @Jdlrobson!

I think the reverts mean it's not really UBN any more -- editing is no longer broken on master. I don't think we have to restore that last patch for things to be in a deployable state?

@Jdlrobson The fix seems incomplete. The main exception is no longer occurring, but when I apply https://gerrit.wikimedia.org/r/754586, the action=parse API is still emitting PHP notices. You can see it in action here: https://patchdemo.wmflabs.org/wikis/9423f6ef98/w/api.php?action=parse&format=jsonfm&title=Main%20Page&text=%5B%5Bcategory%3Atest%5D%5D&prop=categorieshtml (you need to be logged in and set new Vector as your skin).

Notice: Undefined index: preferences in /srv/patchdemo-wikis/9423f6ef98/w/includes/GlobalFunctions.php on line 194

Change 754996 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/core@master] SkinTemplate: Set template context in buildPersonalUrls()

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

This does not block the deployment though, it only blocks merging https://gerrit.wikimedia.org/r/754586.

In T299352#7627179, @gerritbot wrote:

Change 753990 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Don't run Vector hook when menu absent from page

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

This 'fixed' the fatal and gives way to remerge the reverts; but it also introduces regressions that we'd likely have to revert it.

1. It reintroduces the userpage link back to the personal menu dropdown. Making them duplicate.
2. Duplicates logout link
3. Additionally note that, the items are missing their icons.

See https://en.wikipedia.beta.wmflabs.org/wiki/Talk:Main_Page for these. (PS, this is fixed, so no longer reproducible)

I think the important question is why is the 'user-menu' absent even though the user is logged-in? (That's why do we need https://gerrit.wikimedia.org/r/753990, assuming it doesn't even cause any regression?)

I attempted to track this down and found SkinTemplate::buildPersonalUrls() and SkinTemplate::$loggedin.

In https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/753590/ $sk->loggedin was changed to $sk->getUser()->isRegistered(). While this looks benign, I believe it's the immediate cause (together with r752216) of this fallout.
The property and direct call to User::isRegistered() are supposed to always be equivalent, but this assumption is sometime not true because $sk->loggedin will only be correct if SkinTemplate::setupTemplateContext() has been called earlier. The property is used in SkinTemplate::buildPersonalUrls(), but this method does not set the context (it assumes something does). There are various warning in the class to ensure setupTemplateContext() is called before calling the method.

Then comes https://gerrit.wikimedia.org/r/c/mediawiki/core/+/752216 that overrides Skin::getCategoryLinks() in SkinTemplate. The new method makes a call that goes way up to the SkinTemplate::buildPersonalUrls(). The overridden method in Skin (that'd have been previously called in the chain) does not. The method is called via ApiParse via Skin::getCategories(). (Note: The exception only happens via endpoint that goes through parse API: VE, DiscussionTools, LivePreview etc)

Calling SkinTemplate::buildPersonalUrls() without setting the context means SkinTemplate::$loggedin is false and the attendant consequence is why we are here.

Thanks for this writeup @Ammarpad and for flagging the potential issue with $sk->loggedIn
(Looking for the shortest path to unblock the deploy)

Change 754912 had a related patch set uploaded (by Jdlrobson; author: Ammarpad):

[mediawiki/core@wmf/1.38.0-wmf.18] SkinTemplate: Set template context in buildPersonalUrls()

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

Change 754913 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@wmf/1.38.0-wmf.18] Don't run Vector hook when menu absent from page

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

Change 754996 merged by jenkins-bot:

[mediawiki/core@master] SkinTemplate: Set template context in buildPersonalUrls()

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

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/e11b97bb43/w/

Change 754912 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.18] SkinTemplate: Set template context in buildPersonalUrls()

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

Thanks @Ammarpad and @Jdlrobson, everything looks good to me now.

I think we can also undo the original revert (that is, merge https://gerrit.wikimedia.org/r/c/754586), but I'll let Ammarpad do the honors here, because he left a -2 vote on it previously, and because this isn't urgent.

Thanks for checking @matmarex. Given all the above, seems sensible to leave https://gerrit.wikimedia.org/r/c/754586 for wmf19. The fact it's passing is going enough for me to feel confident about the train now.

Change 754913 merged by jenkins-bot:

[mediawiki/skins/Vector@wmf/1.38.0-wmf.18] Don't run Vector hook when menu absent from page

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

Change 754586 merged by jenkins-bot:

[mediawiki/core@master] Revert \"Revert \"Categories are modelled as a portlet\"\"

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

All changes are merged. Most fixes are in wmf.18 and are going to be deployed to production wikis this week; the restoration of the patch changing how the list of categories is rendered (which originally triggered the issue) will be in wmf.19, and it's going to be deployed next week.

All clear
Vector 2022:

Legacy Vector:

I am seeing same error but different stack trace on translatewiki.net. Is this same or different issue?

[2022-01-27T15:13:41.730075+00:00] exception.ERROR: [40924f60b8cad282b0def44c] /wiki/Special:Preferences   TypeError: Argument 1 passed to Vector\Hooks::makeMenuItemCollapsible() must be of the type array, null given, called in /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/Hooks.php on line 226 {"exception":"[object] (TypeError(code: 0): Argument 1 passed to Vector\\Hooks::makeMenuItemCollapsible() must be of the type array, null given, called in /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/Hooks.php on line 226 at /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/Hooks.php:289)
[stacktrace]
#0 /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/Hooks.php(226): Vector\\Hooks::makeMenuItemCollapsible()
#1 /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/Hooks.php(262): Vector\\Hooks::updateUserLinksDropdownItems()
#2 /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/Hooks.php(386): Vector\\Hooks::updateUserLinksItems()
#3 /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/SkinVector.php(428): Vector\\Hooks::onSkinTemplateNavigation()
#4 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/skins/SkinTemplate.php(1518): SkinVector->runOnSkinTemplateNavigationHooks()
#5 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/skins/SkinTemplate.php(776): SkinTemplate->buildContentNavigationUrls()
#6 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/skins/SkinTemplate.php(200): SkinTemplate->getPortletsTemplateData()
#7 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/skins/SkinMustache.php(79): SkinTemplate->getTemplateData()
#8 /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/SkinVector.php(575): SkinMustache->getTemplateData()
#9 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/skins/SkinMustache.php(56): SkinVector->getTemplateData()
#10 /srv/mediawiki/tags/2022-01-26_13:25:46/skins/Vector/includes/SkinVector.php(464): SkinMustache->generateHTML()
#11 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/skins/SkinTemplate.php(147): SkinVector->generateHTML()
#12 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/OutputPage.php(2701): SkinTemplate->outputPage()
#13 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/MediaWiki.php(917): OutputPage->output()
#14 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/MediaWiki.php(930): MediaWiki::{closure}()
#15 /srv/mediawiki/tags/2022-01-26_13:25:46/includes/MediaWiki.php(563): MediaWiki->main()
#16 /srv/mediawiki/tags/2022-01-26_13:25:46/index.php(53): MediaWiki->run()
#17 /srv/mediawiki/tags/2022-01-26_13:25:46/index.php(46): wfIndexMain()
#18 {main}
","exception_url":"/wiki/Special:Preferences","reqId":"40924f60b8cad282b0def44c","caught_by":"entrypoint"} []

@Nikerabbit that looks like T299671

In T299352#7656589, @Jdlrobson wrote:

@Nikerabbit that looks like T299671

hi @Nikerabbit: per the question Jon raised above, does T299671 capture the issue you mentioned in T299352#7656477?

In T299352#7656589, @Jdlrobson wrote:

@Nikerabbit that looks like T299671

I think so.
I just checked to confirm that edits are saved:

I have not been able to confirm, but I assume it does. I'll report if the issue persists after we deploy updates.

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/e11b97bb43/w/