Page MenuHomePhabricator

Some extensions register their namespaces too late
Closed, ResolvedPublic

Description

Apologies in advance for the laconic report -- I am exhausted at the moment.

I made a configuration change earlier that should have resulted in the Schema namespace getting enabled on test2wiki. It only half-worked: I was able to create an article in the Schema namespace, and it had the correct value for page_namespace in the database. But the namespace name was getting stripped from the title, transforming 'Schema:Test' into ':Test', as far as most interfaces were concerned. We ran scap after making the configuration change and later on I updating the i18n cache a second time, but that did not seem to fix things.

In PHP, the return value of '$wgContLang->getNamespaceIds()' contained both new namespaces (Schema and Schema_talk). But they did not appear in the 'wgNamespaceIds' object in JavaScript, even several hours after the deployment.

I do not think it's a static asset caching issue: interfaces generated by PHP that one would have expected to display the Schema were not displaying it.

I finally noticed a comment in CommonSettings.php, added by Gerrit change I147c16ecf1b235d4fec514d0c6f2ef10932caef9, that seemed relevant: Fundraising seems to have resorted to declaring the namespace manually, in the CommonSettings.php. I tried the same trick in I8ac1eae2456d845afe989052f188aea20f9f5d38 and it appears to have worked.

Relevant comments from mwalker on #wikimedia-tech (Fri Feb 15 01:29:37 UTC 2013):

17:25 <mwalker> right -- so that was because I was originally adding the namespaces via the $wgExtensionFunctions[] hook system
17:25 <mwalker> which happened too late
17:26 <mwalker> but, CanonicalNamespaces should always happen at the right time
17:26 <ori-l> Hrm.
17:27 <mwalker> I will say that I'm adding my namespace to both the $namespaces param in that hook; and then to all the other fun arrays
17:27 <mwalker> but -- I haven't tested this on test2
17:27 <mwalker> only test

I *do* use CanonicalNamespaces in EventLogging, so perhaps the problems aren't related after all. But for whatever reason adding the namespace manually in CommonSettings.php seems to have fixed my problem, too.


Version: wmf-deployment
Severity: major

Details

Reference
bz45031

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz45031.
bzimport added a subscriber: Unknown Object (MLST).
ori created this task.Feb 15 2013, 1:31 AM

Setting this bug to high/major, as it's needlessly wasting developer time in debugging why the namespaces are somewhat being registered, but not fully being registered.

Seems to be related to the site configuration so moving to "Site requests"

mwalker wrote:

When I was poking around trying to solve this I noticed that it might have had something to do with how the Language object cached the canonical namespaces in order for it to cache the localized names. This was caused in CentralNotice by attempting to use the Title object too early (it worked but broke everything later on.)

I also note that the CanonicalNamespaces hook is called multiple times even though it's ostensibly located inside of a static gate -- that one is probably just a fluke though.

