[ES] CI build fails for EntitySchema after the inclusion of SpamBlacklist
Open, Needs TriagePublicBUG REPORT
Actions

Assigned To

None

Authored By

	AudreyPenven_WMDE
	Mon, Oct 7, 4:20 PM

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

Event Timeline

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

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMon, Oct 7, 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.Mon, Oct 7, 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 {

[ES] CI build fails for EntitySchema after the inclusion of SpamBlacklistOpen, Needs TriagePublicBUG REPORTActions

Description

Event Timeline

[ES] CI build fails for EntitySchema after the inclusion of SpamBlacklist
Open, Needs TriagePublicBUG REPORT
Actions