Do it self to avoid the global state of the Timing class within the
resourceloader.
Replaced the deferred update with a scoped callback to send stats on all
possible return path for the called function, but directly when needed
and not deferred, which can make wrong times.
The old code was using a fix mark set be
MediaWiki::doPostOutputShutdown, where a defered send does not has
impact of the send stats
Change-Id: I5fec6a73db3e7c3ac6a6ac33bb293727ae3a38ca
User::isRegistered is part of the UserIdentity interface,
which makes moving from heavy User class to UserIdentity easier.
Change-Id: If44bfd398694c509272b3f89c93473b2e3c05759
In all cases where the client registry is used, the `noscript` module
is inapplicable. Additionally, make it illegal to have a module
include the 'noscript' module as a dependency.
Bug: T291735
Change-Id: I5846f2c19b003989b56b12628b385fd37049483b
Follows-up Ieaf04e0c289646dd5 which changed internal references to
bool(true) for 'debug' to the integer DEBUG_ constants, and introduced
a new debug=2 parameter.
In the refactor, I missed the setDebug() calls for
DerivativeResourceLoaderContext, which were still passing a boolean,
but more importantly were effectively passing debug=1 even if the
pageview carried debug=2. This isn't a problem yet in production since
debug=2 is currently identical in behaviour to debug=1.
The impact of this issue is mainly noticed through secondary CSS
requests. The URLs for primary stylesheets, and JS modules was already
forwarding the current "debug" version.
Test Plan:
* Open Main_Page?action=edit&debug=2
* Before this patch, e.g. on mediawiki.org today, secondary
stylesheet requests (part of a JS module) have debug=1.
For example "modules=jquery.makeCollapsible.styles&only=styles".
* After this, everything has debug=2 when the page view has debug=2.
Bug: T85805
Change-Id: Ia8fba4e30397bc5890033f13417b6739b0f83c38
=== Why
* More speed
In debug mode, the server should regenerate the startup manifest
on each page view to ensure immediate effect of changes. But,
this also means more version recomputation work on the server.
For most modules, this was already quite fast on repeat views
because of OS-level file caches, and our file-hash caches and
LESS compile caches in php-apcu from ResourceLoader.
But, this makes it even faster.
* Better integration with browser devtools.
Breakpoints stay more consistently across browsers when the
URL stays the same even after you have changed the file and
reloaded the page. For static files, I believe most browsers ignore
query parameters. But for package files that come from load.php,
this was harder for browsers to guess correctly which old script URL
is logically replaced by a different one on the next page view.
=== How
Change Module::getVersionHash to return empty strings in debug mode.
I considered approaching this from StartupModule::getModuleRegistrations
instead to make the change apply only to the client-side manifest.
I decided against this because we have other calls to getVersionHash
on the server-side (such as for E-Tag calculation, and formatting
cross-wiki URLs) which would then not match the version queries that
mw.loader formats in debug mode.
Also, those calls would still be incurring some the avoidable costs.
=== Notes
* The two test cases for verifying the graceful fallback in production
if version hash computations throw an exception, were moved to a
non-debug test case as no longer happen now during the debug
(unminified) test cases.
* Avoid "PHP Notice: Undefined offset 0" in testMakeModuleResponseStartupError
by adding a fallback to empty string so that if the test fails,
it fails in a more useful way instead of aborting with this error
before the assertion happens. (Since PHPUnit generally stops on the
first error.)
* In practice, there are still "version" query parameters and E-Tag
headers in debug mode. These are not module versions, but URL
"combined versions" crafted by getCombinedVersion() in JS and PHP.
These return the constant "ztntf" in debug mode, which is the hash
of an empty string. We could alter these methods to special-case
when all inputs are and join to a still-empty string, or maybe we
just leave them be. I've done the latter for now.
Bug: T235672
Bug: T85805
Change-Id: I0e63eef4f85b13089a0aa3806a5b6f821d527a92
This is in preparation for making all version hashes the empty string
in debug mode, which will make things faster to work with, but also
helps solve T235672 in an easy way client-side without having to
add additional complexity to the mw.loader client that is specific
to debug mode.
Bug: T235672
Change-Id: I43204f22dfbcf5d236b35adc5b35df3da8021bad
Our basic support has been updated and Firefox supports replacement
`resolution` since version 8 or 16, depending which data source
one checks.
WebKit's support is a bit more complex and it seems like safest bet is
to leave its vendor prefix in for now.
Bug: T277803
Change-Id: I752f0b67672759824099b4b78892d143344e766f
addGoodLinkObj() has many optional arguments, but omitting them actually
means corrupting the cache.
Nearly all existing callers are in tests.
So LinkCacheTestTrait::addGoodLinkObject() was created only
for testing. It is better to have this method in the
trait, because building the row directly in each test
would make these tests brittle against schema changes.
The only usage in WMF production code was in WikiPage and has been
fixed.
Bug: T284955
Change-Id: I03a2bd9ed64fcc0281ee29a286c8db395a9e03d9
This reverts commit bc200b6a1a reapplying
the patch. It has been modified slightly to temporarily work
with the beta cluster's unsupported configuration.
Bug: T207038
Change-Id: Ie86a5b59fbf93a400796a4cac3724207830092b5
This reverts commit 33ac451d3d.
Reason for revert: /wiki/Special:Version TypeError: Argument 1 passed to ResourceLoaderSkinModule::getRelativeSizedLogo() must be of the type array, boolean given, called in includes/resourceloader/ResourceLoaderSkinModule.php on line 600
Change-Id: Iddeb33d6165169dc106e9beb3e4a703a4b97eeb6
In debug mode of a packageFiles module there is now only one newline
between the JavaScript code and the closing }.
This change reuses ResourceLoader::ensureNewline and make this to a
interal public static function.
Change-Id: I89519896e3dc56d966c4a63102904686bff6fac9
The native "foreign module source" feature, as used by the GlobalCssJs
extension, did not work correctly in debug mode as the urls returned
by the remote wiki were formatted as "/w/load.php...", which would
be interpreted by the browser relative to the host document, instead
of relative to the parent script.
For example:
1. Page view on en.wikipedia.org.
2. Script call to
meta.wikimedia.org/w/load.php?debug=true&modules=ext.globalCssJs.user&user
This URL is formatted by getScriptURLsForDebug on en.wikipedia.org,
when building the article HTML. It knows the modules is on Meta, and
formats it as such.
So far so good.
3. meta.wikimedia.org responds with an array of urls for sub resources.
That array contained URLs like "/w/load.php...only=scripts".
These were formatted by getScriptURLsForDebug running on Meta,
no longer with a reason to make it a Meta-Wiki URL as it isn't
perceived as cross-wiki. It is indistinguishable from debugging
a Meta-Wiki page view from its perspective.
This patch affects scenario 3 by always expanding it relative to the
current-request's wgServer. We still only do this in debug mode. There
is not yet a need to do this in non-debug mode, and if there was we'd
likely want to find a way to avoid it in the common case to keep
embedded URLs short.
The ResourceLoader::expandUrl() method is similar to the one in
Wikimedia\Minify\CSSMin.
Test Plan:
* view-source:http://mw.localhost:8080/w/load.php?debug=1&modules=site
For Module base class.
Before, the array entries were relative. After, they are full.
* view-source:http://mw.localhost:8080/w/load.php?debug=1&modules=jquery
For FileModule.
Before, the array entries were relative. After, they are full.
* view-source:http://mw.localhost:8080/wiki/Main_Page?debug=true
Unchanged.
* view-source:http://mw.localhost:8080/wiki/Main_Page
Unchanged.
Bug: T255367
Change-Id: I83919744b2677c7fb52b84089ecc60b89957d32a
About 1.5% of load.php wall-time is spent in isFileModule() calls
during the ServiceWiring/getResourceLoader/register call early on.
Reduce the overhead of this cost by moving that logic to the Module
class.
There are two costs that we save this way:
1. The inherent cost of applying the skinStyles.
This is now limited to only the modules that are constructed in a
given web request. Thus, apart from the startup response (which
constructs all modules), for regular load.php requests and all
index.php page views, the vast majority of modules will never need
to be constructed, and thus won't pay this cost.
2. The overhead of predicting (and class-loading) for whether a module
is (or will become) a FileModule object.
This is what isFileModule() does and is the main reason I wrote
this patch. It involves class loading, and checks and conditions that
run 1000+ times at WMF. This is eliminated now because we no longer
have to calculate this decision. Instead, the logic applies when
it needs to (due to FileModule implementing it), and doesn't
when it doesn't!
Change-Id: Ia2db14f930800c96e767b94ef62fb00e9d52725b
This was covered in small ways already, but we did not have coverage
of the registration logic where the skin attribute is injected into
the module info, and this then correctly getting read and composed
into a single stylesheet.
This is prep for an optimisation in a subsequent commit.
Change-Id: Ia6fff44a345bb4f5811956638cada8b3e6b6ea71
Changes from patch set Icb93c79f4843b59dae80d3eda1a880457a1a68f2
Also some swaps from assertEquals to assertSame/True/False/Null
Change-Id: Ife497ae6cb1888b77eb25e85b76df72adc65641a
We’re appending at least a closing brace to the source code, so if that
code ends with a line comment, we need to have a line break between the
end of the code and our appended brace.
Bug: T286996
Change-Id: I4e93b10f31445f6dab76c1614e546fb41fd6c0f6
Gated behind the flag $wgParserEnableLegacyMediaDOM. The scattershot
usage of it is a little unfortunate but isn't expected to live very long
so maybe that's acceptable.
Further details can be found at,
https://www.mediawiki.org/wiki/Parsing/Media_structure
Bug: T51097
Bug: T266148
Bug: T271129
Change-Id: I978187f9f6e9e0a105521ab3e26821e36a96b911
There is a fallback in Module->getConfig() to the global services
container. This is not meant to be used in practice, but there were
two places where this was missing: WebInstallerOutput, and various
PHPUnit tests.
* Add missing setConfig() to WebInstallerOutput.
* Add missing setConfig() to various tests. Most tests were already
doing this correctly and using the standard mock from
ResourceLoaderTestCase. Upon switching the last few tests as well,
I uncovered various errors due to the mock missing some settings
that the tested code uses, so these have been added now to
ResourceLoaderTestCase.
Bug: T277728
Change-Id: I44f16ec4e00423fb6f641e58fffc1d40e4571f01
`content` should be mapped to `content-thumbnails` when using list mode
rather than throwing a fatal.
Bug: T271441
Change-Id: Idb30a94803f1432e2c1f57d55eb8c8aef1368dfb
The value of content-media now depends on configuration.
These modules can be merged, as the expectation is both should
apply when the UseNewMediaStructure configuration flag is enabled.
For backwards compatibility map any content-thumbnail requests to
content-media
Bug: T278560
Change-Id: I10ba53b1ee217eae9e05182e989d56f5dc107722
Since both MediaWikiIntegrationTestCase and
MediaWikiUnitTestCase use MediaWikiCoversValidator,
subclasses do not need to do so, just tests that
extend the base PHPUnit test case directly
Change-Id: Ie8702625621d55130b04b6ef85114d3375a3b39c
This was added in 381499a675 in response to the issue that when a skin
adopts a feature that was new in MW 1.36, it caused an exception when
that version of the skin is then installed on MW 1.35. That's expected,
and generally helpful because the default support policy requires
like-for-like (MW 1.35 with REL1_35, this applies to all skins and
extensions, except for a few like Translate and SemanticMediaWiki).
However, while supporting LTS within master is unusual, there are
indeed the Translate/SemanticMediaWiki projects, and this problem has
already been solved years ago there by using a conditional with
$wgVersion. The proposed approach on T271441 actually also involves
such a conditional, but late at runtime instead of in the module
definition. Anyway, the way it was fixed in Timeless already solves
the problem by not passing down the invalid feature in the first
place, thus making the 381499a675 patch obsolete. I'm not aware of
any skin in Gerrit having such a compat policy, but if they exist,
they can fix it the same way, by defining the module in code with
a condition, just as Translate/SemanticMediaWiki do.
Bug: T271441
Change-Id: If97b3d04ac1e3355ee4f229db11e524d24ebd27a
* Remove odd use of ResourceLoaderFileModule::extractBasePaths()
SkinModule.php which was basically just returning $IP.
* Use getConfig()->get('ResourceBasePath') so that the test does
not need to override global configuration variables.
Follows-up 6845912bcf and f1457312dc.
Bug: T277728
Change-Id: I64262b48bb5dc71c53b4114eddb2f080a6a076f5
* Remove confusing dead code for T271441, as a continue statement at
the end of a loop body does not do anything. I'll reinstate the
exception once we've looked at whatever skin caused the issue and
have fixed those repos to not pass invalid options.
* Simplify list creation by avoiding the loops and duplicate logic.
Assign the list directly when possible, and otherwise store the
filtered keys.
* Remove unexpected access to `$this->getConfig()` in the constructor.
Follows-up a6c769e976, which worked by accident due to it defaulting
to global state. The injection from ResourceLoader::getModule happens
after the constructor. I am working on a patch to enforce this, and
thus this change needs to land first.
I've moved the logic to getStyleFiles() and boldly changed it to be
a simple skip rather than based on whether it was specified. If this
is a problem we can find a more complex way, but I wanted to try this
first. It seems there is no reason to load these, specified or not,
when the parser feature is not enabled. (Ref T266148)
The unit test was working around this violation in a similar way,
which is now redundant and removed in this patch.
* Document that compatibility method only works on map-form.
* Make applyFeaturesCompatibility() an internal and protected
method. I don't think we'd support extensions calling this
directly.
* Limit applyFeaturesCompatibility() to just dealing with
the features array instead of also handling other options
and applying of the default value.
Bug: T277728
Change-Id: I24a2b783570c888cedee66885647b3ed765e0132
1) Rename content-parser-output to `content-body`
Widen the scope of this module to apply to anything that can be rendered
as the article body. Improve the documentation on what is allowed here.
For early-adopter skins already using content-parser-output,
map the feature to the new name and add tests.
2) Reclaim the `mw-body-content` class for result of
SkinTemplate::wrapHTML
The `mw-body-content` has been used in skins to wrap various elements.
Going forward we will use it to wrap any HTML content generated by
OutputPage.
See dependent patches, which we're not directly depending on to avoid
a CI gremlin:
- I90d85c21f4a62e6697f24e3ce388445a0a53c2b0 (MonoBook)
- I11242e243c9a529b72972089af9ac2a8c906331a (Modern)
- I87942c60e62f6f14acdfeaa1836ace4eac9252ac (CologneBlue)
- I4c1b15d90bacbc9b13782a1d8f52e838ce8ecd83 (Vector)
Bug: T279388
Change-Id: I3a91b294fcb3724cd46743e497dff723de0490a6
It is not entirely meaningless. It might be an indicator that
the number of calls to a method is intentionally unlimited.
This is similar to e.g. an @inheritDoc PHPDoc comment that
marks a method as being "intentionally undocumented".
However, what's the meaning of being "intentionally
unconstrained"? Let's just not have any constraint then.
I feel all these ->expects( $this->any() ) bloat the test
code so much that it's never worth it.
Change-Id: I9925e7706bd03e1666f6eb0b284cb42b0dd3be23
After talking to Volker, the `elements` feature should not contain any
CSS classes. It should apply only on the tag level. I've updated documentation
to explain what's permitted here.
The link styling rules added in b24bff67ed are instead added to a new
feature `content-links`. The existing `content-links` is renamed to
`content-links-external` and for backwards compatibility anyone enabling
`content-links` without `content-links-external` will get the new styles.
The existing legacy feature for backwards compatibility loads the `content-links`
feature which addresses the problem in Timeless with plainlinks styling being lost.
Bug: T279693
Bug: T255717
Bug: T278576
Change-Id: I6e55ed2880782533ea61df98cf16dbf36483d895
It's the same and makes the test code much more readable, I
would like to argue.
Because of the was I split all the changes I made into smaller
patches this patch contains some other changes in the same
lines where I could not split them off. E.g. removal of
->any(), which is the default anyway and doesn't do anything.
Change-Id: Ib297b989d4aec33b31a4e33fe9d5032865b39be0
Ended up using
grep -Prl '\->setMethods\(' . | xargs sed -r -i 's/setMethods\(/onlyMethods\(/g'
special-casing setMethods( null ) -> onlyMethods( [] )
and then manual fix of failing test (from PS2 onwards).
Bug: T278010
Change-Id: I012dca7ae774bb430c1c44d50991ba0b633353f1
This is a reasonable default for all skins and is small enough
to not be a concern for existing skins.
Also: expand documentation so this is understood better.
Bug: T277218
Change-Id: Ia58037e2c32d76d5c7125772a3243b76ede45f3f
Modules that set "es6": true in their module definition will error when
a non-ES6 client tries to load them.
To detect ES6 support, this looks for native Promise support,
RegExp.prototype.flags, and non-BMP characters in variable names. All
browsers that lack full ES6 support fail at least one of those checks.
To flag modules as requiring ES6, this adds a ! to the end of their
version string. This takes up much less space than adding another
register() parameter (which would have to be at the end). It's hacky,
but we expect this feature to be relatively temporary, until we require
ES6 for running any JS at all (probably in about a year).
For distinguishing different types of errors thrown from
sortDependencies(), use e.name. We can't subclass Error properly because
that requires ES6.
Bug: T272104
Change-Id: I45670c910ff12eb422ae54c9fcf372e45c7b2bf1
The canonical way to enable debug mode is and will remain
through via 'debug=true'.
During the transition debug=2 will opt you in to the experimental
newer way. Anything that needs to be pinned to the old way for
compat can already start doing so by using debug=1 explicitly.
Once v2 is "ready", the default will flip and debug=1 will remain
for the foreseeable future to trigger the legacy behaviours.
Bug: T85805
Change-Id: Ieaf04e0c289646dd5d5b027b4f1f8278167b2d57
