Page MenuHomePhabricator

Regression: Module is not being loaded with module
Closed, ResolvedPublic3 Estimated Story Points


On itwiki we noticed that, on certain pages, mobile style isn't loaded. For instance, this happens here, where the class sinottico isn't loaded and thus breaks the infobox. I did a little bit of investigation on my local machine and found out that the culprit might be Specifically, this if evaluates to false and style isn't loaded; actually, when should it evaluate to true? It asks for a global variable which defaults to false and which, as far as I could see, is never changed. Anyway, I'm not sure that the one I exposed is the real cause, but certainly inverting that if makes the style come back.

From the user's perspective, the symptom is Mobile.css being not applied.

Developer notes

Acceptance criteria

  • Loading also loads

Event Timeline

I realised that such global variable is meant to be used in the future. The problem is shifted to includes/modules/MobileSiteModule.php, where I think getDependencies isn't called (while getPages is), although I can't figure out why.

Is it related that ResourceLoaderWikiModule, unlike similar classes, doesn't have a getDependencies method?

Daimona added subscribers: Jdlrobson, Krinkle, Catrope.

Sounds like sometimes the CSS is loaded, sometimes not and sometimes only some rules are loaded. This may lead to corrupted mobile visualization. Adding as subscribers people from previous task.

What is the problem? Its not clear from the description what is happening.

@Jdlrobson the main problem is that mobile style, i.e. Mediawiki:Mobile.css isn't loaded, thus breaking page style.

Jdlrobson renamed this task from Mobile style isn't loaded to Module is not being loaded with module.Apr 8 2018, 4:20 AM

I'll look into this Monday, but yes it seems like the dependencies inside,unified are not being honoured for some reason.

As a workaround you can put this code into MediaWiki:Mobile.js to get this to work:


@Jdlrobson Thanks for the hint, I applied the fix.

Jdlrobson renamed this task from Module is not being loaded with module to Regression: Module is not being loaded with module.Apr 9 2018, 5:11 PM
Jdlrobson triaged this task as High priority.
Jdlrobson moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.

@Jdlrobson I looked at this over the weekend. I believe the culprit is the isKnownEmpty() logic.

The wiki previously did not have a Common.js or Mobile.js page. As such, their was previously loading with an empty script, and the CSS only. That's fine.

When we split the modules into and (with the former depending on the latter), the former became empty. ResourceLoader would normally load both modules at once in the same batch just like before (given dependency resolution happens ahead of time). But this isn't working because WikiModule, unlike FileModule, has an isKnownEmpty() method that makes the module skip itself if none of its associated pages exist on the wiki.

This makes it so that modules like site.styles and, which require loading in a separate http request, only happen from the HTML if said feature is enabled (wgUseSiteCSS) and used (the on-wiki page exists).

The problem is that, while this works fine for style modules, it poses a problem for JS modules, as skipping them means we also don't look at the dependencies anymore.

As far as I can tell, the fix would be to not apply isKnownEmpty to dynamic loading, but only to hardcoded http links. I recall this being what it was designed for, so I'm unsure of as to why and how it got to apply to dynamic loading. But should be a simple fix.

whym rescinded a token.
whym awarded a token.
whym subscribed.

Description edited - hopefully this is now more discoverable by keywords like 'Mobile.css'.

@Krinkle the module in extension.json ( appears to be specifying its own dependencies which clobber the default module dependencies.

pmiazga set the point value for this task to 3.Apr 10 2018, 4:13 PM

@Krinkle the module in extension.json ( appears to be specifying its own dependencies which clobber the default module dependencies.

Yeah, that's a separate issue. That dependency should probably be moved to the MobileSiteModule class. However, note that the code being referred to here already didn't work. Even before this commit, was using class MobileSiteModule, which extends WikiModule without implementing an dependencies option, and without a getDependencies implementation. So as far as I can tell, this has not been working for quite some time. It may be safest to simply remove it.

// Location:

//> {module: {…}, version: "1g4dpfx", dependencies: Array(1), group: null, source: "local", …}

//> [""]

Change 425570 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Do not rely on dependencies to load

I've intentionally kept the fix for this as simple as possible ^

@Jdlrobson, it looks like there's an open question in code review as to how to proceed. Should we split this task so that the original tracks the revert and the split tracks the proper fix?

I'll fix the bug in core which is about as much effort as a revert, but without all the regressions.

But I'd like to have a failing test in RLClientHtml to work from. That should be easy to set-up, and I can help with that too (ping me on IRC).

Change 428545 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] isKnownEmpty modules with dependencies cannot be empty

Change 428545 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Consider having dependencies as non-empty in WikiModule::isKnownEmpty

Minor follow up in needs to be merged. Once that's done we can move this to QA.

Change 425570 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] should not have dependencies

This can skip sign off as it's tricky to test. is now running site css (note the logo "BLabs")

@Jdlrobson @Krinkle et al., thanks for fixing. However there's a little problem: the first patch was deployed with wmf.1, the other one will be in wmf.2. In the meanwhile, keeping the mobile.js as suggested above will load mobile style also in desktop version, while removing the manual RL call we still have the initial problem reported in this task. Could someone please take a look and deploy the last patch on wmf.1 if necessary?

@Daimona the first patch won't do anything. The second patch is the one that's needed.
The suggestion in will load the styles in the mean time on the mobile. It will not load the styles in desktop - MediaWiki:Mobile.js is only loaded for mobile devices. You can use that workaround safely with or without the second patch. If you are seeing that JS running on desktop we have a bigger problem here! :)

@Jdlrobson I had applied the suggestion to manually call RL, but after yesterday's deployment of wmf.1 many users started complaining for wrong graphics. It turned out that the mobile style was loaded on desktop and deleting Mobile.js brought everything back to normal. Of course, without that manual call, mobile users still don't have the CSS. So, I'm afraid we actually have another problem here, although I'm not sure about how big it could be :-)

@Daimona Hmmm.. I'm not seeing how this could happen. I just verified now that it's impossible for the mobile script to load on desktop so I'm not sure what could have caused this.

If the Mobile.js page is restored and the issue returns I'll be happy to debug that and work out what's going on for you but it should be safe to restore that page. If not, we should identify the bug here and get that fixed immediately.

@Jdlrobson Huh, I restored Mobile.js and the problem is gone, both for desktop and mobile. Maybe it's been some cache-ish problem, but at least now everything's fine :-)