The "nonce" CSP feature has been never been properly tested and is not
on track to be enabled at WMF or in MW by default. The use of
nonce-protected eval is not particularly meaningful. It is trivially
bypassed by mw.loader.implement, mw.loader.store, or importScript();
all of which allow any code to be executed directly, or to be stored
and loaded from a first-party origin.
The "nonce" feature is not required for the T208188 roadmap, and with
change I51b8535b2b21a3 there is now also a (small) performance reason
to keep this disabled long-term.
Until and unless there is a plan for enabling this particular aspect
of CSP, we might as well remove it so that we don't waste time our
time building abstraction layers and satisfying dependency injection
through many years for something that might never be used.
Note that this does not remove CSP support from ResourceLoader, and
does not take away from the future of enabling CSP in MediaWiki to
e.g. strictly block third-party scripts, or to require domains to
opt-in by site config or user pref (T208188).
Change-Id: I5a176c41a06a484a11e64bdacdc42b40811fe72e
"BadMethodCallException" sounds like it would fit, but it does
have a very different meaning, described as "exception thrown if
a callback refers to an undefined method or if some arguments are
missing". This is not what's going on here. These are methods that
should only be called from unit tests.
This appears to be a common mistake, often copy-pasted.
Change-Id: Ib39e28f596a883481d5f526460a5c871c75f5313
* Extend ConsoleLogger to support filtering by log level and
forwarding to another logger.
* Extend ConsoleSpi to support filtering by channel/level and
forwarding to another provider.
* Allow configuring which channels shell.php should log to STDERR
and at what levels.
* Deprecate shell.php -d, use more meaningful parameter names.
* Change logging behavior so that logging to STDERR doesn't
prevent normal logging, which seemed like confusing behavior
(either we should always disable normal logging for shell.php
or never, it shouldn't depend on whether we log to console).
Keep old behavior for -d, just in case.
Change-Id: I5d29c95995bf83784c96dd40df9c96e22b9ec73e
Notable changes:
* In SqlBagOStuff::getConnectionFromServerInfo, only two loggers were
injected. The rest implicitly got a NullLogger due to being absent.
These are now effectively unsilenced.
* Database::__construct() required almost all parameters, even the
loggers. I've wanted to move some of DatabaseFactory into the ctor
here for a while. In order to make this change not a breaking
change, the new 'logger' parameter is optional with NullLogger as
default. This allowed some of the test cases, which were simply
passing NullLogger, to be fixed by passing nothing instead of
passing the new option name.
The Database class is behind a dozen layers of indirection for
real use, so this will still be injected just fine (DBF, LB, LBF,
MWLBF, etc.).
* In LegacyLogger, the handling for $wgDBerrorLog was previously
limited to DBConnection and DBQuery. This now includes errors
from other (generally, newer) parts of Rdbms as well, which were
previously missing.
This only affects sites (typically CI and dev setup) where
$wgDBerrorLog is used, as opposed to the more common
$wgDebugLogGroups by-channel configuration.
* TransactionProfiler gets its logger injected in a rather odd way,
via entrypoints (MediaWiki.php, ApiMain.php, and MaintenanceRunner)
as opposed to service wiring. This is kept as-is for now.
* In LBFactoryTest, in particular testInvalidSelectDBIndependent2,
there are cases that intentionally produce failures of which
the result is then observed. In CI we assert that dberror.log is
empty so instead of adding the missing logger fields to that
LBFactory instance, the only one set (replLogger) is removed.
The alternative is to set 'logger' now, which would naturally
cause CI failures due to unexpected entries coming through to
non-mocked error log.
Bug: T320873
Change-Id: I7ca996618e41b93f488cb5c4de82000bb36e0dd3
* ForkController, OrderedStreamingForkController: indeed pcntl_fork()
can't return false.
* RL\Image: Specify type instead of using suppression, since the issue
name changes.
* VueComponentParser: Accept complaint about nullable nodeValue.
* Disable PHP 8.0 polyfill stubs when running on PHP 8.0+ to avoid
duplicate interface errors.
* Add Socket stub and use it in LegacyHandler instead of multiple
existing suppressions.
* MemcachedPeclBagOStuff: accept complaint recommending !$result over
$result === false when the type is boolean.
* MemcachedPeclBagOStuff: fix probable bug, ignoring errors from
Memcached::getMulti(). Phan noticed that $res=false was unreachable,
but it should probably be reachable.
* DatabaseMysqli: accept complaint that $this->conn->errno is already
known to be an int. It was probably a hack for some previous version
of Phan.
* BcryptPassword, MWOldPassword, MWSaltedPassword: accept complaint that
the !is_string() checks are unnecessary, after code review of PHP.
* Pbkdf2PasswordUsingHashExtension: note that contrary to Phan's
suggestion, this check is necessary.
* DefaultPreferencesFactory: remove an existing hack for
array_diff_key(), no longer necessary on 7.4 and causes an error on
8.1. Use coalesce instead of cast for the remaining
array_intersect_key() hack since it better shows that we are casting
away null.
* FullSearchResultWidget: fix likely bug involving strict comparison
between a float and an int.
* SpecialWatchlist: accept complaint that $selectedHours is
unconditionally a float, being the return value of round(), and thus
the cast is unnecessary.
* Add stub for AllowDynamicProperties, resolving an error in User.php.
* Xml: accept complaint that $encMonth is already known to be an int.
Six errors remain. These need suppressions or otherwise conflict with
PHP 7.4 support.
Bug: T322278
Change-Id: Ie375bbc8ccf22330b9a169e8da98f2bbe26ec8b9
Move ResourceLoader classes to their own namespace. Strip the
"ResourceLoader" prefix from all except ResourceLoader itself.
Move the tests by analogy.
I used a namespace alias "RL" in some callers since RL\Module is less
ambiguous at the call site than just "Module".
I did not address DependencyStore which continues to have a non-standard
location and namespace.
Revert of a241d83e0a.
Bug: T308718
Change-Id: Id08a220e1d6085e2b33f3f6c9d0e3935a4204659
This reverts commit e08ea8ccb9.
Reason for revert: Breaks Phan in extensions, and as far as I’m aware,
this change isn’t urgently needed for anything, so the simplest fix is
to revert it again for now. After PHP 7.4 it should be safer to try this
again (we hopefully won’t need the two “hack” classes by then).
Bug: T308443
Change-Id: Iff3318cbf97a67f821f78e60da62a583f63e389e
Move ResourceLoader classes to their own namespace. Strip the
"ResourceLoader" prefix from all except ResourceLoader and
ResourceLoaderContext.
Move the tests by analogy.
I used a namespace alias "RL" in some callers since RL\Module is less
ambiguous at the call site than just "Module".
I did not address DependencyStore which continues to have a non-standard
location and namespace.
Change-Id: I92998ae6a82e0b935c13e02a183e7c324fa410a3
The benefit of this setting was largely lost due to wfDebug() having
been replaced with calls to Logger->debug() for much of our code.
This is motivated by making ResourcLoader debug=2 able to make
its concurrent load.php requests more quickly during local dev.
This effectively reverts T49960. I was wrong there, thinking that
this variable has no reason for being. The vast majority of the time,
these aren't of interest on subresource requests. It's trivial
to enable when wanted, and any non-debug level messages still come
through either way, including PHP runtime errors, exceptions,
warnings, etc.
This seems to make load.php about 50% faster for local development
when using mediawiki-docker.
I used `ab` to perform a load.php request 300 times. The mean was
320-360ms before this patch (with and without DebugRawPage=false),
and around 120-140ms with patch (with DebugRawPage=false, default).
Bug: T85805
Bug: T225730
Change-Id: I061419adebf43e83cd0103bdb9b7a3296a2a1122
The new class, UrlUtils, is usable standalone or as a service. Using it
as a service will just automatically load a few settings from site
configuration.
In addition to just making our code cleaner, this will enable making
some of Setup.php's dynamic configuration more sane.
Test coverage is all lines except invalid URLs -- I couldn't find any.
Bug: T305093
Change-Id: I706ef8a50aafb518e13222719575d274c3583b90
The wfDeprecated is called directly from __get/__set/__isset which needs
only a 2, not 3. That means the message is one caller of.
Change-Id: Ia29fb0a07841f69a4d8b95431f0915df9793c340
These two interfaces' methods have tentative return types in PHP 8.1,
which causes code without the type hints to raise warnings. Where the
type hint is "mixed", we need to use the special declaration
[\ReturnTypeWillChange] in a comment to suppress the warning as long as
we still support PHP < 8.0, which doesn't have a "mixed" type hint.
Bug: T289879
Change-Id: I1a126e602e92b8d13c7795eb6d790effd5ddc986
MediaWiki still supports PHP 7.2+, but we want to mainly test in newer versions
of PHP. Setting minimum_target_php_version to 7.2 this lets us run phan without
phan trying to get us to make PHP 7.2-incompatible changes to 'appease' PHP 8.0
or whatever later changes.
Some switches of generic 'resource' type-hinting to 'resource|object' to inform
phan to ignore this (triggering PHPCS at the time, ah well), rather than trying
to hint the specific novel PHP encapsulation classes to that have replaced them
from PHP 8.0 onwards but don't yet exist, and fixes from where we were checking
the results of implode and explode.
Bug: T293924
Change-Id: I629e3fb3adfad73beb3d424a07e643c2e079d9bb
Also remove the unmaintained kafka-php package from the from "suggested"
and "dev" composer dependencies, as this is now no longer used.
Change-Id: If5668974f417b627df95bce47db18d46fa03327c
These require use of nmred/kafka-php 0.1.0, which is from 2015.
Upstream has had one major release since then, in 2017. There have
been various commits, issues, and pull requests since but nothing
released for five years.
EventRelayer was created for use with WANObjectCache, where WMF
considered use of Kafka as one of the possible transports. This never
happened, and WANObjectCache has not supported EventRelayer since 2019,
removed with change I328eb74f21c23.
The EventRelayer abstraction itself remained unused, until it became
re-purposed for CdnCacheUpdate, which WMF now relays via EventBus.
Monolog KafkaHandler is no longer used, and was similarly created for
WMF's use case. It seems unlikely to be of interest to most third
parties. And those larger installs like WMF would likely be more
interested in SyslogHandler which is more standardised across the
industry (especially for cloud native tooling around containers).
Someone using this can continue to do this by adding a similar class
to their own code instead, possibly published as an extension.
Deprecating this in MW 1.38 (with backport) means we can remove it
shortly after in master, and no longer maintain this unused/untested
integration for which no known users exist, as well as remove our
supported suggested dependency on the unmaintained kafka-php lib.
Change-Id: I475227293ca050acead357efebce12e93a971f65
Make phan stricter about null types by setting null_casts_as_any_type to
false (the default in mediawiki-phan-config)
Remaining false positive issues are suppressed.
The suppression and the setting change can only be done together
Bug: T242536
Bug: T301991
Change-Id: I0f295382b96fb3be8037a01c10487d9d591e7e01
The functions returning null or the class property is set explict null
Found by phan strict checks
Change-Id: I4a271093fb6526564d8083a08249c64cb21f2453
This replaces references to DefaultSettings with
references to config-schema.yaml where appropriate.
NOTE: this does not yet change Setup.php. DefaultSettings.php
remains intact and is still being used.
NOTE: this does not remove usages in the installer, see I5d8843a1062fbf
for that.
Bug: T300129
Change-Id: Ie6152cf510c3be61bc22167ca6d90dfc28910a45
* Enforce the ban on is_resource in phpcs
* In OrderedStreamingForkController, the comment was incorrect. I
confirmed using a small test script that if the child closes one end
of a socket pair, the other end will still be open, and is_resource()
will still return true, and fclose() will not fail. The issue was
introduced in c82d30d19c, it was not present in the
CirrusSearch copy of the class.
* Allow is_resource() for debug logging.
* Allow is_resource() for parameter validation where a stream may be
passed to a function, since there is no alternative.
Bug: T260735
Change-Id: I59a752f7bb4b043ddfb2434b52a02f9221bf54db
The cost of creating closures adds up, so avoid it if we can be using
method names instead.
Bug: T297236
Change-Id: Ifb78d5f310fe45db58fd450c9db3c7af295ae399
DeprecationHelper currently breaks dynamic properties
on phpunit mocks. This happens because phpunit starts
mocking the magic methods if they're explicitly defined.
By default, magic methods and up doing nothing, but
if proxying to original methods is enabled, magic methods
are called like regular methods, regarless of whether
property exists or not. With this patch we can workaround
this issue, and create mocks for classes with deprecations.
Needed-By: I4297aea3489bb66c98c664da2332584c27793bfa
Change-Id: Id60a7751ece05669eced6eddd3216da7149411c7
We need a way to actually just detect the deprecation override,
without instantly emitting the deprecation warning. Providing
'false' to the method as a $version parameter seems like a logical
thing to use for it.
Change-Id: I7733846613f37a4ba168e944bc671b43264aa498
Setting and getting dynamic class properties (e.g. $parser->my_fancy_prop)
is a horrific practice that we should get rid of, but it is still
used. DeprecationHelper right now can not be used on classes
where this practice is used by extensions. Introduce methods
to allow or deprecate this practice.
Change-Id: Ib378bef792dd31be155f186f1a4965a05004a37c
