This regression was introduced in I6670a58fe1.
Bug: T399793
Co-Authored-By: Jonathan Lee <cookmeplox@weirdgloop.org>
Change-Id: I26b61e2a08b51aaca5d2740dcaf20b509be380eb
(cherry picked from commit fa05279424e0688a7b34f1186050dca1e2ec5f4b)
Almost every call to isCascadeProtected() (which uses short-circuit
mode) is followed by a call to getCascadeProtectionSources() (which
doesn't), so this attempted optimization (skipping a loop that does
some very cheap operations) actually results in worse performance
in the common case (because the result of the database query can't
be cached in short-circuit mode, and we must query it again), and
it makes the code really annoying to read or modify.
Relevant code: https://codesearch.wmcloud.org/search/?q=getCascadeProtectionSources\(|isCascadeProtected\(&excludeFiles=RestrictionStore.php|HISTORY|tests%2F
Change-Id: Ib9eb6cab28492776d40a10cbfb28e9c1cec8c1d2
(cherry picked from commit f9180c4a36fb8874fc0211f05a1eebaceb67aa0c)
This patch reworks RestrictionStore::getCascadeProtectionSourcesInternal
to return a third and fourth array:
* One for cascading restrictions originating from templatelinks
* Another for those originating from imagelinks
They are used in PermissionManager::checkCascadingSourcesRestrictions
to differentiate cascading protection of file content and file page,
but could also be used in the future by action=info and other callers.
Bug: T24521
Bug: T62109
Bug: T140010
Change-Id: Ia5863f418538106f4fd657c672298ff6ac835805
(cherry picked from commit 7a4952ef2c5d593fae9419bad39f3e9894f42adf)
Why:
- PermissionManager::getUserPermissions() checks whether the user is
blocked if $wgBlockDisablesLogin = true, so that it can then limit
user's permissions to the set of permissions assigned to unregistered
users if so.
- This causes the GetUserBlock hook to run, which may itself check
permissions on the user (e.g. in the GlobalBlocking extension),
causing an infinite loop.
- Since the decision whether the user is blocked isn't yet final by the
time GetUserBlock runs, any permission checks triggered by
GetUserBlock handlers should see the user's full set of permissions.
What:
- Stash the user's permissions in PermissionManager's in-memory cache
before running block checks if BlockDisablesLogin = true.
- Add tests.
Bug: T384197
Change-Id: I3e3804fe518627e9edc2b574cce88f533fd93fe4
(cherry picked from commit 27062b9f8752cc853a65e8a46c9d7d1a9af32c48)
Why:
- Setting the increment to 0 should check the limit without bumping it.
- This was apparently broken by If3e66491306f22650.
What:
- Use LimitBatch::peek if the increment amount is 0
Bug: T381033
Change-Id: Ife76a1976a2063f051f00302e5adaebd701e6367
(cherry picked from commit e09606b3dc44711571cc6cf2d0d11bd7784d0cdd)
Implicitly marking parameter $... as nullable is deprecated in php8.4,
the explicit nullable type must be used instead
Created with autofix from Ide15839e98a6229c22584d1c1c88c690982e1d7a
Break one long line in SpecialPage.php
Bug: T376276
Change-Id: I807257b2ba1ab2744ab74d9572c9c3d3ac2a968e
A constant is not a variable. The type is hard-coded via the value
and can never change. While the extra @var probably doesn't hurt much,
it's redundant and error-prone and can't provide any additional
information.
Change-Id: Iee1f36a1905d9b9c6b26d0684b7848571f0c1733
Also:
1. Other minor fixes in comments touched by this patch.
2. Add a separate test for subpage with spaces.
Change-Id: I267f19027098e5778b1cd491826e1741fc7ee794
Mostly used find-and-replace:
Find:
/\*[\*\s]+@var (I?[A-Z](\w+)(?:Interface)?)[\s\*]+/\s*(private|protected|public) (\$[a-z]\w+;\n)((?=\s*/\*[\*\s]+@var (I?[A-Z](\w+)(?:Interface)?))\n|)
Replace with:
\3 \1 \4
More could be done, but to keep this patch reasonably sized, I only
changed the most obvious and unambiguously correct cases.
In some cases, I also removed redundant doc comments on the
constructor, and re-ordered the properties to match the constructor.
Change-Id: I3f8427ae4f5d55177ae18986ef15d84d0e7bf6f4
In change I625a48a6ecd3fad5c2ed76b23343a0fef91e1b83 I am planning to
make Wikimedia\Message\MessageValue use it, and we try to pretend that
it is a library separate from MediaWiki, so it makes sense to move
MessageSpecifier to the same namespace under Wikimedia\.
Bug: T353458
Change-Id: I9ff4ff7beb098b60c92f564591937c7d789c6684
In 279fd16bab, I made what I thought
was a trivial change to UserAuthorityTest:
(diff slightly modified here for clarity)
- $message = $permissionStatus->getErrors()[2]['message'];
+ $message = $permissionStatus->getMessages()[2];
$this->assertArrayEquals(
$this->getFakeBlockMessageParams(),
$message->getParams()
);
And in 3d92cb2f82, I made what I thought
was also a trivial change to UserAuthority:
(diff slightly modified here for clarity, likewise)
- foreach ( $errors as $err ) {
- $status->fatal( wfMessage( ...$err ) );
- }
+ $status->merge( $tempStatus );
However, it turns out these two pieces of code had vital roles:
* The code in UserAuthority ensured that the final status contains
Message objects instead of key strings + parameter arrays, and thus
does not trigger wikitext escaping in a legacy code path (T368821).
* The code in UserAuthorityTest accessed the internals of the same
status with (now deprecated) getErrors() to check that it indeed
contained a Message object, rather then a key string, which would
cause a test failure due to a fatal error in the code below.
getMessages() returns objects regardless of what's inside the
status, so the test never fails.
Thus I managed to disarm the regression test, and then cause exactly
the regression it was supposed to prevent: block error messages on
Special:CreateAccount have parameters shown as wikitext (T306494).
Restore a foreach loop instead of `$status->merge()` to fix that, and
document why it is there. Change the test so that it actually runs
the code whose behavior it wants to verify, instead of a related but
different method, hopefully making it more resilient against future
developers.
(I found the bug because the test started failing with the refactoring
I'm trying to do in I625a48a6ecd3fad5c2ed76b23343a0fef91e1b83.)
Bug: T306494
Change-Id: I7601fc51702cb33ef9d2b341ea555dc230d31537
Almost all of this code still needs a full User object. Still I
found a few places that can easily live with the much more narrow
UserIdentify interface.
Change-Id: I6a43f8acbb7470144a4118d86aa1b266d4e293f4
Replace UserCache with UserIdentityLookup
UserIdentityLookup is implemented by ActorStore and
there is already a cache
Change-Id: I8a59e77391da45d2726aab3d5432f08ad0c9a84f
PermissionManager::resultToStatus() is used for both
getUserPermissionsErrors / getUserPermissionsErrorsExpensive (which
return a single error) and TitleQuickPermissions (which returns an array
of errors). This mostly works because it processes arrays recursively,
but the array of errors can be empty if a hook returns false just to
prevent other extensions from adding errors (wich is apparently a thing
we do: Ib56f1085eea1)
Bug: T369260
Change-Id: Iaa93c04ca195718131b37289ffcbc35d5a1b8500
getPermissionStatus() does the same thing but better.
A lot of things use the legacy error arrays though, we're nowhere near
removing it.
Change-Id: Iff60dbb0593329a584d003b2407bbf24d5b22aea
getPermissionErrors() uses a weird format for its return value that
is slightly different from the usual "legacy error array", and legacy
errors arrays are already icky. Deprecate it without changing this
format, and introduce getPermissionStatus() to replace it. Document
the return format more precisely.
Refactor PermissionManager to use PermissionStatus objects internally,
and only convert to the weird format in the deprecated method.
However, fix a scenario where the error array could directly contain
MessageSpecifier objects or strings instead of nested arrays,
as the documentation said that was not possible. Fix a test case
demonstrating this incorrect behavior.
Change-Id: I6670a58fe1fcb4e1ae87351277e5ddf29c548183
Why:
* When temporary accounts were first implemented, temporary users were
added to the 'user' group, and named users were added to a 'named'
user group, to differentiate them from temporary users.
* Since T340457, temporary users were no longer added to the 'user'
group. This means that the 'named' group is no longer needed.
What:
* Stop adding users to a 'named' user group in UserGroupManager.
* Remove special handling for the 'named' group in PermissionManager
and tests.
Bug: T355741
Change-Id: I06bd5d7150203bbcb0806868fe829a438af486ba
What:
* Make user rights "deletedhistory" and "deletedtext" get rid of the
restriction of the namespace protection.
Why:
* On Special:Undelete, users who have rights to view deleted pages and
its revisions, will get restricted if the namespace of the page have
a namespace protection, and they don't have the right to edit pages
in this namespace, so the two designing purposes are conflicted:
read-only rights shouldn't be restricted by a namespace protection
which is designed to restrict edit actions.
Bug: T362536
Follow-Up: I02e820c818e9e58e6591ec412fc3eca9384e0bb2
Change-Id: I61ec3f8e1fe84927a6c987f387cbba349ec4a357
This is most certainly auto-generated by some IDEs. Unfortunately
there is nothing to learn from such comments. It's just noise.
Especially in tests.
Change-Id: Idf59332d96ca4718b6ce9d17b4da79a88641d4fd
According to the dictionary, "per" (or more conventionally "as per")
means "according to". Refer OED "per" sense II.3.a. For example:
"No value was passed, so return null, as per default".
In this sentence, we are not specifying the default, we are referring
to the default. This correct usage of "per default" was used nowhere
in MediaWiki core as far as I can see.
Instead we have "per default" being used to mean "by default", that is,
giving the value to use when no explicit value was specified.
In OED, the phrase "by default" is blessed with its own section just
for computing usage:
"P.1.e. Computing. As an option or setting adopted automatically by a
computer program whenever an alternative is not specified by the user
or programmer. Cf. sense I.7a."
There are highly similar pre-computing usages of the same phrase,
whereas the phrase "per default" is not mentioned.
As a matter of style, I think "per default" should not be used even
when it is strictly correct, since the common incorrect usage makes it
ambiguous and misleading.
Change-Id: Ibcccc65ead864d082677b472b34ff32ff41c60ae
When rendering Special:RecentChanges for an user with rollback rights,
the software checks whether the user has permission to perform the
rollback action on each entry via User::probablyCan(), which
delegates to PermissionManager. This in turn checks for each entry
whether the title has protections (e.g. from action=protect) against the
"rollback" action, which is unlikely to be the case since by default
wgRestrictionTypes only supports create/edit/move/upload.
This triggers noticeable memcached I/O and overhead.[1]
So, as an optimization, avoid fetching page restrictions in
RestrictionStore for action types that are known to not be restrictable
via page protection. In the unlikely event that wgRestrictionTypes is
customised to offer separate restrictions for "rollback", this will
work continue to work as expected and is covered by tests.
-
[1] acd6281072/recentchanges.speedscope.json
Bug: T341319
Change-Id: I912e4ef2fa3d48238c01631d4f35940aea93ab03
The array spread operator is documented to behave identical to
array_merge. The syntax is just much shorter and easier to read in
situations like this, in my opinion.
Change-Id: I3b016e896e552af53d87d5e72436dc4e29070ce1
The idea is that all formatters that need the user language or
other request specific context should be instantiated by
FormatterFactory.
Change-Id: I8334cc89dcf0f293298b82e004116be50a90f0d1
Found via (?<!IDBAccessObject)::READ_
We are planning to deprecate and remove implementing IDBAccessObject
interface just to use the constants.
Bug: T354194
Change-Id: I89d442fa493b8e5332ce118e5bf13f13b8dd3477
