Page MenuHomePhabricator

Remove support for automatic 'dir' parameter inference for load.php urls
Open, LowPublic

Description

The dir query parameter for load.php can set explicitly, but MediaWiki doesn't do this for its own urls. Instead, it passes lang only, and depends on ResourceLoaderContext to infer the correct directionality based on MediaWiki's localisation system.

This appears to be one of few non-trivial uses (if not, the only use) of MediaWiki Language classes in ResourceLoader and is a dependency I'd like to remove if possible.

Two changes required:

  • For urls created server-side in PHP, move the responsibility for setting this information to MediaWiki OutputPage, where it creates the ResourceLoaderContext object for ResourceLoaderClientHtml.
  • For urls created client-side in JS, the mw.loader code will need to be expanded to know the current directionality somehow and use it for subsequent load.php requests.

Event Timeline

Krinkle added a project: patch-welcome.
Krinkle added a subscriber: Fomafix.

@Fomafix Would you be interested in working on this?

The server-side part is easy.
The client-side part requires the information of the direction on JavaScript side. I see four possibilities to get this information:

  • Add an addition JavaScript config variable wgUserDirection or wgUserLanguageDirection.
  • Get the information from the existing JavaScript config variable wgUserLanguage via a mapping table. ULS already contains such a table. A complete table is big. Maybe the table can just filled with the used language codes wgUserLanguage and maybe other languages that are used.
  • Get the information from the browser.
  • Add a new JavaScript config variable specific for resource load containing all URL parameters that just reflected to the server. Default values like dir=ltr or debug=false can be omitted in this variable.

A similar dependency is the call of Language::getFallbacksFor. This can avoided in ResourceLoader by transmitting always the whole fallback language chain instead of only the user interface language. By the way this allows an user individual fallback chain.

The server-side part is easy.
The client-side part requires the information of the direction on JavaScript side. I see four possibilities [..]

  1. Add a new JavaScript config variable specific for resource load containing all URL parameters that just reflected to the server. Default values like dir=ltr or debug=false can be omitted in this variable.

I like this one most. It would be very efficient and reduces the amount of information and code required on the client. You can use the existing $VARS that StartupModule and mediawiki.js make use of to very efficiently export these for the current context. I think you can replace reqBase with a $VARS.something assignment to make this work. E.g. skin/lang always, and debug/dir only as needed.

Krinkle triaged this task as Medium priority.Jun 16 2019, 7:35 PM

I like this one most. It would be very efficient and reduces the amount of information and code required on the client. You can use the existing $VARS that StartupModule and mediawiki.js make use of to very efficiently export these for the current context. I think you can replace reqBase with a $VARS.something assignment to make this work. E.g. skin/lang always, and debug/dir only as needed.

Yes, this is the best solution. I created T225899 to set the default URL parameters for JavaScript on server-side.

Why $VARS.something and not mw.config.get( 'something' )?

Change 518688 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] resourceloader: Add parameter 'dir' to loader URLs

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

Change 693629 had a related patch set uploaded (by Fomafix; author: Fomafix):

[mediawiki/core@master] resourceloader: Add parameter 'dir' to loader URLs [1/2]

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

@Fomafix I've been thinking some more about this in reviewing the latest version of your patch today and I'm realizing this may not be a good idea after all. I'm sorry for not realizing sooner. If you have thoughts or opinions on this, I'd be very open to hearing those. If you were mainly helping out from a code cleanup perspective, that's fine too, I hope there's plenty more that we can work on together.

This task is part of a series to make resourceloader a standalone library, but I think in this instance we may have taken it too far to the point that it affects usability and API complexity.

  • Usability, because the users of this library would now have to come up with their own mapping for language codes to RTL in order for it to work correctly. I don't think we can claim that ResourceLoader supports 300 languages when it still requires the user to specify the correct direction.
  • API complexity, because this means we'd have to set an additional parameter for a significant number of languages and carry that state through from the backend to the frontend and then back to the backend again. Even if we are going to require the RL consumer (e.g. MediaWIki) to provide their own mapping, I think we could do it in a smarter way where the mapping array is injected into the ResourceLoader constructor and then automatically used when determining the context direction, without going through back-front-back. Also with T190129, this could become a small composer dependency that we use by default and perhaps not even need a way to inject/override it.

As for the query parameter existing, maybe it is useful for testing purposes, but I would actually be open to considering removing that parameter in the future if there is no need for it.

Of course there must be somewhere a mapping from a language code to the direction of the language. For ResourceLoader as standalone library it would be good to have standard way for this mapping. But I still think it would be good to use this mapping only once on the initial constructor of the ResourceLoader instead of on every follow up request. I thing it is better to use separate values for dir and lang and include them only if the response vary on the value and omit the dir attribute if it is equal to the DEFAULT_DIR.

For https://gerrit.wikimedia.org/r/c/mediawiki/core/+/518688/29/ fresnel reports in https://integration.wikimedia.org/ci/job/mediawiki-fresnel-patch-docker/43311/consoleFull a reduction of the JavaScript size:

