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
Advertize the fact that production errors resulting from corruption in
the content storage layer can be remedied using findBadBlobs.php.
Change-Id: I4eab773d0d2da57a545f211c2fef36146331c570
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
The fallback was previously done too late, re-trying based on the
same (empty) list of rows from the slots table.
Bug: T212428
Bug: T258666
Change-Id: Ie4aa2be8d73e597e79b7e0c3e12a539acee427a0
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 "main slot of revision not found in database" issue described
in T212428 has been ongoing for over a year and is throwing an
exception that is difficult to filter out during logstash triage.
Remove the revision id from the exception message, making it easier
to filter out. Instead, include this in new diagnostic logging.
Bug: T256127
Change-Id: I8f986daeab44c0b1c6b6e3fbf126b32053bd58e0
Once the Revision class is hard deprecated, we will still need to
run hooks that use Revision objects; even though the hooks will be
deprecated, Revision objects still need to be created for them.
To ensure that deprecation warnings aren't triggered by creating
Revision objects in deployed code, for deprecated hooks only
create the Revision object if the hook is registered.
All hooks that pass Revision objects have already been hard deprecated.
Bug: T246284
Change-Id: I7e718551822825cd390662bb201dd13e2e527e8b
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
Static code analyzer like phan-taint-check have problems to see the correct escaping in this case
Bug: T216348
Change-Id: I5fb6234ae4007ac7601e6584449e32a305fdb8b5
This is to protect against race conditions. There may be situations
where we already got the revision itself, but information is still
missing from the slots or content tables.
Bug: T212428
Bug: T252156
Change-Id: Id0cd85ae93616ad91b07afeccb766e8345fa7c9c
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
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
This script scans for content blobs that can't be loaded due to
database corruption, and can change their entry in the content table
to an address starting with "bad:". Such addresses cause the content
to be read as empty, with no log entry. This is useful to avoid
errors and log spam due to known bad revisions.
The script is designed to scan a limited number of revisions from a
given start date. The assumption is that database corruption is
generally caused by an intermedia bug or system failure which will
affect many revisions over a short period of time.
Bug: T205936
Change-Id: I6f513133e90701bee89d63efa618afc3f91c2d2b
This are fairly straight-forward shortcuts. Use the class directly.
Also remove comments about injecting the current time via callbacks
which is obsoleted by the wikimedia/timestamp now having support for
mocking the time, via Wikimedia\ConvertibleTimestamp::setFakeTime
or (subclass) MWTimestamp::setFakeTime.
Bug: T225730
Change-Id: I5b49f0a0f0ae2daca8f054243b1c4c0a94422653
They naturally belong in RevisionLookup. They return Revision,
so should be replaced anyway.
Bug: T246284
Change-Id: Ie5c478e4667ca0e773186b9cb8a319cd09145112
As well hard-deprecate RevisionStore::listRevisionSizes.
It was marked as @deprecated, but the release for the
deprecation was not specified. Let's say it's 1.35.
Additionally, in order to avoid temporary code duplication,
listRevisionSizes now uses getRevisionSizes and ignores the
database object injected into it. This is ok since we're
hard-deprecating the method and all the usages have been
removed.
Bug: T246284
Change-Id: Ifad1c25a0af892b88fce492b2d34c8cf71279b70
This should be the exact same. Its more a style change than anything.
So why do it then?
* I believe this is much less confusing than code mentioning a weird
"standard class". Barely anybody knows what this is, and what the
difference between "object" and "stdClass" is.
* The code is shorter.
* It's even faster. In my micro benchmark it's twice as fast.
Change-Id: I7ee0e8ae6d9264a89b6cd1dd861f0466ae620ccc
Part of the soft deprecated revision class, no known callers outside
of EditPage, which is updated, and tests, which hide the deprecation
Bug: T246284
Change-Id: I099cb93a12f3a1d9a720e18e3236374321ce7b0c
Added:
- ContentHandlerFactory
Tests:
- PHPUnit
Changed
- Calls of changed and deprecated
- DI for some service/api
Deprecated:
- ContentHandler::* then similar to ContentHandlerFactory
- ContentHandler::getForTitle
- ContentHandler::$handlers
Bug: T235165
Change-Id: I59246938c7ad7b3e70e46c9e698708ef9bc672c6
Released just now.
Many old suppressions can now be removed. Enabling the issue for
undeclared variables is left to do later, given that there are
roughly 200 warning.
Change-Id: I99462a1e9232d6e75022912e2df82bc2038476ef
getRevisionFromTimestamp was renamed to getRevisionByTimestamp
I3b049acff9313814a4ac448289d1aef88cb7f9df, and this comment was left
behind.
Change-Id: I9cf9056768937b3fb6f79ee5cb76fcacf1831f04
The SlotRecord passed to RevisionStore::insertRevisionOn may have its sha1 and size
already set by the caller. This is useful when restoring archived re
visions, but apparently sometimes goes wrong, leading to a revision
being recorded with the hash and size of its parent revision.
This patch introduces a sanity check that ensures that the revision's
hash and size matches the main slot's hash and size. If there are
additional slots, the check is skipped.
This does not fix the bug as reported, but should prevent any new
revisions to be inserted with mismatching meta-data, and provide us with
a stack trace that should help with finding the original cause of the
issue.
Bug: T239717
Change-Id: I4947731364925fef1c77cabcfd144fd2c07bf9db
In several places, we're including rc_timestamp or other fields in a
query selecting on rc_this_oldid because there was historically no index
on the column.
The needed index was created by I0ccfd26d and deployed by T202167, so
let's remove the hacks.
Bug: T139012
Bug: T239772
Change-Id: Ic99760075bde6603c9f2ab3ee262f5a2878205c7
It is converted to a valid sql string from the abstract database layer
Also use array for GROUP BY and column alias
Change-Id: I293a563607d115a42c8456c9b9ac66665d71d943
When restoring revisions that have an empty user name associated with
them, force "Unknown user" to be used instead, even if the actor table
has an entry for the empty user name.
Bug: T236624
Change-Id: I31edd5f7d89d9b43806ad18e96f5e93cff0f8c6f
* Move Title::{get,count}AuthorsBetween to RevisionStore
* Use it in REST count handler to to provide 'from' and 'to'
support for editors count.
Bug: T235896
Bug: T235666
Change-Id: Ib7947ae64b82871ceb4a3097a0ea03c0ba218c7c
* Add 'from' and 'to' query parameters to history count endpoint,
in case we are counting 'edits'. The support for parameters will be
expanded in the future, given that 'editors' is on the way.
* Refactor RevisionStore::countRevisionsBetween to be able to omit
starting or ending revision.
Bug: T235666
Change-Id: I1fc3f2d4e422f17ccfc99664ea35e210c13ade5f
In SCHEMA_COMPAT_READ_NEW mode, the 'text' flag should no longer be
supported by RevisionStore::getQueryInfo().
This reverts commit f7ca65da88.
Change-Id: I8b4dfc783724395586836c7565fd2d66dd653e80
Depends-On: Ibc943d757b97c144296e7116c1f9caf944c7f345
Depends-On: Ic799aa2dbe26de2e0cbe2be6218341a44ae051e8
Bug: T200918
If the rows were obtained using RevisionStore::getQueryInfo with
'page' flags, the revision row already contains the fields needed
to construct the Title without an additional database query.
Change-Id: Ie36c85871a8996a5706c80d286854a9c8363905f
getContentBlobsForBatch provides a more low level interface than
newRevisionsFromBatch, allowing bulk access to serialized content
without the need to instantiate Title, RevisionRecord, RevisionSlots,
SlotRecord, and Content objects.
Bug: T228675
Bug: T234034
Change-Id: I8481ad211e2d9f11bc10ea10c16e78b74538d95b
These callers just need to load some data from DB_MASTER.
Subsequent code needing that latest title data should also use the
required flags, rather than relying on flakey global cache state.
Change-Id: I53248ea4b5bf1cd953f956c41b8244831ec5ef04
* Inject settings and global instances as dependencies to the
ExternalStoreMedium instances. This includes the local wiki
domain, so that wfWikiId() calls are not scattered around.
* Create ExternalStoreAccess service for read/write logic.
* Deprecate the ExternalStore wrapper methods.
* Add some exception cases for bogus store URLs are used instead
of just giving PHP warnings and failing later.
* Make moveToExternal.php require the type/protocol to decide
which ExternalStoreMedium to use instead of assuming "DB".
* Convert logging calls to use LoggerInterface.
Change-Id: I40c3b5534fc8a31116c4c5eb64ee6e4903a6197a
These fields are passed to methods like LoadBalancer::getConnection() and are
already expected to be DB domains. Update various comments as well.
Fix a few minor IDEA warnings.
Change-Id: I7cf76700690aec449872a80d30b5ba540d2bf315
This introduces a way to construct a RevisionRecord based on a
known set of SlotRecords. To allow this to be used consistently
with the legacy revision schema, some tweaks had to be made
to getSlotsQueryInfo().
Bug: T220493
Change-Id: I5ea972bb07ca1cfb3a2ad8ef120aef77e460745c
This is slightly more robust and makes the intent much clearer
than random calling code checking getServerCount() all over the
place. In addition, this yields better separation of concern.
Also, cleanup the LoadBalancer constructer a bit and make the
validation a bit stricter.
Make some server index comparisons strict while at it.
Change-Id: Icc1a35bd65c6862ff81faa3ab9b2aa7cafe29443
Fix five instances of PhanPluginDuplicateConditionalNullCoalescing;
escape the rest for now.
Bug: T219114
Change-Id: Ic4bb30c43c5315ce6b878b37b432c6e219414f8b
Stop-gap solution for the problem described in T222212.
Force the User ID and Actor ID to zero for users loaded
from the database of another wiki, to prevent subtle data
corruption and confusing failure modes.
Bug: T222381
Change-Id: Ic585f972d61da136744d080df13d8eb1ecd04cf5
They belong in RevisionStore. This change removes the dependency on
Title for these methods and will assist in porting more code to
LinkTarget.
At the same time, deprecate the Title parameter to
RevisionLookup/RevisionStore getPreviousRevision/getNextRevision, and
add a $flags parameter to match the functionality of the Title versions.
Since code search turned up no callers that passed a Title outside core,
this variant is immediately hard-deprecated. The Title methods
themselves are only soft-deprecated.
Change-Id: I76bc6fd6ee1a9f35b5f29fa640824fb5da3bb78e
3e36ba655e added an option for passing a page ID to this method of
Revision, and e03787afd9 switched it to a Title and made it mandatory.
This behavior propagated to the method in RevisionStore. As far as I
can tell, the parameter does not help anything, but it can add a
database query to get the page ID if it's not cached, and impedes
conversion to LinkTarget. I can't figure out any reason to not
completely drop it. I've noted it as deprecated but still supported it
for now for compatibility -- I found one extension that does pass it.
(It's ignored, though, which theoretically would be a behavior change if
someone was passing a Title that didn't match the revision ID.)
While I was at it, I added the method to RevisionLookup, although it's
only used in later patches. Properly I should move that piece to a later
patch, but it didn't seem worth the effort.
I didn't change the Revision method, because the whole Revision class is
deprecated anyway.
Change-Id: I26ef5f2bf828f0f450633b7237d26d888e2c8773
* RevisionStore: phan is confused by the array not being the callable
* ApiContinuationManger: phan false positive
* ChronologyProtector: fix doc block
* MssqlBlob: phan doesn't like __construct returning things
* WikiPage: Remove Revision typehint that doesn't match doc block
Change-Id: I0661d97424d0ad6f7a3357542e79aceb6a4f6c64
Failure of getRevisionByTitle to pass its LinkTarget argument
to newRevisionFromRow resulted in a needless second instantiation
of the Title (an extra query).
Because newRevisionFromRow needs a Title, not just a LinkTarget,
it is unfortunately necessary to call Title::newFromLinkTarget
on it for now -- however this does not involve a DB lookup and
is on track to be fixed with revisions to the Title class.
Bug: T206498
Change-Id: Ic6f98d8fbf66d85121668571c17e148efc5ec2be
Created class RevisionStoreCacheRecord to avoid returning stale
cached revision visibility and actor data when changes were
made after the cache was populated for that revision.
Bug: T216159
Change-Id: Iabed80e06a08ff0793dfe64db881cbcd535cb13f