(In reply to comment #4)

Happened again w/deployment of Campaign NS: see
https://test.wikipedia.org/wiki/Special:Contributions/Maintenance_script

At least at the moment, the early call to CanonicalNamespaces seems to be from ShortUrlHooks::setupUrlRouting(), which is called for the WebRequestPathInfoRouter hook, which is being called from the call to $wgRequest->interpolateTitle() in Setup.php.

ShortUrlHooks::setupUrlRouting() calls getPrefixedText() on a Title object for its special page, which eventually calls MWNamespace::getCanonicalNamespaces() to find the local name for the "Special" namespace, which calls the hook and caches the result.

One possibility for a general fix would be to call MWNamespace::getCanonicalNamespaces( true ) from near the bottom of Setup.php, to force the list of canonical namespaces to be recached.

https://gerrit.wikimedia.org/r/74523 implements the suggestion in the last comment, but since I am not completely too sure what is happening here, might be completely wrong.

Change 74523 had a related patch set uploaded by SuchABot:
Force re-cache of Canonical Namespaces

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

Change 74598 had a related patch set uploaded by Reedy:
Force re-cache of Canonical Namespaces

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

Change 74598 merged by jenkins-bot:
Force re-cache of Canonical Namespaces

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

That change doesn't seem to help. However, disabling ShortUrl does seem to help.

(In reply to comment #10)

That change doesn't seem to help. However, disabling ShortUrl does seem to
help.

Ah. The Language objects keep their own cache of namespace names, so we'd need to clear that too. Sigh.

(In reply to comment #5)

At least at the moment, the early call to CanonicalNamespaces seems to be
from
ShortUrlHooks::setupUrlRouting(), which is called for the
WebRequestPathInfoRouter hook, which is being called from the call to
$wgRequest->interpolateTitle() in Setup.php.

Setup.php line 530 doesn't seem very early to me. Why are namespaces being added after that? Usually, early initialisation problems affect LocalSettings.php.

ori added a comment.Jul 24 2013, 5:49 AM

(In reply to comment #12)

Setup.php line 530 doesn't seem very early to me. Why are namespaces being
added after that? Usually, early initialisation problems affect
LocalSettings.php.

The CanonicalNamespaces hook handler is registered in a $wgExtensionFunctions[] function. It's a pattern I started with EventLogging. Possibly an anti-pattern.

Yuvi uses it in UploadWizard and Yuri uses it in ZeroRatedMobileAccess and Molly uses it in BookManagerv2. It has possibly spread to another extension in the time it took me to write this comment.

I don't think there's a reason for UploadWizard or BookManagerv2, since these two extensions are either wholly enabled or disabled; they don't branch on the wiki name. EventLogging and ZeroRatedMobileAccess are different: the extensions are active on multiple Wikimedia wikis but select a specific wiki to house the configuration namespace for the entire cluster. (It's metawiki in both cases, but EventLogging also enables the Schema namespace on test2wiki.) The wikis on which the extra namespace is activated are specified via configuration variables.

I'm game for cleaning it up if it's bad pattern. How should this be done?

Why can't you register the CanonicalNamespaces hook in the normal way? Why does it need to be deferred?

$wgExtensionFunctions is meant to be as late as possible in the setup process, I think it's too late for adding namespaces.

ori added a comment.Jul 24 2013, 6:51 AM

(In reply to comment #14)

Why can't you register the CanonicalNamespaces hook in the normal way? Why
does it need to be deferred?

Because there are several things that need to be executed on the target configuration wiki, not just the namespace registration, and it seemed neater to have the activation logic be all in one place rather than sprinkled in multiple places. I suppose I could aggregate it all in the CanonicalNamespaces hook handler, though that looks a bit odd.

$wgExtensionFunctions is meant to be as late as possible in the setup
process, I think it's too late for adding namespaces.

Well, OK, but in that case why have the hook at all? If namespace registration shouldn't be deferred, the only interface for adding namespaces should be adding entries to $wgCanonicalNamespaceNames.

ori added a comment.Jul 24 2013, 7:08 AM

(Tim replied on #wikimedia-dev; edited for clarity and reproduced below. --OL)

ZeroRatedMobileAccess sets hooks based on a boolean configuration variable. Obviously you can't do that from the file scope. It should instead have the configuration variable checks inside unconditionally-registered hook functions, e.g.:

static function onCanonicalNamespaces( array &$namespaces ) {
    if ( !$GLOBALS['wgZeroRatedMobileAccessEnableZeroConfigPages'] ) return;
    ...

But most of the content of zero's onCanonicalNamespaces() can be in the file scope. For example:

$wgNamespaceContentModels[NS_ZERO] = 'JsonZeroConfig';

This can just be in the file scope. It's harmless to configure a non-existent namespace. There definitely shouldn't be global variable assignments in a CanonicalNamespaces hook handler. It's not guaranteed that the hook will be called before the variables are accessed. What's guaranteed is that there will be no hook calls between execution of the file scope and setting of the configuration variables so you can reference configuration globals from hook functions with confidence.

Change 76325 had a related patch set uploaded by Ori.livneh:
Register NS_ZERO early if running on configuration wiki

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

Change 76326 had a related patch set uploaded by Ori.livneh:
Register NS_SCHEMA early if running on schema wiki

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

Change 76327 had a related patch set uploaded by Ori.livneh:
Declare NS_BOOK namespace unconditionally

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

Change 76328 had a related patch set uploaded by Ori.livneh:
Register NS_CAMPAIGN at file level in extension's entry point

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

Change 74523 abandoned by Ori.livneh:
Force re-cache of Canonical Namespaces

Reason:
The underlying cause of 45031 was the deferment of namespace registration to an extension function, which is way too late. I committed a fix to each extension which followed this pattern: I2fb2bcaa2 (UploadWizard), I76cfa1d5a (BookManagerv2), Ide5f09206 (EventLogging), and I2fb2bcaa2 (ZeroRatedMobileAccess).

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

Change 76327 merged by jenkins-bot:
Declare NS_BOOK namespace unconditionally

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

Change 76328 merged by jenkins-bot:
Register NS_CAMPAIGN at file level in extension's entry point

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

Change 76326 merged by jenkins-bot:
Register NS_SCHEMA early if running on schema wiki

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

Change 76862 had a related patch set uploaded by Ori.livneh:
Register NS_ZERO early if running on configuration wiki

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

Change 76325 merged by jenkins-bot:
Register NS_ZERO early if running on configuration wiki

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

I'm not really sure what's going on with Gerrit changeset 76325 v. Gerrit changeset 76862, but this bug seems nearly resolved/fixed.

Change 76862 abandoned by Ori.livneh:
Register NS_ZERO early if running on configuration wiki

Reason:
Dupe.

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

The four affected extensions (ZeroRatedMobileAccess, EventLogging, BookManagerv2, and UploadWizard) have all been fixed. Marking this bug resolved/fixed accordingly.