[ES] CI build fails for EntitySchema after the inclusion of SpamBlacklist
Closed, ResolvedPublicBUG REPORT
Actions

Description

Steps to replicate the issue (include links if applicable):

for any commit to EntitySchema, the quibble-vendor-mysql-php74-noselenium build fails. Here's an example: https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php74-noselenium/44850/console

The errors can also be reproduced on a local mediawiki installation:

load the SpamBlacklist extension by adding wfLoadExtension( 'SpamBlacklist' ); to LocalSettings.php
run tests with this command: mw docker mediawiki composer phpunit extensions/EntitySchema/tests/phpunit/

What happens?:
Many tests fail with an error in /extensions/SpamBlacklist/includes/Hooks.php:127

Here's one example:

17:19:38 13) EntitySchema\Tests\Integration\DataAccess\MediaWikiRevisionEntitySchemaUpdaterTest::testUpdateSchemaNameBadge_comment with data set "aliases changed" (EntitySchema\Services\Converter\NameBadge Object (...), EntitySchema\Services\Converter\NameBadge Object (...), 'entityschema-summary-update-s...liases', 'new alias, other')
17:19:38 array_keys() expects parameter 1 to be array, null given
17:19:38 
17:19:38 /workspace/src/extensions/SpamBlacklist/includes/Hooks.php:127
17:19:38 /workspace/src/includes/HookContainer/HookContainer.php:159
17:19:38 /workspace/src/extensions/EntitySchema/src/DataAccess/MediaWikiRevisionEntitySchemaUpdater.php:353
17:19:38 /workspace/src/extensions/EntitySchema/src/DataAccess/MediaWikiRevisionEntitySchemaUpdater.php:202
17:19:38 /workspace/src/extensions/EntitySchema/tests/phpunit/integration/DataAccess/MediaWikiRevisionEntitySchemaUpdaterTest.php:1011
17:19:38 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:106

Details

	Subject	Repo	Branch	Lines +/-
	Add static return type for `ParserOutput::getExternalLinks`	mediawiki/core	master	+2 -2
	[DNM] test CI	mediawiki/extensions/EntitySchema	master	+0 -0

Customize query in gerrit

Event Timeline

AudreyPenven_WMDE created this task.Oct 7 2024, 4:20 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 7 2024, 4:20 PM

The failing code is here:

SpamBlacklist::Hooks.php::onEditFilterMergedContent()

$links = LinkFilter::getIndexedUrlsNonReversed( array_keys( $pout->getExternalLinks() ) );

In the affected tests, $pout is a PHPUnit mock object; since the getExternalLinks() method isn’t configured by the test (the test probably didn’t even mock the ParserOutput, it’s just an automatically returned value from another mock), it defaults to returning null, which is invalid.

My usual preferred solution for cases like this is to add a static return type, so that PHPUnit knows which type the mock should return (example); however, in this case I’m not sure if that’s a good idea. ParserOutput::getExternalLink() returns $this->mExternalLinks by reference; what if there’s some existing code which reassigns a non-array value to that reference (and, consequently, to $this->mExternalLinks)? Currently, this might possibly work (depending on what other code does with the external links, I guess), but if we add static types, then this will start to throw TypeErrors, either during the assignment (if we also add array to ParserOutput’s private $mExternalLinks = [];), or in any subsequent getExternalLink() call (if the private property remains untyped).

But then again, maybe this is overly paranoid of me. $mExternalLinks is documented to contain an array, after all – there’s not really a reason to expect anyone to assign something else there.

Well, this alternative approach works to fix the tests (at least locally):

diff --git a/tests/phpunit/integration/DataAccess/MediaWikiRevisionEntitySchemaUpdaterTest.php b/tests/phpunit/integration/DataAccess/MediaWikiRevisionEntitySchemaUpdaterTest.php
index 46bf78dca9..a78d3e5db2 100644
--- a/tests/phpunit/integration/DataAccess/MediaWikiRevisionEntitySchemaUpdaterTest.php
+++ b/tests/phpunit/integration/DataAccess/MediaWikiRevisionEntitySchemaUpdaterTest.php
@@ -20,6 +20,7 @@
 use MediaWiki\HookContainer\HookContainer;
 use MediaWiki\Page\PageIdentityValue;
 use MediaWiki\Page\WikiPageFactory;