13:15:55 ### scenario View recent changes
13:15:55 |----------------------------------------------------------|---------------------|---------------------|----------|---|
13:15:55 | Metric                                                   |              Before |               After |     Diff |   |
13:15:55 |----------------------------------------------------------|---------------------|---------------------|----------|---|
13:15:55 | navtiming:                                               |                     |                     |          |   |
13:15:55 |   Time to first byte (responseStart)                     |      60 ms (± 5 ms) |      56 ms (± 5 ms) |     0 ms |   |
13:15:55 |   Total page load time (loadEventEnd)                    |    492 ms (± 18 ms) |    489 ms (± 21 ms) |     0 ms |   |
13:15:55 |   Time from responseEnd to domComplete (processing)      |    427 ms (± 19 ms) |    429 ms (± 17 ms) |     0 ms |   |
13:15:55 |   Time from loadEventStart to loadEventEnd (onLoad)      | <0.1 ms (± <0.1 ms) | <0.1 ms (± <0.1 ms) |     0 ms |   |
13:15:55 | paint:                                                   |                     |                     |          |   |
13:15:55 |   Time to first paint (TTFP)                             |    340 ms (± 14 ms) |    331 ms (± 23 ms) |  P 0.221 |   |
13:15:55 |   Total to first contentful paint (TTFCP)                |    340 ms (± 14 ms) |    331 ms (± 23 ms) |  P 0.221 |   |
13:15:55 | transfer:                                                |                     |                     |          |   |
13:15:55 |   Total size of transfers during page load (pageWeight)  |      1.3 MB (± 0 B) |      1.3 MB (± 0 B) | - 1.4 kB | ✓ |
13:15:55 |   Transfer size of HTML document (html)                  |     32.0 kB (± 0 B) |     32.0 kB (± 0 B) |      0 B |   |
13:15:55 |   Transfer size of CSS resources (css)                   |     44.4 kB (± 0 B) |     44.4 kB (± 0 B) |      0 B |   |
13:15:55 |   Transfer size of JavaScript resources (js)             |      1.2 MB (± 0 B) |      1.2 MB (± 0 B) | - 1.4 kB |   |
13:15:55 |   Transfer size of Image document (img)                  |     28.9 kB (± 0 B) |     28.9 kB (± 0 B) |      0 B |   |
13:15:55 |   Transfer size of other resources (other)               |         0 B (± 0 B) |         0 B (± 0 B) |      0 B |   |
13:15:55 |----------------------------------------------------------|---------------------|---------------------|----------|---|

Is this a real improvement because of the separation of the values and omitting of the default values?

Of course there must be somewhere a mapping from a language code to the direction of the language. For ResourceLoader as standalone library it would be good to have standard way for this mapping. But I still think it would be good to use this mapping only once on the initial constructor of the ResourceLoader instead of on every follow up request. I thing it is better to use separate values for dir and lang and include them only if the response vary on the value and omit the dir attribute if it is equal to the DEFAULT_DIR.

I agree. But would it not be simpler if we never had to specify dir on load.php at all, and then load.php can figure it out from the map given to the constructor without needing to pass it to every RTL request.

This is actually the situation we have today. The dir parameter in ResourceLoaderContext only exists for testing purposes. It is never set in reality on any LTR or RTL wiki as far as I know. For example, on https://he.wikipedia.org/ we use load.php?lang=he, and the direction is figured out from Language->getDir().

For https://gerrit.wikimedia.org/r/c/mediawiki/core/+/518688/29/ Fresnel reports a reduction of the JavaScript size:

### scenario View recent changes
...
 |   Transfer size of JavaScript resources (js)             |      1.2 MB (± 0 B) |      1.2 MB (± 0 B) | - 1.4 kB |   |

Is this a real improvement because of the separation of the values and omitting of the default values?

I am not sure where this difference is coming form, but I do notice that it only applied to the "View recent changes" scenario. The "View a page" scenario shows ± 0 B difference. I suspect it might be that something broke in RCFilters with the current patch that causes it to load less JavaScript code for some reason. 1.5 kB is quite a lot.

However, as mentioned, we already omit all dir values, both default and non-default, because it is determined by the Language class. My proposal is to remove the dir parameter entirely. Then for the standalone use case, we change Language->getDir with an array or closure pass to ResourceLoader::__construct() and then depending on whether we make this public or private, we either have RL call context->setDirection (if the map is private), or if the map is public the context class could call rl->getDirectionForLang().

This may seem complicated, but remember that we need something like this no matter what. The only question is, do we call the map when making load.php URLs, or do we call the map when responding to load.php requests. I propose the latter, because it means we can continue to omit the dir parameter always, and even remove it entirely (it currently only exists for testing, as far as I know).

Ok, I agree. HTTP has also just one parameter for the language in the HTTP header field Accept-Language and no parameter for the direction of the language. It should be possible with a short and fast mapping table to get the direction of the language based on a language code.

Change 693629 abandoned by Krinkle:

[mediawiki/core@master] resourceloader: Add parameter 'dir' to loader URLs [1/2]

Reason:

Closing per task, change in direction

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

Change 518688 abandoned by Krinkle:

[mediawiki/core@master] resourceloader: Add parameter 'dir' to loader URLs [2/2]

Reason:

Closing per task, change in direction

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

Krinkle lowered the priority of this task from Medium to Low.Sep 18 2023, 1:07 PM

@Fomafix: Per emails from Sep18 and Oct20 and https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup , I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!). Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task. Please claim this task again when you plan to work on it (via Add Action...Assign / Claim in the dropdown menu) - it would be welcome. Thanks for your understanding!