Callers should not catch an unchecked exception, so it doesn't belong
in a function signature. Unchecked exceptions indicate a coding error,
which by definition the code will not be able to handle correctly.
If any of these exceptions were supposed to be in response to an edge
case, user input, or initial conditions, then they should be changed
to a runtime error. If the exception class cannot be changed, then
the annotation should include a comment explaining its purpose and
prognosis.
Bug: T240672
Change-Id: I2e640b9737cb68090a8e1cb70067d1b74037d647
This touches various production classes and maintenance scripts.
The code should do the exact same as before. The main benefit is that
the syntax avoids any repetition.
Change-Id: I5c552125469f4d7fb5b0fe494d198951b05eb35f
And deprecated aliases for the the no namespaced classes.
ReplicatedBagOStuff that already is deprecated isn't moved.
Bug: T353458
Change-Id: Ie01962517e5b53e59b9721e9996d4f1ea95abb51
Versions are changed in 8e940c4f21,
but that makes the version wrong
Follow-Up: I7f85d931d3b79da23e87b4e5692b2e14be8fcaa0
Change-Id: Iae43725b8e0fffc4d44bf57f6227334b41290bd9
This patch introduces a namespace declaration for the
MediaWiki\Json to FormatJson and establishes a class
alias marked as deprecated since version 1.43.
Bug: T353458
Change-Id: I5e1311e4eb7a878a7db319b725ae262f40671c32
This patch introduces a new namespace declaration,
MediaWiki\Xml and adds Xml and XmlSelect to it
and establishes class aliases marked as deprecated
since version 1.43.
Bug: T353458
Change-Id: I45cccd540b6e15f267d3ab588a064fbeb719d921
Or just remove it. It's not needed when the structure is already an
array. PHP will happily initialize multi-dimentional arrays when
needed.
Change-Id: I93845e8d6f870d147bd55cfe3827bc94b375d0ba
This makes the code easier to read to avoid jumps and improves
encapsulation by not allowing the rest of the class having access to the
logic. Of course, if needed then some code can be refactored out again
but for now let's not do YAGNI.
Change-Id: Ic37524e386fc04fd67e33768417ff8425f85b0ca
empty() only makes sense when the expression it checks is possibly
undefined, otherwise it's equivalent to a truthiness check with the
additional downside of suppressing errors when it's not wanted.
Replace it with simple truthiness checks, using strict comparison when
that seems to help with polymorphic variables.
These were caught by a bespoke phan plugin.
Change-Id: I70b629dbf9e47cf3ba48ff439b18f19e839677f4
The design principle for SelectQueryBuilder was to make the chained
builder calls look as much like SQL as possible, so that developers
could leverage their knowledge of SQL to understand what the query
builder is doing.
That's why SelectQueryBuilder::select() takes a list of fields, and by
the same principle, it makes sense for UpdateQueryBuilder::update() to
take a table. However with "insert" and "delete", the SQL designers
chose to add prepositions "into" and "from", and I think it makes sense
to follow that here.
In terms of natural language, we update a table, but we don't delete a
table, or insert a table. We delete rows from a table, or insert rows
into a table. The table is not the object of the verb.
So, add insertInto() as an alias for insert(), and add deleteFrom() as
an alias for delete(). Use the new methods in MW core callers where
PHPStorm knows the type.
Change-Id: Idb327a54a57a0fb2288ea067472c1e9727016000
Possible after ALL_ROWS support was added (7cf42e26)
Also remove return from clear(), it is not documented on the interface
Bug: T340065
Change-Id: Ibc41c92148c317862e8e3ea9583e2e28cfa0f61f
I did this using a script written on top of antlr4 parser so it doesn't
have some clean ups a human would do but it's pretty nice already.
Bug: T330640
Change-Id: I608566700c6d737ee986bf47dda87effc69614d6
== Background ==
In 2012, commit afe46f1403 (Id7e9b59c7e) added libs/GenericArrayObject
along with an (unused) abstract GenericArrayObjectTest case.
The same code was also added to the Wikibase extension with
change 6347b35a55cd (Ifa7f1dc702).
The code was then factored out from Wikibase into the wmde/Diff
library.
In 2013, GenericArrayObject was removed from wmde/Diff in the commit
at https://github.com/wmde/Diff/commit/d9c2bd5c140e2a783fd42298db6c.
== This change ==
Remove the GenericArrayObject indirection from SiteList as there exist
nothing outside SiteList refering to it in Codesearch Everywhere, and
even in SiteList much of the code in GenericArrayObject is overridden,
unused, or otherwise needlessly indirect.
Change-Id: Ifea09c5de50af1616058d8baa9037db273dfb0e5
These exceptions are not documented with @throws and they're really not
meant to be caught.
Bug: T86704
Change-Id: I07f32e42c6fd4bc8785bac91547858f15a9fc2a8
All callers available in codesearch pass in a $path (and for all callers
except getFileUrl()’s own test, that path is 'api.php'); omitting the
path hasn’t worked correctly for almost ten years (see change
I5ed7e169c9, commit 779df10df7).
Change-Id: I50819efb66fd72f2b5a1cd83d5cdd3a148742299
Follows-up I5ed7e169c9753 (779df10d, T319219) which introduced a new
use of MWException, as per T86704.
While at it, also remove a few unhelpful `@throws` annotations that
do nothing other than save an outdated IDE scan of what throw
statements internally exist. These aren't meant to be caught and thus
aren't of use to consumers of these docs as per T321683.
Change-Id: I4ef964342de80eb2c1dec99cd6258727e6cd1a2c
Its unfortunate that this class has encoded these unusual terms
in its stable API but we can at least use more common terminology
and document what these things actually are.
Virutally every word in the explaination "a full file path [is] a site
url and relative file path", meant something different from its usual
meaning, namely the full URL to an entry point script under /w/.
Change-Id: I15f1c9d56f61e622734bb3ddc304025ad4bfc169
* Recognise CachingSiteStore as the internal class that it is.
* Clean up the file header and move ingroup annotation, similar to
other commits under
<https://gerrit.wikimedia.org/r/q/message:ingroup+owner:Krinkle>.
The one here was particularly odd as it placed the license inside
the class doc block rather than the file doc block.
* Remove unused $cacheKey and $cacheTimeout constructor args. They
were also not covered by any unit tests.
* Avoid use of `empty()` as per
<https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP>
* Use a more conventional cache key in CachingSiteStore, keeping
the dynamic part for a later segment and keeping the first segment
explicitly tied to a single concrete key group. This also improves
ergonomics of cache statistics e.g. in Grafana and makes the cache
key easier to locate in codesearch/grep, and more recognisable,
e.g. starting with the code component ("site").
Change-Id: I541ad62551ab76127d70ef665272fd1d590ab556
Site::getPath() has returned null since change I08ffa6a970 (commit
a00337c3f8), almost ten years ago; I think on the whole it’s safer to
fix the prose documentation and keep the @return and actual returned
value as they are, rather than the other way around.
MediaWikiSite::getPageUrl() compared the $filePath to false instead of
null, so the comparison would always be true; but on closer inspection,
I think this code was supposed to check $path, not $filePath, in the
first place. The only reason this didn’t come up so far is that all
known callers pass in a custom path (specifically, all non-test callers
pass in 'api.php'), so the $path = false default was never used.
To avoid a deprecation warning in PHP 8.1 when passing a null $filePath
into str_replace(), throw in that case instead – the function isn’t
documented to return string|null, and none of the callers seem prepared
to handle a null return as far as I can tell, so throwing seems better
than returning null. Tests that produce a null $filePath need to be
fixed to complete the Site object (compare Wikibase change If20688411e).
Bug: T319219
Change-Id: I5ed7e169c9753486c765fd816d1b21016c5c1def
Depends-On: If20688411e3f0108b0c0ba3b91e5cc264b70a208
Note that null triggers the following for php 8.1:
PHP Deprecated: strrev(): Passing null to parameter #1 ($string) of
type string is deprecated
Got a few of these in WikiMapTest::testMakeForeignLink(), caused by a
single call in DBSiteStore.
Bug: T289926
Change-Id: I0b29963ec0e2e8a38a66e250e81fbc16e42465ab
This is mostly about adding return types to methods that implement PHP
interfaces, and not passing null to core functions that want a string.
After this patch, and an update to return types in RemexHtml,
tests/phpunit/integration/ has no more errors than in PHP 8.0.
Bug: T289879
Bug: T289926
Change-Id: Ia424f5cc897070f4188ae126b5bf6a1f552db0e1
This edition brought to you by:
grep -ERIn $(grep -o "'[A-Za-z0-9_]*'" includes/MainConfigNames.php | tr
"\n" '|' | sed 's/|$/\n/') includes/
I only corrected a fraction of the results provided by that command. I'm
submitting the partial patch now so it doesn't bitrot.
Bug: T305805
Change-Id: If1918c0b3d88cdf90403921e4310740e206d6962
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
* Enforce the ban on is_resource in phpcs
* In OrderedStreamingForkController, the comment was incorrect. I
confirmed using a small test script that if the child closes one end
of a socket pair, the other end will still be open, and is_resource()
will still return true, and fclose() will not fail. The issue was
introduced in c82d30d19c, it was not present in the
CirrusSearch copy of the class.
* Allow is_resource() for debug logging.
* Allow is_resource() for parameter validation where a stream may be
passed to a function, since there is no alternative.
Bug: T260735
Change-Id: I59a752f7bb4b043ddfb2434b52a02f9221bf54db
As discussed in the task, there is no simple replacement for this
function.
Reduces core PHPUnit error count from 121 to 2.
Bug: T268847
Change-Id: Ia8697e1c3ba369346b39ef50d94ea8b132eb9c80
This allows the backend to do whatever it can to ensure atomicity, and
is also easier to read, since the intent is obvious.
Change-Id: Ibbfecd92a2c6d9a5997ca37ea101e068bd1e8202