+use MediaWiki\Parser\ParserOutput;
 use MediaWiki\Request\FauxRequest;
 use MediaWiki\Revision\RevisionLookup;
 use MediaWiki\Revision\RevisionRecord;
@@ -27,6 +28,7 @@
 use MediaWiki\Status\Status;
 use MediaWiki\Storage\PageUpdater;
 use MediaWiki\Storage\PageUpdateStatus;
+use MediaWiki\Storage\PreparedUpdate;
 use MediaWiki\Tests\User\TempUser\TempUserTestTrait;
 use MediaWiki\User\User;
 use MediaWikiIntegrationTestCase;
@@ -101,6 +103,11 @@ private function getPageUpdaterFactoryExpectingComment(
 	private function getPageUpdaterFactory( PageUpdater $pageUpdater = null ): MediaWikiPageUpdaterFactory {
 		$wikiPage = $this->createConfiguredMock( WikiPage::class, [
 			'newPageUpdater' => $pageUpdater ?? $this->createMock( PageUpdater::class ),
+			'getCurrentUpdate' => $this->createConfiguredMock( PreparedUpdate::class, [
+				'getParserOutputForMetaData' => $this->createConfiguredMock( ParserOutput::class, [
+					'getExternalLinks' => [],
+				] ),
+			] ),
 		] );
 		$wikiPageFactory = $this->createConfiguredMock( WikiPageFactory::class, [
 			'newFromTitle' => $wikiPage,

But I don’t like it. This reaches deep into MediaWiki internals, for no obvious reason – this is just the code path that SpamBlacklist happens to call.

Perhaps adding a return type is better after all…

Lucas_Werkmeister_WMDE added projects: Wikidata, Wikidata Dev Team (Wikidata.org Slice), ci-test-error (WMF-deployed Build Failure), SpamBlacklist.Oct 7 2024, 4:30 PM

Lucas_Werkmeister_WMDE added a project: EntitySchema.

And just to confirm – alternatively, this patch in MediaWiki core also fixes the error locally:

diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php
index 2fe99ae2ee..b7b7e74b03 100644
--- a/includes/parser/ParserOutput.php
+++ b/includes/parser/ParserOutput.php
@@ -198 +198 @@ class ParserOutput extends CacheTime implements ContentMetadataCollector {
-	private $mExternalLinks = [];
+	private array $mExternalLinks = [];
@@ -798 +798 @@ public function &getFileSearchOptions() {
-	public function &getExternalLinks() {
+	public function &getExternalLinks(): array {

ArthurTaylor claimed this task.Oct 8 2024, 7:48 AM

ArthurTaylor moved this task from In Task Breakdown to In Development on the Wikidata Dev Team (Wikidata.org Slice) board.

Change #1078601 had a related patch set uploaded (by Arthur taylor; author: Arthur taylor):

[mediawiki/core@master] Add static return type for `ParserOutput::getExternalLinks`

https://gerrit.wikimedia.org/r/1078601

gerritbot added a project: Patch-For-Review.Oct 8 2024, 7:57 AM

Change #1078606 had a related patch set uploaded (by Arthur taylor; author: Arthur taylor):

[mediawiki/extensions/EntitySchema@master] [DNM] test CI

https://gerrit.wikimedia.org/r/1078606

I staged @Lucas_Werkmeister_WMDE's proposed change and it seems to work and fix the issue. Should we try and see if we can get that upstream to Mediawiki?

ArthurTaylor moved this task from In Development to In Peer Review on the Wikidata Dev Team (Wikidata.org Slice) board.Oct 8 2024, 9:57 AM

ArthurTaylor moved this task from In Peer Review to Waiting for Deployment Window on the Wikidata Dev Team (Wikidata.org Slice) board.Oct 10 2024, 10:37 AM

Change #1078601 merged by jenkins-bot:

[mediawiki/core@master] Add static return type for `ParserOutput::getExternalLinks`

https://gerrit.wikimedia.org/r/1078601

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.27; 2024-10-15).Oct 10 2024, 11:00 AM

CI succeeded on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EntitySchema/+/1077969 \o/

[ES] CI build fails for EntitySchema after the inclusion of SpamBlacklistClosed, ResolvedPublicBUG REPORTActions

Description

Details

Event Timeline

[ES] CI build fails for EntitySchema after the inclusion of SpamBlacklist
Closed, ResolvedPublicBUG REPORT
Actions