It is quite rare that a notice like this is best fixed so close to
the caller. More often than not, low-level code like this doesn't
have a reasonable way to handle it and it's a genuine issue but one
that a higher-level should prevent instead.
In this case though it seems fair to handle here because an empty
string is actually, according to most l10n-related classes, a valid
message key. It can't exist, but it is a valid key.
See also MessageCache::get() which does not consider it an error and
similarly returns false. That method never calls normalizeKey with
an empty string though. The error came from ApiQueryAllMessages.
I don't think it should be ApiQueryAllMessages's concern to normalize
this key or otherwise hardcode its non-existence over there.
Bug: T300792
Change-Id: I8a8eed66704862bb3a645ef5941a5264e65bde4c
Don't catch and discard exceptions from the RequestTimeout library,
except when the exception is properly handled and the code seems to be
trying to wrap things up.
In most cases the exception is rethrown. Ideally it should instead be
done by narrowing the catch, and this was feasible in a few cases. But
sometimes the exception being caught is an instance of the base class
(notably DateTime::__construct()). Often Exception is the root of the
hierarchy of exceptions being thrown and so is the obvious catch-all.
Notes on specific callers:
* In the case of ResourceLoader::respond(), exceptions were caught for API
correctness, but processing continued. I added an outer try block for
timeout handling so that termination would be more prompt.
* In LCStoreCDB the Exception being caught was Cdb\Exception not
\Exception. I added an alias to avoid confusion.
* In ImageGallery I added a special exception class.
* In Message::__toString() the rationale for catching disappears
in PHP 7.4.0+, so I added a PHP version check.
* In PoolCounterRedis, let the shutdown function do its thing, but
rethrow the exception for logging.
Change-Id: I4c3770b9efc76a1ce42ed9f59329c36de04d657c
The context is already called, just reuse it.
This avoids unstub of $wgLang in ParserOptions constructor
Change-Id: I3ac6abf6237db7c1c09740c1fec1224779e86888
Use RequestContext::getMain()->getUser() instead.
Given that there is an explicit check if the user is safe
to load, this should be fine - if the RequestContext
doesn't already have a user, it'll create one via
User::newFromSession() and then the isSafeToLoad()
call will fail.
Bug: T243708
Change-Id: I217f169b2f5c5a0a337011e383b5171f7c7a2975
array_fill_keys() was introduced in PHP 5.2.0 and works like
array_flip() except that it does only one thing (copying keys) instead
of two things (copying keys and values). That makes it faster and more
obvious.
When array_flip() calls were paired, I left them as is, because that
pattern is too cute. I couldn't kill something so cute.
Sometimes it was hard to figure out whether the values in array_flip()
result were used. That's the point of this change. If you use
array_fill_keys(), the intention is obvious.
Change-Id: If8d340a8bc816a15afec37e64f00106ae45e10ed
Replace the many references to $wgLanguageCode with a cached copy of the
code of the content language passed to the constructor. The references
to $wgLanguageCode in this class were just meant to be a shortcut for
$wgContLang->getCode().
Change-Id: I60c61aaef0abe6df79ab39f123206d8aae044113
Loading all messages with getSubitemList() takes about 10ms per
language and loads an array with ~20k elements. When messages in many
languages are requested, this causes an OOM.
So, use getSubitemList() only when isMainCacheable() is called from
loadFromDB(). Remove the second parameter in that case, since it was
always the same.
In the getMsgFromNamespace() case, use getSubitem() to check the
specific message for existence. Have the caller specify the language, in
order to share a subitem cache entry with usual previous
getMessageForLang() call.
Bug: T247223
Change-Id: I6369f307b6bf74bd4aeb1d6e4c41d6e59e403703
This has effectively been the case since 1.35; this just cleans up the
remaining code which assumed it still needed to explicitly call
Parser::firstCallInit() on a newly-constructed Parser.
Bug: T250444
Change-Id: I340947c721172f12ff413322b4283627c0b0b3a4
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
This saves two queries per local overridden message when filling the
cache
(one from RevisionStore::loadSlotRecords, one from
SqlBlobStore::fetchBlobs)
Remove 'user' join, as nothing is getting user information from this
records. Join against comment and actor also not needed, but there is no
option to avoid the join.
Change-Id: Ieb5086f81310092f573ef95d997bf0bc99c30952
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 class was last used in ~2008 as @tstarling was developing the
original wikitext Parser. It has since code-rotted and wouldn't work
as a drop in for the Parser class any more anyway. We'll probably
(re)invent something similar when we eventually switch Message
rendering from the legacy parser to Parsoid, but this existing code
isn't a good starting point for that; we'll need to tackle T236812
(splitting Parser into a base class) first.
Last meaningful change to ParserDiffTest:
350b498b9f
Code search:
https://codesearch.wmflabs.org/search/?q=ParserDiffTest&i=nope&files=&repos=
Bug: T236811
Change-Id: I98f1ef8ad296791a810bd8b10343f8640fd23c5e
Replace usage of Title in LanguageConverter with LinkTarget which
is more light weighted and provides just the props needed in Language.
Bug: T226834
Change-Id: I02a386bd9898e83c773cbd3d738d347d08f52c11
Done:
* Replace LanguageConverter::newConverter by LanguageConverterFactory::getLanguageConverter
* Remove LanguageConverter::newConverter from all subclasses
* Add LanguageConverterFactory integration tests which covers all languages by their code.
* Caching of LanguageConverters in factory
* Make all tests running (hope that's would be enough)
* Uncomment the deprecated functions.
* Rename FakeConverter to TrivialLanguageConverter
* Create ILanguageConverter to have shared ancestor
* Make the LanguageConverter class abstract.
* Create table with mapping between lang code and converter instead of using name convention
* ILanguageConverter @internal
* Clean up code
Change-Id: I0e4d77de0f44e18c19956a1ffd69d30e63cf51bf
Bug: T226833, T243332
The query for this result already contains a join to page table,
reuse that information and avoid a Title::newFromId in the RevisionStore
Change-Id: Ie02c238292778c3048cc950e37f51f04fc238fea
Repeating the variable name doesn't do anything. Documentation
generators don't need it. It's more stuff to read that doesn't add new
information. And it can become outdated.
Note there are two types of @var docs. When used inline (and not on a
class property) the variable name is needed.
Change-Id: If5a520405efacd8cefd90b878c999b842b91ac61
These call the deprecated Language::getLocalisationCache() method and
thereby indirectly access MediaWikiServices. Callers should be changed
to inject a LocalisationCache and use that directly.
There are only a few callers in code search, so it didn't seem worth
adding convenience methods to LocalisationCache. The one caller that was
already using DI was MessageCache, and I injected LocalisationCache
there.
Bug: T201405
Change-Id: I01919fba5685fc5e0a31f739714f125a22de8939
Before c962b48056, the 'loadedLanguages' array was used to track
which languages were loaded and in the cache, with 'cache' being a
simple array. In that commit, the 'cache' array also started being used
for incomplete datasets, which didn't affect 'loadedLanguages'.
Then in 97e86d934b, the 'loadedLanguages' array was removed in favour
of checking keys on 'cache' directly, and 'cache' was converted to
MapCacheLRU.
This led to problem where partially loaded data was mistaken for being
full datasets (fatal error, T208897). This was fixed in a5c984cc59,
by bringing back the 'loadedLanguages' array, which fixed the issue from
the POV of partially loaded data.
However, this then exposed a new problem. The 'cache' data can be evicted
by MapCacheLRU, whereas 'loadedLanguages' is not aware of that. Thus it
claims languages are loaded that sometimes aren't. (This only affects web
requests where more than 5 language codes are involved, per MapCacheLRU.)
Fix this by re-removing the 'loadedLanguages' array, this time
strengthening the 'cache' key check to not just check that the root key
exists, but that it is in fact holding the full dataset as generated by
MessageCache::load(). The 'VERSION' key appears to be a good proxy for
that.
Bug: T230690
Change-Id: I1162a3857376aa37e5894ae3c8be84a2295782a3
When reporting exceptions that occur during initialization, wgUser may
be null. Don't die when that happens.
Change-Id: I65d5a17d80f9021e28a218c7a5a17e399bc7ce98
This variable has never been set to anything other than the default value of
24 hours as introduced in 2003 (r2203, r2204; or 036ff960ce, edf6b38626).
The variable has never changed in core, it's not overridden at WMF,
and MessageCache is not constructed anywhere other than ServiceWiring.php
anywhere in repos on Wikimedia Gerrit, indexed by MediaWiki Codesearch,
or any GitHub-hosted repository (incl Wikia repos and WikiHow mirrors).
I've also checked all GitHub-hosted repos for boilerplates and/or public
settings files from devs or prod, and couldn't find any example of
this being overridden (after filtering out copies of the core files
themselves). Rather than having to support potentially hard-to-predict
interactions betweeen caching layers by checking its state, make it
a constant so we can code reason about it more easily.
Change-Id: Ie2e139001aae3ac54b509d94a3d917bb408eaca0
