My personal best practice is to not document @params when there
is a @dataProvider. I mean, these test…() functions are not
meant to be called from anywhere. They do not really need
documentation. @param tags don't do much but duplicate what the
@dataProvider does. This is error-prone, as demonstrated by the
examples in this patch.
This patch also removes @throws tags from tests. A test…() can
never throw an exception. Otherwise the test would fail.
Most of these are found by the not yet released I10559d8.
Change-Id: I3782bca43f875687cd2be972144a7ab6b298454e
Ideally, only User associated with the global request
should be associated with UserAuthority. For a random
user instance, not the current performer, checking permissions
should be based solely on user groups and perhaps an
existing block. Right now however, PermissionManager
is coupled with global request, so we still instantiate
a UserAuthority for non-current users. This mimics the
behaviour we've had before. As we refactor PermissionManager,
we will be able to replace Authority implementation in this case,
or even entirely prohibit non-performer authority.
Bug: T271459
Depends-On: Iebf2dca34eea751391d9740443c195287399aa5c
Change-Id: Ib094e498fd883db23f2763f171281b1c9e99217e
Most of these are found by the not yet released I10559d8.
I remove the type MockObject in some cases when the calling
code really does not need to know if he get's a mock or the
real thing. However, I do this only in places that are very
closely related to the fixes.
Change-Id: I26a4c3c5a8ae141bf56161b52b54bce7e68f2e30
The code didn't properly handle requests for info about non-existing
pages.
Bug: T271804
Bug: T271815
Change-Id: If4e56ff5ecb4a048767833c89847bb58269cad43
Many files were in the autoloader despite having potentially harmful
file-scope code.
* Exclude all CommandLineInc maintenance scripts from the autoloader.
* Introduce "NO_AUTOLOAD" tag which excludes the file containing it
from the autoloader. Use it on CommandLineInc.php and a few
suspicious-looking files without classes in case they are refactored
to add classes in the future.
* Add a test which parses all non-PSR4 class files and confirms that
they do not contain dangerous file-scope code. It's slow (15s) but
its results were enlightening.
* Several maintenance scripts define constants in the file scope,
intending to modify the behaviour of MediaWiki. Either move the
define() to a later setup function, or protect with NO_AUTOLOAD.
* Use require_once consistently with Maintenance.php and
doMaintenance.php, per the original convention which is supposed to
allow one maintenance script to use the class of another maintenance
script. Using require breaks autoloading of these maintenance class
files.
* When Maintenance.php is included, check if MediaWiki has already
started, and if so, return early. Revert the fix for T250003 which
is incompatible with this safety measure. Hopefully it was superseded
by splitting out the class file.
* In runScript.php add a redundant PHP_SAPI check since it does some
things in file-scope code before any other check will be run.
* Change the if(false) class_alias(...) to something more hackish and
more compatible with the new test.
* Some site-related scripts found Maintenance.php in a non-standard way.
Use the standard way.
* fileOpPerfTest.php called error_reporting(). Probably debugging code
left in; removed.
* Moved mediawiki.compress.7z registration from the class file to the
caller.
Change-Id: I1b1be90343a5ab678df6f1b1bdd03319dcf6537f
The expiry is returned as a separate key, 'watchlistexpiry', to match
other APIs, and because some clients might expect 'watched' to be a
boolean (or blank string depending on the formatversion).
Bug: T268834
Change-Id: I227d6ed42e70ba1ddec0139e8198f536dfba0b46
* parent::setUp() should be first, and ::tearDown()
should be last
* Move tests that directly extend PHPUnit\Framework\TestCase
to /unit
Change-Id: I1172855c58f4f52a8f624e6d596ec43beb8c93ff
The mandatory Unicode NFC normalization on API parameters was causing
spurious dirty diffs in VisualEditor/DiscussionTools when editors used
HTML entities to encode non-NFC codepoints, like  . Although
wikitext is (ought to be!) in NFC form, the output HTML may not be,
due to explicit entities in the wikitext.
This type is used in VisualEditor change
I0d34c9a01f1132c2616ed3392ea40d8b73e15325 to prevent Parsoid HTML from
being corrupted when it is round-tripped.
Bug: T266140
Change-Id: I2e78e660ba1867744e34eda7d00ea527ec016b71
Every user matching User::isBot would be affected and not longer adding
pages to the watchlist along to its (default) preferences.
To override this behaviour the bot user must change watch explizit.
The watchlist is mostly not useful for bots doing massive edits or
uploads. This reduce the grow of the watchlist tables at all and avoids
less users with massive entries unable to delete or clear.
Bug: T258108
Change-Id: If76127315767bde70147197c88e93f51ca70edaa
The InterwikiLoadPrefix hook isn't compatible with Parsoid, as it is
unidirectional and doesn't support enumerating all valid prefixes
(T270444). Set/reset $wgInterwikiCache to mock the interwiki table
for parserTests and other unit tests instead.
This is a soft deprecation, as the used-in-production
Extension:Interwiki still uses InterwikiLoadPrefix, although not in a
way that would break Parsoid (since $wgInterwikiCache is set in
production).
Bug: T270444
Change-Id: If2507017c99c4ee42c104a0890bc45a84d7239d5
If I try to run ApiMainTest with GlobalPreferences enabled, it takes
forever (or at least some number of minutes) to run. This is because
when @depends is used to transfer data from one test to another via
the return value, PHPUnit runs Enumerator::enumerate() on the data. When
it contains a reference to the active TestCase, PHPUnit ends up
iterating through its internal data structures, which takes a long time.
My conclusion is that @depends with a return value should be considered
harmful. Stop doing it in ApiMainTest. Stop recommending it in
SampleTest.
Change-Id: I63e94f2886a4ee4b3fd0ea6b19cd2fb67ba912de
Some User methods fail if they are called before $wgRequest is
set. But according to the Setup.php comment, it is only set for b/c.
The global request object can be lazy-initialised at any time.
This is sufficient to avoid T263911 (loss/obfuscation of the $wgServer
error message).
In tests, try to keep $wgRequest and RequestContext::$request in sync.
Introduce MediaWikiIntegrationTestCase::setRequest() which sets both at
once, and use that instead of setMwGlobals() or direct assignment.
BlockManagerTest was accidentally exploiting the fact that the global
context request and $wgRequest were separate objects. Making them the
same causes session cookies to appear in the response, breaking the
cookie counts. Use a new response for the test.
Bug: T263911
Bug: T245940
Change-Id: I2be99f7251a837bc6b62be0b152038157dec10f2
Several important extensions (Disambiguator, ProofreadPage, and
SoftRedirector) use the GetLinkColours hook to add additional CSS
classes to links on article pages. Parsoid previously relied on
backdoor knowledge of the way Disambiguator used the page property
table to support these, but they should be exported properly from the
API.
Bug: T237538
Change-Id: I945940aa872541d7e01f1e543ca854231c857fe2
Check the internal array of styles to determine whether
the method was inappropriately used to alter the array of styles and
if it was, call wfDeprecatedMsg().
Change-Id: I591b03c2e19d4b8cadfe220b498ae244d332f9fb
The commit did not really hard-deprecate overriding of setupSkinUserCss() as stated in the commit message, rather it removed core calls to setupSkinUserCss(), instantly breaking the many skins that still override it. It did not actually create a deprecation period for graceful migration.
As discussed in T267080, there is presently no way to hard-deprecate the override of a method.
This reverts commit 334cfeffd6.
Bug: T257990
Change-Id: I8f669ba30affc437800890c3a875994a9f2eb3c8
The limit tests would fail if there is apihighlimits for everyone
(group *)
mergeMwGlobalArrayValue does not do deep merge, it just overrides;
setting the '*' group to the minimum required to pass tests.
Change-Id: I95fba69af1845f28132370e9ded3350acdfdb8c4
In some tests mergeMwGlobalArrayValue should merge the empty array,
but when have settings in LocalSettings.php the result is not the empty
array as needed for the tests.
Just start with empty array to work on top of that.
1) ApiUserrightsTest::testAddAndRemoveGroups with data set "Add with
only remove permission" (array(array(), array('sysop')),
array(array('sysop'), array()), array('bot'))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
Array &0 (
0 => 'bot'
+ 1 => 'sysop'
)
with $wgAddGroups['bureaucrat'] = true; in LocalSettings.php
Change-Id: I092a3353aa65d53c84b765c04ec213ed8294b65d
Consistency aside, this is useful for grouping more tests
into separate threads when using directory based suites.
Bug: T50217
Change-Id: Ife9acd5990c4ae4a5fc18371559e93d7d86fb57d
This includes fixing some mistakes, as well as removing
redundant text that doesn't add new information, either because
it literally repeats what the code already says, or is actually
duplicated.
Change-Id: I3a8dd8ce57192deda8916cc444c87d7ab1a36515
For example, documenting the method getUser() with "get the User
object" does not add any information that's not already there.
But I have to read the text first to understand that it doesn't
document anything that's not already obvious from the code.
Some of this is from a time when we had a PHPCS sniff that was
complaining when a line like `@param User $user` doesn't end
with some descriptive text. Some users started adding text like
`@param User $user The User` back then. Let's please remove
this.
Change-Id: I0ea8d051bc732466c73940de9259f87ffb86ce7a
Redirects can form a loop but it was unaccounted for in this process.
Track seen pages to detect when a loop has formed. In the case of a
redirect loop the ApiPageSet will include the source page and ignore
the found redirects.
Bug: T264534
Change-Id: Ia5f4eeb9a4d90f02aceec3ed83bd4fc4a8a23ca4
Normalize watchlist expiry field so that it is set to
ConvertibleTimestamp and the expiry value validation is
handled by the WatchedItem class.
Bug: T260868
Bug: T260009
Change-Id: I3ef31900cfbe7bce23c5ebe1db777a5137ea6167
This data structure is sufficient to generate menus in
all Wikimedia deployed skins.
This new method will be used immediately in Example skin:
Ifb30a2c1314692c2869bd99c523e19c821be1f08
and Vector skin:
I5f7adc1840441b508ffee40139b85b64021789e6
Bug: T262098
Bug: T255924
Change-Id: I1a163cac0bff7620dcac50350cb6b93445a0cfbc
Rather than having to do DatabaseBlock calls directly,
and then ManualLogEntry calls to facilitate logging,
let's create a BlockUser service, capable of blocking users
and logging, optionally with permission checking.
This should make blocking users easier for developers,
for instance, AbuseFilter or CheckUser can easily
benefit from this commit.
Bug: T189073
Change-Id: Ifdced735b694b85116cb0e43dadbfa8e4cdb8cab
This made it impossible to correctly undo changes if revision IDs are out of
order with respect to timestamps.
Removes block of code responsible for creating this bug, and updates tests to reflect the change
Bug: T190285
Change-Id: Id5837ed958023bdbf544ae0f79e2d5e05c94cc64
To ensure the functionality doesn't break as part of
Ifdced735b694b85116cb0e43dadbfa8e4cdb8cab,
which refactors the block handling
Bug: T189073
Change-Id: I7cb1be6532c5b32a4a77924c42483d43a290c464
Following 23c3c70d7f, soft deprecate the static methods on
DatabaseBlock that have been moved to DatabaseBlockStore:
* ::insert
* ::delete
* ::update
* ::purgeExpired
Update calls to the deprecated methods from core.
Change-Id: I1272eb978594fd4f386bda12cbc24131ad7d882f
The PHPDoc for the editPage method stated that if $user is null,
the test sysop user will be used instead. This is not the case in
practice, as the method simply passes null to the doEditContent
method.
As Ammarpad has pointed out in a comment, this confusion is due to
I5a10163 assuming the sysop would be used, which is incorrect.
It's also probably better for the default user to be a "regular"
user, not a sysop, so I changed it to getTestUser() and noted this
in the doc block.
Depends-On: I7a79e0eaa1617e4d87a8d615a5391723c0e30b6a
Change-Id: I9f77474f40e0f6901aa2c6f846e471b822636aa5
Ensure the content we are trying to save and the base content have
identical content models before proceeding to save so as to forestall
Exception that may be thrown by ContentHandler if it founds they're not.
There are two cases where the models are allowed to differ: Edit that
undoes content model change or edit that's meant to explicitly change
the model. The logic for these is handled separately and may succeed
or fail, but exception will not be thrown.
Bug: T255700
Change-Id: I8782732bb0fc3059693cd7035b7ebb43fd71d333
Expiries are copied post-send so as to not slow anything down.
Note the expiry feature has been disabled in some unit tests that mock
database queries, simply because these tests are so hard to maintain and
are very fragile. The logic introduced with this patch is covered by the
integration tests.
Bug: T257259
Change-Id: I4223eaa6782a319fb684acf656f54b88a92e5288
* Move the method to SkinFactory and replaces usages.
* Inject $wgSkipSkins into the SkinFactory
Bug: T257993
Change-Id: I9869cf34c5e87cbad963f48db0649b3b7a252a4a
This introduces an ApiWatchlistTrait that refactors out common code
across APIs that allow you to watch pages. Some methods have been
migrated from ApiBase and changed completely, but codesearch suggests
they aren't being used outside the API modules in this patch.
Bug: T248512
Bug: T248514
Change-Id: Ia18627b9824dca81f44f0571e8420d89b7626cf6
The name change happened some time ago, and I think its
about time to start using the name name!
(Done with a find and replace)
My personal motivation for doing this is that I have started
trying out vscode as an IDE for mediawiki development, and
right now it doesn't appear to handle php aliases very well
or at all.
Change-Id: I412235d91ae26e4c1c6a62e0dbb7e7cf3c5ed4a6
There is native support for all of this now in PHP, thanks to changes
and additions that have been made in later versions. There should be no
need any more to ever use call_user_func() or call_user_func_array().
Reviewing this should be fairly easy: Because this patch touches
exclusivly tests, but no production code, there is no such thing as
"insufficent test coverage". As long as CI goes green, this should be
fine.
Change-Id: Ib9690103687734bb5a85d3dab0e5642a07087bbc
Migrate all callers of Hooks::run() to use the new
HookContainer/HookRunner system.
General principles:
* Use DI if it is already used. We're not changing the way state is
managed in this patch.
* HookContainer is always injected, not HookRunner. HookContainer
is a service, it's a more generic interface, it is the only
thing that provides isRegistered() which is needed in some cases,
and a HookRunner can be efficiently constructed from it
(confirmed by benchmark). Because HookContainer is needed
for object construction, it is also needed by all factories.
* "Ask your friendly local base class". Big hierarchies like
SpecialPage and ApiBase have getHookContainer() and getHookRunner()
methods in the base class, and classes that extend that base class
are not expected to know or care where the base class gets its
HookContainer from.
* ProtectedHookAccessorTrait provides protected getHookContainer() and
getHookRunner() methods, getting them from the global service
container. The point of this is to ease migration to DI by ensuring
that call sites ask their local friendly base class rather than
getting a HookRunner from the service container directly.
* Private $this->hookRunner. In some smaller classes where accessor
methods did not seem warranted, there is a private HookRunner property
which is accessed directly. Very rarely (two cases), there is a
protected property, for consistency with code that conventionally
assumes protected=private, but in cases where the class might actually
be overridden, a protected accessor is preferred over a protected
property.
* The last resort: Hooks::runner(). Mostly for static, file-scope and
global code. In a few cases it was used for objects with broken
construction schemes, out of horror or laziness.
Constructors with new required arguments:
* AuthManager
* BadFileLookup
* BlockManager
* ClassicInterwikiLookup
* ContentHandlerFactory
* ContentSecurityPolicy
* DefaultOptionsManager
* DerivedPageDataUpdater
* FullSearchResultWidget
* HtmlCacheUpdater
* LanguageFactory
* LanguageNameUtils
* LinkRenderer
* LinkRendererFactory
* LocalisationCache
* MagicWordFactory
* MessageCache
* NamespaceInfo
* PageEditStash
* PageHandlerFactory
* PageUpdater
* ParserFactory
* PermissionManager
* RevisionStore
* RevisionStoreFactory
* SearchEngineConfig
* SearchEngineFactory
* SearchFormWidget
* SearchNearMatcher
* SessionBackend
* SpecialPageFactory
* UserNameUtils
* UserOptionsManager
* WatchedItemQueryService
* WatchedItemStore
Constructors with new optional arguments:
* DefaultPreferencesFactory
* Language
* LinkHolderArray
* MovePage
* Parser
* ParserCache
* PasswordReset
* Router
setHookContainer() now required after construction:
* AuthenticationProvider
* ResourceLoaderModule
* SearchEngine
Change-Id: Id442b0dbe43aba84bd5cf801d86dedc768b082c7
I find this …->{…} curly bracket syntax surprising and confusing,
and try to avoid it as hard as I can. And that's indeed very, very
easy here. Note this really creates the exact same \stdClass object.
Change-Id: I612184be877b2aa404db9829a827cfcec2d253cf
This data is the same as the 'credits' data that is already compiled,
cached and made available via ExtensionRegistry.
Similar to various other configuration variables previously, the
$wgExtensionCredits variable is now also required to only be used
for providing input to the system (e.g. from LocalSettings.php,
or from legacy extension PHP entry points). It is no longer
supported to use this variable to reliably read out a full view
of all extension credits (specifically those registered via
extension.json).
Doing so had the downside of adding processing cost to every
web request, as well as taking one the single largest portion
of the ExtensionRegistry APCu cache key, which in PHP7+ incurs
a linear cost for every string value, string key, of every
(sub)array in this huge structure; and does to on every request
just in case something reads from $wgExtensionCredits.
The new method to access this information reliably is owned
by SpecialVersion for now (could be moved elsewhere). This
also makes the merging logic more testable and incurs it on-demand
rather than upfront.
Details:
* Move 'type' internally from NOT_ATTRIBS to CREDIT_ATTRIBS.
These two arrays behave identically for most purposes (they are
both used to mean "don't export this as a global attribute").
By placing it in CREDIT_ATTRIBS it becomes complete and makes
it easy to refer to in docs. Previously, extractCredits()
read the 'type' key outside the loop for CREDIT_ATTRIBS.
* Remove redundant code in ApiBase.php, that is now more obviously
redundant. Looks like a left-over or merge conflict mistake
from when ExtensionRegistry was first introduced.
Bug: T187154
Change-Id: I6d66c58fbe57c530f9a43cae504b0d6aa4ffcd0d
Introduces $wgWatchlistExpiryMaxDuration which is used instead of given
expiry if the given exceeds it. This is done in the storage layer. The
reasoning is to control the size of the watchlist_expiry table. Hence,
the max duration does not apply to indefinite expiries (since that would
mean now row in watchlist_expiry).
The frontend is responsible for disallowing expiries greater than the
max, if it choses to do so.
APIs should now pass in $wgWatchlistExpiryMaxDuration as the PARAM_MAX
setting for the 'expiry' type. They should also set PARAM_USE_MAX so
that the maximum value is used if it is exceeded.
Other APIs that watch pages will be updated in separate patches
(see T248512 and T248514).
Bug: T249672
Change-Id: I811c444c36c1da1470f2d6e185404b6121a263eb
