Aude has uploaded a new change for review.
https://gerrit.wikimedia.org/r/180877
Change subject: Fix EditFilter bug with creating items and properties
......................................................................
Fix EditFilter bug with creating items and properties
Too often, other extensions make the assumption that
context title is the target title (of the new content),
and then run into bugs when the EditFilter hook is triggered.
We set title / id later, after the hook is run.
This patch sets a 'fake' temporary context title
that has a valid namespace for the given entity type,
to help avoid such problems with the EditFilter hook.
With the LinkBegin hook, it is transformed into a link
for the actual special page (Special:NewItem or NewProperty)
It would be better if other extensions didn't make such
assumption, and this is just a workaround for this
recurring issue.
Added / reworked the tests to ensure they catch this.
Bug: T78645
Bug: T61788
Change-Id: Ie71ebdebd18cfd217daae6db1881951d356f88b0
---
M repo/Wikibase.hooks.php
M repo/includes/EditEntity.php
M repo/tests/phpunit/includes/EditEntityTest.php
3 files changed, 104 insertions(+), 49 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/77/180877/1
diff --git a/repo/Wikibase.hooks.php b/repo/Wikibase.hooks.php
index 399fafd..e6ee002 100644
--- a/repo/Wikibase.hooks.php
+++ b/repo/Wikibase.hooks.php
@@ -26,6 +26,7 @@
use SearchResult;
use Skin;
use SkinTemplate;
+use SpecialPageFactory;
use SpecialSearch;
use SplFileInfo;
use Title;
@@ -731,23 +732,7 @@
global $wgTitle;
wfProfileIn( __METHOD__ );
- $entityContentFactory =
WikibaseRepo::getDefaultInstance()->getEntityContentFactory();
-
- //NOTE: the model returned by Title::getContentModel() is not
reliable, see bug 37209
- $contentModel = $target->getContentModel();
-
- // we only want to handle links to Wikibase entities
differently here
- if ( !$entityContentFactory->isEntityContentModel(
$contentModel ) ) {
- wfProfileOut( __METHOD__ );
- return true;
- }
-
- // if custom link text is given, there is no point in
overwriting it
- // but not if it is similar to the plain title
- if ( $html !== null && $target->getFullText() !== $html ) {
- wfProfileOut( __METHOD__ );
- return true;
- }
+ $entityNamespaceLookup =
WikibaseRepo::getDefaultInstance()->getEntityNamespaceLookup();
// $wgTitle is temporarily set to special pages Title in case
of special page inclusion! Therefore we can
// just check whether the page is a special page and if not,
disable the behavior.
@@ -759,6 +744,28 @@
return true;
}
+ if ( !$entityNamespaceLookup->isEntityNamespace(
$target->getNamespace() ) ) {
+ wfProfileOut( __METHOD__ );
+ return true;
+ }
+
+ $targetText = $target->getText();
+
+ if ( SpecialPageFactory::exists( $targetText ) ) {
+ $target = Title::makeTitle( NS_SPECIAL, $targetText );
+ $html = Linker::linkKnown( $target );
+
+ wfProfileOut( __METHOD__ );
+ return true;
+ }
+
+ // if custom link text is given, there is no point in
overwriting it
+ // but not if it is similar to the plain title
+ if ( $html !== null && $target->getFullText() !== $html ) {
+ wfProfileOut( __METHOD__ );
+ return true;
+ }
+
// The following three vars should all exist, unless there is a
failurre
// somewhere, and then it will fail hard. Better test it now!
$page = new WikiPage( $target );
diff --git a/repo/includes/EditEntity.php b/repo/includes/EditEntity.php
index 4730e51..c49f63f 100644
--- a/repo/includes/EditEntity.php
+++ b/repo/includes/EditEntity.php
@@ -5,6 +5,7 @@
use DerivativeContext;
use Hooks;
use Html;
+use IContextSource;
use InvalidArgumentException;
use MWException;
use ReadOnlyError;
@@ -13,6 +14,7 @@
use Title;
use User;
use Wikibase\DataModel\Entity\Entity;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\Lib\Store\EntityRevisionLookup;
use Wikibase\Lib\Store\EntityStore;
@@ -761,21 +763,13 @@
return;
}
- if ( !$this->isNew() ) {
- $context = clone $this->context;
-
- $title = $this->getTitle();
- $context->setTitle( $title );
- $context->setWikiPage( new WikiPage( $title ) );
- } else {
- $context = $this->context;
- }
-
// Run edit filter hooks
$filterStatus = Status::newGood();
$entityContentFactory =
WikibaseRepo::getDefaultInstance()->getEntityContentFactory();
$entityContent = $entityContentFactory->newFromEntity(
$this->newEntity );
+
+ $context = $this->getContextForEditFilter( $this->newEntity );
if ( !wfRunHooks( 'EditFilterMergedContent',
array( $context, $entityContent, &$filterStatus,
$summary, $this->getUser(), false ) ) ) {
@@ -791,6 +785,28 @@
$this->status->merge( $filterStatus );
}
+ /**
+ * EntityDocument $entity
+ *
+ * @return IContextSource
+ */
+ private function getContextForEditFilter( EntityDocument $entity ) {
+ if ( !$this->isNew() ) {
+ $context = clone $this->context;
+ $title = $this->getTitle();
+ } else {
+ $context = $this->context;
+ $entityType = $entity->getType();
+ $namespace = $this->titleLookup->getNamespaceForType(
$entityType );
+ $title = Title::makeTitle( $namespace, 'New' . ucfirst(
$entityType ) );
+ }
+
+ $context->setTitle( $title );
+ $context->setWikiPage( new WikiPage( $title ) );
+
+ return $context;
+ }
+
protected function applyPreSaveChecks() {
if ( $this->hasEditConflict() ) {
if ( !$this->fixEditConflict() ) {
diff --git a/repo/tests/phpunit/includes/EditEntityTest.php
b/repo/tests/phpunit/includes/EditEntityTest.php
index 0daedb9..4638952 100644
--- a/repo/tests/phpunit/includes/EditEntityTest.php
+++ b/repo/tests/phpunit/includes/EditEntityTest.php
@@ -4,6 +4,7 @@
use FauxRequest;
use HashBagOStuff;
+use IContextSource;
use RequestContext;
use Status;
use Title;
@@ -15,7 +16,9 @@
use Wikibase\DataModel\Term\Fingerprint;
use Wikibase\EditEntity;
use Wikibase\Lib\Store\EntityTitleLookup;
+use Wikibase\Repo\Content\EntityContentFactory;
use Wikibase\Repo\Store\EntityPermissionChecker;
+use Wikibase\Repo\WikibaseRepo;
/**
* @covers Wikibase\EditEntity
@@ -46,27 +49,18 @@
}
protected function setUp() {
- global $wgGroupPermissions, $wgHooks;
+ global $wgGroupPermissions;
parent::setUp();
$this->permissions = $wgGroupPermissions;
$this->userGroups = array( 'user' );
-
- if ( empty( $wgHooks['EditFilterMergedContent'] ) ) {
- // This fake ensures EditEntity::runEditFilterHooks is
run and runtime errors are found
- $wgHooks['EditFilterMergedContent'] = array( null );
- }
}
protected function tearDown() {
- global $wgGroupPermissions, $wgHooks;
+ global $wgGroupPermissions;
$wgGroupPermissions = $this->permissions;
-
- if ( $wgHooks['EditFilterMergedContent'] === array( null ) ) {
- unset( $wgHooks['EditFilterMergedContent'] );
- }
parent::tearDown();
}
@@ -126,22 +120,24 @@
/**
* @param MockRepository $repo
* @param Entity $entity
- * @param User $user
+ * @param EntityTitleLookup $titleLookup
+ * @param User|null $user
* @param bool $baseRevId
*
* @param null|array $permissions map of actions to bool, indicating
which actions are allowed.
*
* @return EditEntity
*/
- protected function makeEditEntity( MockRepository $repo, Entity
$entity, User $user = null, $baseRevId = false, $permissions = null ) {
+ protected function makeEditEntity( MockRepository $repo, Entity $entity,
+ EntityTitleLookup $titleLookup, User $user = null, $baseRevId =
false, $permissions = null
+ ) {
$context = new RequestContext();
$context->setRequest( new FauxRequest() );
- if ( !$user ) {
+ if ( $user === null ) {
$user = User::newFromName( 'EditEntityTestUser' );
}
- $titleLookup = $this->newTitleLookupMock();
$permissionChecker = $this->newEntityPermissionCheckerMock(
$permissions );
$edit = new EditEntity( $titleLookup, $repo, $repo,
$permissionChecker, $entity, $user, $baseRevId, $context );
@@ -295,7 +291,8 @@
}
// save entity ----------------------------------
- $editEntity = $this->makeEditEntity( $repo, $entity, $user,
$baseRevisionId );
+ $titleLookup = $this->newTitleLookupMock();
+ $editEntity = $this->makeEditEntity( $repo, $entity,
$titleLookup, $user, $baseRevisionId );
$conflict = $editEntity->hasEditConflict();
$this->assertEquals( $expectedConflict, $conflict,
'hasEditConflict()' );
@@ -313,6 +310,35 @@
$this->assertArrayEquals( $expectedValue,
$actualValue, false, true );
}
}
+ }
+
+ public function testEditFilterMergedContentHook_withNewEntity() {
+ $hooks = array_merge(
+ $GLOBALS['wgHooks'],
+ array( 'EditFilterMergedContent' => array() )
+ );
+
+ $testCase = $this;
+
+ $hooks['EditFilterMergedContent'][] = function( IContextSource
$context ) use( $testCase ) {
+ $entityContentFactory =
WikibaseRepo::getDefaultInstance()->getEntityContentFactory();
+
+ $page = $context->getWikiPage();
+ $title = $page->getTitle();
+ $contentModel = $title->getContentModel();
+ $testCase->assertTrue(
$entityContentFactory->isEntityContentModel( $contentModel ) );
+ };
+
+ $this->setMwGlobals( array(
+ 'wgHooks' => $hooks
+ ) );
+
+ $titleLookup =
WikibaseRepo::getDefaultInstance()->getEntityTitleLookup();
+
+ $item = Item::newEmpty();
+ $item->setLabel( 'en', 'omg' );
+ $editEntity = $this->makeEditEntity( $this->makeMockRepo(),
$item, $titleLookup );
+ $editEntity->attemptSave( "Testing", EDIT_NEW, false );
}
private function fingerprintToPartialArray( Fingerprint $fingerprint ) {
@@ -347,7 +373,8 @@
$entity = $entity->copy();
$entity->getFingerprint()->setLabel( 'en', 'Trust' );
- $editEntity = $this->makeEditEntity( $repo, $entity, $user,
$baseRevId );
+ $titleLookup = $this->newTitleLookupMock();
+ $editEntity = $this->makeEditEntity( $repo, $entity,
$titleLookup, $user, $baseRevId );
$editEntity->getLatestRevision(); // make sure EditEntity has
page and revision
$this->assertEquals( $baseRevId !== false,
$editEntity->doesCheckForEditConflicts(), 'doesCheckForEditConflicts()' );
@@ -443,7 +470,8 @@
$user = self::getUser( "EditEntityTestUser" );
$item = $this->prepareItemForPermissionCheck( $user, $repo,
$create );
- $edit = $this->makeEditEntity( $repo, $item, $user, false,
$permissions );
+ $titleLookup = $this->newTitleLookupMock();
+ $edit = $this->makeEditEntity( $repo, $item, $titleLookup,
$user, false, $permissions );
$edit->checkEditPermissions();
$this->assertEquals( $expectedOK, $edit->getStatus()->isOK() );
@@ -455,12 +483,13 @@
*/
public function testAttemptSavePermissions( $permissions, $create,
$expectedOK ) {
$repo = $this->makeMockRepo();
+ $titleLookup = $this->newTitleLookupMock();
$user = self::getUser( "EditEntityTestUser" );
$item = $this->prepareItemForPermissionCheck( $user, $repo,
$create );
$token = $user->getEditToken();
- $edit = $this->makeEditEntity( $repo, $item, $user, false,
$permissions );
+ $edit = $this->makeEditEntity( $repo, $item, $titleLookup,
$user, false, $permissions );
$edit->attemptSave( "testing", ( $item->getId() === null ?
EDIT_NEW : EDIT_UPDATE ), $token );
@@ -611,6 +640,7 @@
$this->setUserGroups( $user, $groups );
$items = array();
+ $titleLookup = $this->newTitleLookupMock();
foreach ( $edits as $e ) {
$name = $e[ 'item' ];
@@ -628,7 +658,7 @@
$item->setLabel( 'en', $label );
- $edit = $this->makeEditEntity( $repo, $item, $user );
+ $edit = $this->makeEditEntity( $repo, $item,
$titleLookup, $user );
$edit->attemptSave( "testing", ( $item->getId() ===
null ? EDIT_NEW : EDIT_UPDATE ), false );
$this->assertEquals( $expectedOK,
$edit->getStatus()->isOK(), var_export( $edit->getStatus()->getErrorsArray(),
true ) );
@@ -665,7 +695,8 @@
$user = self::getUser( "EditEntityTestUser" );
$item = Item::newEmpty();
- $edit = $this->makeEditEntity( $repo, $item, $user );
+ $titleLookup = $this->newTitleLookupMock();
+ $edit = $this->makeEditEntity( $repo, $item, $titleLookup,
$user );
// check valid token --------------------
if ( $token === true ) {
@@ -720,7 +751,8 @@
$repo->updateWatchlist( $user, $item->getId(), $watched
);
}
- $edit = $this->makeEditEntity( $repo, $item, $user );
+ $titleLookup = $this->newTitleLookupMock();
+ $edit = $this->makeEditEntity( $repo, $item, $titleLookup,
$user );
$status = $edit->attemptSave( "testing", $new ? EDIT_NEW :
EDIT_UPDATE, false, $watch );
$this->assertTrue( $status->isOK(), "edit failed: " .
$status->getWikiText() ); // sanity
--
To view, visit https://gerrit.wikimedia.org/r/180877
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie71ebdebd18cfd217daae6db1881951d356f88b0
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: wmf/1.25wmf13
Gerrit-Owner: Aude <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
