Make getServer() always return a string, as documented, even with new
Database::NEW_UNCONNECTED handles that have yet to call open(). If the
'host' parameter to __construct() is ''/null, getServer() now returns
'localhost' instead of null. This avoids problems like fatal errors in
calls to TransactionProfiler::recordConnection().
Use Database constants for "connectionParams" field keys for better
static analysis.
Also:
* Add Database::getServerName() method that returns "readable" server
names in the style of LoadBalancer::getServerName(). Note that the
"hostName" field is already passed in from LoadBalancer.
* Migrate most getServer() callers to getServerName() for easier
debugging and more readable logging.
* Also, normalize Database/LoadBalancer SPI logging context to use
"db_server" and reduce logging code duplication in LoadBalancer.
Bug: T277056
Change-Id: I00ed4049ebb45edab1ea07561c47e226a423ea3b
When we find that a revision we loaded doesn't belong to the expected
title, log the error and continue, instead of failing hard.
This could happen if a caller to e.g. getRevisionById supplied a Title that is
plain wrong. In this case, we should ideally throw an IllegalArgumentException.
However, it is more likely that we encountered a race condition during a page
move (T268910, T279832) or database corruption (T263340). That situation
should not be ignored, but we can allow the request to continue in a reasonable
manner without breaking things for the user.
Note that this patch does not resolve the mentioend bugs. It only
reduces impact for users. We still need to find out when and why we are
gettign mismatching titles.
Bug: T259022
Bug: T268910
Bug: T263340
Bug: T279832
Change-Id: Ia64ad3b8a6e3c3348bc0a276d528ead127c6b0e0
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
Code that needs to store an actor ID in the database to
represent a UserIdentity, or needs to construct a UserIdentity based on
an actor ID loaded from the database, should use the ActorNormalization
service.
Note: The getActorId() method is removed from the UserIdentity interface,
but all concrete classes continue to support it for now.
UsererIdentityValue::getActorId() is hard deprecated and should
be removed in 1.37. It always returns 0.
User::getActorId() is not deprecated at this point.
Bug: T274179
Depends-On: Id2b3ddf6a2a7cdf90f8936a69148d2cce6fde237
Change-Id: I9925906d11e47efaec3c1f48d5cb3f9896a982c1
Providing a PageIdentity to getRevisionById removes the need
to look up page related data, and to construct a Title.
Bug: T275531
Bug: T273284
Depends-On: I8d0dc30ecd0c0c65c337170d271c2e2531914686
Change-Id: I683362ccd669acf2aadf56853afeb1ca6dee6f8c
The new getPage() handles cross-wiki access correctly. It's not a direct
replacement for getTitle(), since it is private.
getTitle() cannot work properly for RevisionStore instances connected to
a sister-wiki. It was already discouraged, and is nearly unused outside
of RevisionStore. Remainign usages should be replaced with
RevisionRecord::getPage().
Note that PageIdentityValue is not lazy loading, while Title is.
Code in newRevisionFromRowAndSlots() was adjusted to take advantage of
any fields from the page table that may already be present from a join,
to avoid an additional query to the page table.
Bug: T275531
Bug: T273284
Change-Id: If4525d76a578b92dbcfe1f5150028f7a28811119
This is micro-optimization of closure code to avoid binding the closure
to $this where it is not needed.
Created by I25a17fb22b6b669e817317a0f45051ae9c608208
Change-Id: I0ffc6200f6c6693d78a3151cb8cea7dce7c21653
The following methods have beed converted:
::getRevisionByTitle
::getRevisionByTimestamp
::newRevisionFromArchiveRow
::newRevisionsFromBatch
::getKnownCurrentRevision
::getFirstRevision
Appropriate tests were adjusted to test both old interface (with
Title) and new interface (with PageIdentityValue)
Bug: T272886
Change-Id: Ica8da21d2413df885365475eaa498214edbcfd1d
As we convert the RevisionRecord to using Authority,
we no longer need Title instances, so we can convert
that to PageIdentity.
Ideally, we'd part away from using Title at all, but:
1. For foreign wikis PageIdentity has stronger validation,
so calling PageIdentity getId() on Title will break things.
There's still a lot of code depending on lax Title guarantees,
so we keep it.
2. A lot of code still depends on Title, so we try to pass it
through even if we don't nesessarily need to, to save cost
on recreating it later on.
Bug: T271458
Depends-On: I287400b967b467ea18bebbb579e881a785a19158
Change-Id: I63d9807264d7e2295afef51fc9d982447f92fcbd
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
RevisionStoreDbTestBase is mistakenly passing an array instead of an integer as
a query flags parameter in many places, which eventually gets passed to
DBAccessObjectUtils that uses it with a bitwise operator. PHP < 8 handles this
silently, but PHP >= 8 throws "TypeError: Unsupported operand types: array &
int" here.
Bug: T248925
Change-Id: I4e29b2709fe5e6d583727d45e005b3f16a1e3f4d
As the same string values are used by multiple methods in
RevisionStore, factoring them out to constants would make sense.
The only other appearances of these strings in core are some
long-deprecated methods that are probably not worth updating.
Change-Id: I85fe6ab96716f237291202dbb1ebefb90bb9f0da
Motivation:
This method would be useful to many extensions and other parts of
code that want to obtain a list of revisions in a given range. This
is quite hard to do properly, as the limit conditions are rather
complicated, so it would be useful to have a tested, working method
that does this.
This would be especially useful when obtaining a list of reverted
revisions. Currently extensions are provided with the newest and
oldest reverted revision ids, so this method could be used in such
a case. I think at least Echo, FlaggedRevs and EventBus could make
use of it. I also plan to use it in code marking reverted edits with
mw-reverted change tag.
Design:
I've decided for this method to return only ids, as this would suit
many (but not all) use cases and is far simpler than returning entire
revisions. If the method was to return RevisionRecords, it'd have to
have a few extra parameters (which parts of the revision should we
load? which slots should be loaded? should the content be loaded as
well?) and it would quickly grow to a huge supermethod that has way
too many responsibilities and is extremely hard to test reliably.
Bug: T258951
Change-Id: I0b82e8fa8bd7cb8aa599376a72ebc0a3b320c0e9
This causes RevisionStore to use FallbackContent instances to represent
content for which no content handler is defined.
This may happen when loading revisions using a model that was defined
by an extension that has since been uninstalled.
Bug: T220594
Bug: T220793
Bug: T228921
Change-Id: I5cc9e61223ab22406091479617b077512aa6ae2d
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
Clean up some technical debt; use MutableRevisionRecord instead of
manually constructing a Revision from an array, remove last uses of
RevisionStoreDbTestBase::revisionToRow and remove the method.
Each file can be reviewed separately (except that the removal of
revisionToRow depends on replacing its usage)
Bug: T246284
Change-Id: I0bdc069b21a5c41ef8f9e972c5b17ff189d4a741
Use assertRevisionRecordsEqual, not assertRevisionRecordMatchesRevision
also revisionRecordToRow, not revisionToRow
RevisionStoreDbTestBase::assertRevisionRecordMatchesRevision can thus
be removed
Bug: T254952
Change-Id: If5d7d4e01947d9f8a2ed51c950e750e82380930c
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
We're finally to the point of making the massive alter to the `revision`
table that we've been building up to for 2.5 years now.
Changes here are:
* Drop `rev_text_id`, `rev_content_model`, and `rev_content_format` that
MCR obsoleted.
* Drop `ar_text_id`, `ar_content_model`, and `ar_content_format` that
MCR obsoleted.
* Replace `rev_comment` with `rev_comment_id`.
* Replace `rev_user` and `rev_user_text` with `rev_actor`, plus
associated index changes.
Future patches will make the code changes to migrate data from
`revision_actor_temp` and `revision_comment_temp` into the new
`revision` columns.
Bug: T251343
Bug: T184615
Bug: T215466
Change-Id: I18071a2fe45907a0cf1b0fefebd96a97a2dacb7b
Change Ic6f98d8fb changed RevisionStore::getRevisionByTitle to convert
the passed LinkTarget to a Title using Title::newFromLinkTarget, to
address bug T206498.
Unfortunately, this breaks when RevisionStore is operating in the context
of a foreign wiki. In this scenario, when RevisionStoreRecord::__construct
calls Title::getArticleID on the Title instance to perform a sanity check
of the page ID against the page ID reference stored in the revision table
for the given revision record, then Title::getArticleID will lazily fetch
the page ID on the local, rather than the foreign wiki. This results in
an InvalidArgumentException thrown by RevisionStoreRecord::__construct.
This patch changes the logic to only resolve the LinkTarget to a Title
when operating in the context of the local wiki, similar to the checks
that RevisionStore::getTitle and friends are already performing.
It also adds a regression test to verify that
RevisionStore::getRevisionByTitle does not try to access the local wiki
DBLoadBalancer when called in the context of a foreign wiki.
Bug: T248756
Change-Id: I76090fa2907c2db8e23a62959346db531c6d9cb2
When for some reason we can't determine the title for a revision
in the batch, this should not trigger a fatal TypeError, but handled
gracefully, with helpful information included in the error message.
Bug: T205936
Change-Id: I0c7d2c1fee03d1c9208669a9b5ad66612494a47c
See codesearch - only deployed call outside of core is in flaggedrevs,
and already passes a RevisionRecord:
https://codesearch.wmflabs.org/deployed/?q=-%3EupdateRevisionOn%5C(&i=nope&files=&repos=
Also fixed a use of Revision::newFromId in orphans.php
Bug: T249561
Bug: T249021
Change-Id: I5933a278de8645b7005c11026c87ae27c0373770
Sometimes, an edit is done with a Title object that has gone
out of sync with the database after a page move. In this case,
we should re-load the current page ID from the database,
instead of failing the update hard.
Bug: T246720
Bug: T204793
Bug: T221763
Bug: T225366
Change-Id: If7701205ec2bf4d4495349d3e67cf53d32ee8357
