Some less trivial cases. Also update variable names.
This doesn't really change anything, but I hope it helps promote
getConnectionProvider() as the common way to do this.
Follow-up to 8604c384f6.
Change-Id: I6657d783375fac5c7fa856b884ff1fb09285e94c
UTPage is a badly named page that should not be used directly. In fact,
soon it will no longer be guaranteed to exist (T342428). Tests should
create the fixtures they need, using titles/summaries that are actually
meaningful and that make it easy to identify what test is responsible
for creating a given page.
In ApiSetNotificationTimestampIntegrationTest, make separate assertions
because the order of the two pages in the result set is not stable
(ApiPageSet::initFromTitles has no ORDER BY).
Change-Id: Iecfb75e16deaa2f682afd916f58d8c548ee1cbac
This reduces the acceptable forms for hook handlers to three things:
* a callable (in the form of a string, an array, or a closure)
* an object, which is expected to have a public "on" method that
matches the hook name.
* an array containing an object spec in the "handler" key, for use
with ExtensionRegistry.
All other forms will trigger a deprecation warning.
Bug: T339167
Depends-On: I980f2d45e6bb8c6a04058e68c758f71bbcf709de
Depends-On: Ieae405f70caa01d84602583cc214b0ee3fadc796
Depends-On: If15df4b598c02ed9bda5eea0ae89a16ebbf4f2e2
Depends-On: Id70276fa1e1821bd400dc0ae5cea722a21d524d5
Change-Id: I83bc81d1b3033c38b9313884a9c70a187fdde227
Unicode flag should be used in the regular expression to supply a
long enough string without broken multi-byte characters, since the
truncateForVisual() method would mesure and truncate by code points.
The second regexp may not need the flag, but add it for consistency.
Also, cleanup some messy quotation marks.
Bug: T333653
Change-Id: Iadc7b38c60817a737b72b461e45d10198e04005b
Just methods where adding "static" to the declaration was enough, I
didn't do anything with providers that used $this.
Initially by search and replace. There were many mistakes which I
found mostly by running the PHPStorm inspection which searches for
$this usage in a static method. Later I used the PHPStorm "make static"
action which avoids the more obvious mistakes.
Bug: T332865
Change-Id: I47ed6692945607dfa5c139d42edbd934fa4f3a36
Prepare phpunit tests for a world where $wgHooks behaves like other
config variables: on-the-fly manipulation doesn't have an effect,
service objects need to be reset after changing it.
Change-Id: I34385f4c52febfe1c91bd7ea4100c06d1331adc8
Results in passing a user where previously the fallback
to $wgUser was being used, mostly in tests.
Bug: T255507
Change-Id: Iabe24315b23c0ad1272353186425e71974528d23
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
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
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
I found this while working on T249167. Searching for something like
"[" or copy-pasting something like "[[Foo]]" simply returns
*all* pages, in alphabetical order. This is unexpected.
A worst-case example is
api.php?action=opensearch&search=Test
vs.
api.php?action=opensearch&search=Test]
While the first example behaves as expected (it will either find
pages with this prefix, or nothing if there is none), the second
will return the entire database.
Expected: It should either find nothing, or pages that start with
"Test".
Note this patch intentionally does not fix one edge-case: Input
like "[" will still return all pages. I left this untouched for
now because it might be used as a feature somewhere.
Note you can not test this on Wikimedia wikis because they don't
use this search backend. Which is just another reason to fix this.
On Wikipedia, searching for "[[Test]]" works just fine.
Change-Id: I877297f69ca560460766e1035e02b009919054e1
Done with `composer fix` and suppressing the rest (i.e. sniffs for
global variables, which for core should be suppressed anyway).
Additionally, add `-p` to `phpcbf`, as otherwise it just seems stuck.
Change-Id: Ide8d6cdd083655891b6d654e78440fbda81ab2bc
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
This patch tries to make assertions in tests more readable by using more
self-documenting assertions as provided by modern PHPUnit versions. Among
a few others, these two main changes are done:
* I found a lot of assertions with the expected value being the *second*
parameter. I did not changed all of them. Only some that can be replaced
with assertNull() and such.
* I try to replace all `assertTrue( is_…() )` with dedicated assertions.
Change-Id: I1fc72188fbd0edacf13886e7f9a9eacbd85f13c2
And start indicating that hooks relying on this data might become
unreliable as this data is only populated by SearchDatabase search
engines.
This information was only populated by SearchDatabase implementations
and due to bad initial design of SearchResult[Set] (now fixed) it forced
users of these classes to carry this information for the sole purpose of
highlighting.
Because SearchEngine can now own their SearchResult[Set] implementations
nothing that is engine specific should be exposed outside of these
specific implementations.
If there are some logic that still requires access to such list of terms
they should be made engine specific by guarding their code against
instanceof SqlSearchResult.
Change-Id: I38b82c5e4c35309ee447edc3ded60ca6a18b247a
Depends-On: I53fe37c65c7940f696c1e184125e01e592a976e4
Also make the update() methods of the subclasses use DB_MASTER as they
should. This avoids read-only errors.
In addition, avoid passing a dummy argument of null in some cases
within SearchEngineFactory::create(). Fix some dynamic calls to
static methods too.
Change-Id: Id94f34994b0f9c18e23ef30cb2fe895e6dedd09c
These methods are very similar there should be no need to have
two differents way to extract the namespace prefix.
Bug: T198860
Change-Id: I22802278452559d35a3d8f6068549c1fef1a5e86
This mimics how full text works by silenty dropping results returned from
search that no longer exist. This could be because the search index is slightly
out of sync with reality, or the search engine could simply be broken.
Only silent from the users perspective. We maintain a count in statsd of
the number of titles dropped. This can be monitored over time to
recognize any increases.
Bug: T115756
Change-Id: I2f29d73e258cd448a14d35a2b4902a4fb6f61c68
The funky iteration here was at best annoying. Switch
it over to an iterator based approach with appropriate
BC code to simulate the old iteration style.
Depends-On: I19a8d6621a130811871dec9335038797627d9448
Change-Id: I9fccda15dd58a0dc35771d3b5cd7a6e8b02514a0
Partially revert I61dc536 that broke phrase search support.
Fix phrase search by making explicit that there are two
kind of legalSearchChars() usecases :
- the chars allowed to be part of the search query (including special
syntax chars such as " and *). Used by SearchDatabase::filter() to
cleanup the whole query string (the default).
- the chars allowed to be part of a search term (excluding special
syntax chars) Used by search engine implementaions when parsing with
a regex.
For future reference:
Originally this distinction was made "explicit" by calling directly
SearchEngine::legalSearchChars() during the parsing stage. This was
broken by Iaabc10c by enabling inheritance.
This patch adds a new optional param to legalSearchChars to make this
more explicit.
Also remove the function I introduced in I61dc536 (I wrongly assumed
that the disctinction made between legalSearchChars usecases was due
to a difference in behavior between indexing and searching).
Added more tests to prevent this from happening in the future.
Bug: T167798
Change-Id: Ibdc796bb2881a2ed8194099d8c9f491980010f0f
I think the bug was introduced during a cleanup in Iaabc10c.
I don't think that " should be part of the legalSearchChars at query
time, it seems to break the regex.
The strategy here is to distinguish legalSearchChars used query time vs
the ones used at index time by introducing:
SearchEngine::legalSearchCharsForUpdate()
Bug: T167798
Change-Id: I61dc53665e26d3c6c48caed78dd3bbde9a33def7
The default will remain PHPUnit 4.x due to PHP 5.5 support.
But, we should allow developers to run tests with newer PHPUnit
versions which are noticably faster (especially for code coverage
reports).
* <https://github.com/sebastianbergmann/phpunit/wiki/Release-Announcement-for-PHPUnit-5.4.0>
PHPUnit 5 deprecates the getMock() shortcut for getMockBuilder()->getMock().
It instead introduces the shortcut createMock() which has better defaults
than getMockBuilder(). For example, it sets 'disableArgumentCloning' and
other things by default.
Going forward, code should either use getMockBuilder directly and configure
it using the setter methods (instead of the confusing variadic arguments
of getMock) or simply use the new minimalistic createMock method. This patch
backports the createMock method to MediaWikiTestCase so that we can start
using it.
Change-Id: I091c0289b21d2b1c876adba89529dc3e72b99af2
ContentHandler implementations were not including fields
defined by their parent ContentHandler classes.
merge method is added to the SearchIndexFieldDefinition
mock in SearchEngineTest, to allow merges of fields
in the way that SearchIndexFieldDefition implementation does.
Change-Id: Id04a51528f566da2666bad0394a2f61c949c69b4
This reduces the runtime of database-bound tests by about 40%
(on my system, from 4:55 to 2:47; results from Jenkins are
inconclusive).
The basic idea is to call addCoreDBData() only once, and have
a addDBDataOnce() that is called once per test class, not for
every test method lie addDBData() is. Most tests could be
trivially be changed to implement addDBDataOnce() instead of
addDBData(). The ones for which this did not work immediately
were left out for now. A closer look at the tests that still
implement addDBData() may reveal additional potential for
improvement.
TODO: Once this is merged, try to change addDBData() to
addDBDataOnce() where possible in extensions.
Change-Id: Iec4ed4c8419fb4ad87e6710de808863ede9998b7
