jenkins-bot has submitted this change and it was merged.
Change subject: Bug: [DB]Mis-reporting content length
......................................................................
Bug: [DB]Mis-reporting content length
We have been mis-reporting content length and the change in content
length between two revisions due to worries of having to fetch
the old content, decompress it and then run the html through parsoid just
to get the number of characters in it.
This patch updates the AbstractRevision model to track the wikitext length
explicitly, along with changing the places that calculate this length
to use the new value. Also adds a variety of tests for the places this
is changing code within.
This database change has been applied in WMF production.
Bug: 60646
Change-Id: I6996acf45aebbf6fda2ce85f0f10d3d9d1690fa6
---
M Hooks.php
M autoload.php
A db_patches/patch-add-revision-content-length.sql
M flow.sql
M includes/Data/Listener/RecentChangesListener.php
M includes/Data/Storage/RevisionStorage.php
M includes/Formatter/AbstractQuery.php
M includes/Formatter/RecentChanges.php
M includes/Formatter/RevisionFormatter.php
M includes/Model/AbstractRevision.php
M includes/SpamFilter/ContentLengthFilter.php
A maintenance/FlowUpdateRevisionContentLength.php
A tests/phpunit/Formatter/RevisionFormatterTest.php
M tests/phpunit/Model/PostRevisionTest.php
M tests/phpunit/PostRevisionTestCase.php
A tests/phpunit/SpamFilter/ContentLengthFilterTest.php
16 files changed, 485 insertions(+), 46 deletions(-)
Approvals:
Matthias Mullie: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Hooks.php b/Hooks.php
index a608d50..7683142 100644
--- a/Hooks.php
+++ b/Hooks.php
@@ -154,6 +154,7 @@
$updater->addExtensionTable( 'flow_ext_ref',
"$dir/db_patches/patch-add-linkstables.sql" );
$updater->dropExtensionTable( 'flow_definition',
"$dir/db_patches/patch-drop_definition.sql" );
$updater->dropExtensionField( 'flow_workflow',
'workflow_user_ip', "$dir/db_patches/patch-drop_workflow_user.sql" );
+ $updater->addExtensionField( 'flow_revision',
'rev_content_length', "$dir/db_patches/patch-add-revision-content-length.sql" );
require_once __DIR__.'/maintenance/FlowUpdateRecentChanges.php';
$updater->addPostDatabaseUpdateMaintenance(
'FlowUpdateRecentChanges' );
@@ -181,6 +182,9 @@
require_once __DIR__.'/maintenance/FlowPopulateLinksTables.php';
$updater->addPostDatabaseUpdateMaintenance(
'FlowPopulateLinksTables' );
+ require_once
__DIR__.'/maintenance/FlowUpdateRevisionContentLength.php';
+ $updater->addPostDatabaseUpdateMaintenance(
'FlowUpdateRevisionContentLength' );
+
return true;
}
diff --git a/autoload.php b/autoload.php
index 57a96e6..f35fa90 100644
--- a/autoload.php
+++ b/autoload.php
@@ -304,6 +304,7 @@
'Flow\\Tests\\FlowActionsTest' => __DIR__ .
'/tests/phpunit/FlowActionsTest.php',
'Flow\\Tests\\FlowTestCase' => __DIR__ .
'/tests/phpunit/FlowTestCase.php',
'Flow\\Tests\\Formatter\\FormatterTest' => __DIR__ .
'/tests/phpunit/Formatter/FormatterTest.php',
+ 'Flow\\Tests\\Formatter\\RevisionFormatterTest' => __DIR__ .
'/tests/phpunit/Formatter/RevisionFormatterTest.php',
'Flow\\Tests\\Handlebars\\FlowPostMetaActionsTest' => __DIR__ .
'/tests/phpunit/Handlebars/FlowPostMetaActionsTest.php',
'Flow\\Tests\\HookTest' => __DIR__ . '/tests/phpunit/HookTest.php',
'Flow\\Tests\\Import\\ConverterTest' => __DIR__ .
'/tests/phpunit/Import/ConverterTest.php',
@@ -338,6 +339,7 @@
'Flow\\Tests\\Repository\\TreeRepositorydbTest' => __DIR__ .
'/tests/phpunit/Repository/TreeRepositoryDbTest.php',
'Flow\\Tests\\SpamFilter\\AbuseFilterTest' => __DIR__ .
'/tests/phpunit/SpamFilter/AbuseFilterTest.php',
'Flow\\Tests\\SpamFilter\\ConfirmEditTest' => __DIR__ .
'/tests/phpunit/SpamFilter/ConfirmEditTest.php',
+ 'Flow\\Tests\\SpamFilter\\ContentLengthFilterTest' => __DIR__ .
'/tests/phpunit/SpamFilter/ContentLengthFilterTest.php',
'Flow\\Tests\\SpamFilter\\SpamBlacklistTest' => __DIR__ .
'/tests/phpunit/SpamFilter/SpamBlacklistTest.php',
'Flow\\Tests\\SpamFilter\\SpamRegexTest' => __DIR__ .
'/tests/phpunit/SpamFilter/SpamRegexTest.php',
'Flow\\Tests\\TemplateHelperTest' => __DIR__ .
'/tests/phpunit/TemplateHelperTest.php',
diff --git a/db_patches/patch-add-revision-content-length.sql
b/db_patches/patch-add-revision-content-length.sql
new file mode 100644
index 0000000..95f4fd0
--- /dev/null
+++ b/db_patches/patch-add-revision-content-length.sql
@@ -0,0 +1,3 @@
+ALTER TABLE flow_revision
+ ADD rev_content_length int NOT NULL DEFAULT 0,
+ ADD rev_previous_content_length int NOT NULL DEFAULT 0;
diff --git a/flow.sql b/flow.sql
index e86c89b..639d160 100644
--- a/flow.sql
+++ b/flow.sql
@@ -108,6 +108,9 @@
rev_edit_user_ip varbinary(39) default null,
rev_edit_user_wiki varchar(32) binary default null,
+ rev_content_length int not null default 0,
+ rev_previous_content_length int not null default 0,
+
PRIMARY KEY (rev_id)
) /*$wgDBTableOptions*/;
diff --git a/includes/Data/Listener/RecentChangesListener.php
b/includes/Data/Listener/RecentChangesListener.php
index d83655b..91b1478 100644
--- a/includes/Data/Listener/RecentChangesListener.php
+++ b/includes/Data/Listener/RecentChangesListener.php
@@ -78,6 +78,7 @@
*/
public function onAfterInsert( $revision, array $row, array $metadata )
{
global $wgRCFeeds;
+
$action = $revision->getChangeType();
$revisionId = $revision->getRevisionId()->getAlphadecimal();
$timestamp = $revision->getRevisionId()->getTimestamp();
@@ -89,29 +90,18 @@
}
$title = $this->getRcTitle( $workflow,
$revision->getChangeType() );
-
- $collection = $revision->getCollection();
-
- // get content of both this & the current revision
- $content = $revision->getContent( 'wikitext' );
- $previousContent = '';
- $previousRevision = $collection->getPrevRevision( $revision );
- if ( $previousRevision ) {
- $previousContent = $previousRevision->getContent(
'wikitext' );
- }
-
$attribs = array(
'rc_namespace' => $title->getNamespace(),
'rc_title' => $title->getDBkey(),
'rc_user' => $row['rev_user_id'],
'rc_user_text' => $this->usernames->get( wfWikiId(),
$row['rev_user_id'], $row['rev_user_ip'] ),
'rc_type' => RC_FLOW,
- 'rc_source' => self::SRC_FLOW, // depends on core
change in gerrit 85787
+ 'rc_source' => self::SRC_FLOW,
'rc_minor' => 0,
'rc_bot' => 0, // TODO: is revision by bot
'rc_patrolled' => 0,
- 'rc_old_len' => strlen( $previousContent ),
- 'rc_new_len' => strlen( $content ),
+ 'rc_old_len' => $revision->getPreviousContentLength(),
+ 'rc_new_len' => $revision->getContentLength(),
'rc_this_oldid' => 0,
'rc_last_oldid' => 0,
'rc_log_type' => null,
diff --git a/includes/Data/Storage/RevisionStorage.php
b/includes/Data/Storage/RevisionStorage.php
index 80bf75f..97a5c13 100644
--- a/includes/Data/Storage/RevisionStorage.php
+++ b/includes/Data/Storage/RevisionStorage.php
@@ -26,6 +26,12 @@
'rev_mod_user_wiki',
'rev_mod_timestamp',
'rev_mod_reason',
+
+ // This is temporary for a maint script, can be removed once
+ // FlowUpdateRevisionContentLength is no longer needed. These
two
+ // fields should *not* be updated in the normal course of
operation
+ 'rev_content_length',
+ 'rev_previous_content_length',
);
/**
diff --git a/includes/Formatter/AbstractQuery.php
b/includes/Formatter/AbstractQuery.php
index bd78693..ee01887 100644
--- a/includes/Formatter/AbstractQuery.php
+++ b/includes/Formatter/AbstractQuery.php
@@ -100,7 +100,9 @@
}
$revisions[$result->getRevisionId()->getAlphadecimal()]
= $result;
- $previousRevisionIds[get_class( $result )][] =
$result->getPrevRevisionId();
+ if ( $this->needsPreviousRevision( $result ) ) {
+ $previousRevisionIds[get_class( $result )][] =
$result->getPrevRevisionId();
+ }
$collection = $result->getCollection();
$collectionIds[get_class( $result )][] =
$collection->getId();
@@ -138,7 +140,7 @@
// should cover the lot.
$workflows = $this->storage->getMulti( 'Workflow', array_merge(
$rootPostIds, $workflowIds ) );
- // preload all previous revisions
+ // preload all requested previous revisions
foreach ( $previousRevisionIds as $revisionType => $ids ) {
// get rid of null-values (for original revisions,
without previous revision)
$ids = array_filter( $ids );
@@ -196,7 +198,9 @@
$row = $row ?: new FormatterRow;
$row->revision = $revision;
- $row->previousRevision = $this->getPreviousRevision( $revision
);
+ if ( $this->needsPreviousRevision( $revision ) ) {
+ $row->previousRevision = $this->getPreviousRevision(
$revision );
+ }
$row->currentRevision = $this->getCurrentRevision( $revision );
$row->workflow = $workflow;
@@ -233,6 +237,18 @@
}
/**
+ * Decides if the given abstract revision needs its prior revision for
formatting
+ * @param AbstractRevision $revision
+ * @return boolean true when the previous revision to this should be
loaded
+ */
+ protected function needsPreviousRevision( AbstractRevision $revision ) {
+ // crappy special case needs the previous object so it can show
the title
+ // but only when outputting a full history api result(we don't
know that here)
+ return $revision instanceof PostRevision
+ && $revision->getChangeType() === 'edit-title';
+ }
+
+ /**
* Retrieves the previous revision for a given AbstractRevision
* @param AbstractRevision $revision The revision to retrieve the
previous revision for.
* @return AbstractRevision|null AbstractRevision of the previous
revision or null if no previous revision.
diff --git a/includes/Formatter/RecentChanges.php
b/includes/Formatter/RecentChanges.php
index 839fb9a..04e0d70 100644
--- a/includes/Formatter/RecentChanges.php
+++ b/includes/Formatter/RecentChanges.php
@@ -40,12 +40,6 @@
throw new FlowException( 'Could not format data for row
' . $row->revision->getRevisionId()->getAlphadecimal() );
}
- // @todo where should this go?
- $data['size'] = array(
- 'old' => $row->recentChange->getAttribute( 'rc_old_len'
),
- 'new' => $row->recentChange->getAttribute( 'rc_new_len'
),
- );
-
if ( $linkOnly ) {
return $this->getTitleLink( $data, $row, $ctx );
}
diff --git a/includes/Formatter/RevisionFormatter.php
b/includes/Formatter/RevisionFormatter.php
index 7e686a2..20ed0f7 100644
--- a/includes/Formatter/RevisionFormatter.php
+++ b/includes/Formatter/RevisionFormatter.php
@@ -198,8 +198,8 @@
// These are write urls
'actions' => $this->buildActions( $row ),
'size' => array(
- 'old' => strlen( $row->previousRevision ?
$row->previousRevision->getContentRaw() : '' ),
- 'new' => strlen(
$row->revision->getContentRaw() ),
+ 'old' =>
$row->revision->getPreviousContentLength(),
+ 'new' => $row->revision->getContentLength(),
),
'author' => $this->serializeUser(
$row->revision->getUserWiki(),
@@ -212,10 +212,10 @@
$row->revision->getLastContentEditUserIp()
),
'lastEditId' => $row->revision->isOriginalContent() ?
null : $row->revision->getLastContentEditId()->getAlphadecimal(),
+ 'previousRevisionId' =>
$row->revision->isFirstRevision()
+ ? null
+ :
$row->revision->getPrevRevisionId()->getAlphadecimal(),
);
-
- $prevRevId = $row->revision->getPrevRevisionId();
- $res['previousRevisionId'] = $prevRevId ?
$prevRevId->getAlphadecimal() : null;
if ( $res['isModerated'] ) {
$res['moderator'] = $this->serializeUser(
@@ -235,23 +235,11 @@
// topic titles are always forced to plain text
$contentFormat = $this->decideContentFormat(
$row->revision );
- $res += array(
- // @todo better name?
- 'content' => array(
- 'content' =>
$this->templating->getContent( $row->revision, $contentFormat ),
- 'format' => $contentFormat
- ),
- 'size' => array(
- 'old' => null,
- // @todo this isn't really correct
- 'new' => strlen(
$row->revision->getContentRaw() ),
- ),
+ // @todo better name?
+ $res['content'] = array(
+ 'content' => $this->templating->getContent(
$row->revision, $contentFormat ),
+ 'format' => $contentFormat
);
- if ( $row->previousRevision
- && $this->permissions->isAllowed(
$row->previousRevision, 'view' )
- ) {
- $res['size']['old'] = strlen(
$row->previousRevision->getContentRaw() );
- }
}
if ( $row instanceof TopicRow ) {
diff --git a/includes/Model/AbstractRevision.php
b/includes/Model/AbstractRevision.php
index 3a14936..f1af62a 100644
--- a/includes/Model/AbstractRevision.php
+++ b/includes/Model/AbstractRevision.php
@@ -125,6 +125,16 @@
protected $lastEditUser;
/**
+ * @var integer Size of previous revision wikitext
+ */
+ protected $previousContentLength = 0;
+
+ /**
+ * @var integer Size of current revision wikitext
+ */
+ protected $contentLength = 0;
+
+ /**
* @param string[] $row
* @param AbstractRevision|null $obj
* @return AbstractRevision
@@ -166,6 +176,9 @@
$obj->lastEditId = isset( $row['rev_last_edit_id'] ) ?
UUID::create( $row['rev_last_edit_id'] ) : null;
$obj->lastEditUser = UserTuple::newFromArray( $row,
'rev_edit_user_' );
+ $obj->contentLength = isset( $row['rev_content_length'] ) ?
$row['rev_content_length'] : 0;
+ $obj->previousContentLength = isset(
$row['rev_previous_content_length'] ) ? $row['rev_previous_content_length'] : 0;
+
return $obj;
}
@@ -199,6 +212,9 @@
'rev_edit_user_id' => $obj->lastEditUser ?
$obj->lastEditUser->id : null,
'rev_edit_user_ip' => $obj->lastEditUser ?
$obj->lastEditUser->ip : null,
'rev_edit_user_wiki' => $obj->lastEditUser ?
$obj->lastEditUser->wiki : null,
+
+ 'rev_content_length' => $obj->contentLength,
+ 'rev_previous_content_length' =>
$obj->previousContentLength,
);
}
@@ -220,6 +236,8 @@
$obj->user = UserTuple::newFromUser( $user );
$obj->prevRevision = $this->revId;
$obj->changeType = '';
+ $obj->previousContentLength = $obj->contentLength;
+
return $obj;
}
@@ -420,6 +438,8 @@
// should this only remove a subset of flags?
$this->flags = array_filter( explode( ',',
\Revision::compressRevisionText( $this->content ) ) );
$this->flags[] = $storageFormat;
+
+ $this->contentLength = mb_strlen( $this->getContent( 'wikitext'
) );
}
/**
@@ -642,6 +662,20 @@
}
/**
+ * @return integer
+ */
+ public function getContentLength() {
+ return $this->contentLength;
+ }
+
+ /**
+ * @return integer
+ */
+ public function getPreviousContentLength() {
+ return $this->previousContentLength;
+ }
+
+ /**
* @return string
*/
abstract public function getRevisionType();
diff --git a/includes/SpamFilter/ContentLengthFilter.php
b/includes/SpamFilter/ContentLengthFilter.php
index fb9940b..c4a9d25 100644
--- a/includes/SpamFilter/ContentLengthFilter.php
+++ b/includes/SpamFilter/ContentLengthFilter.php
@@ -8,6 +8,15 @@
class ContentLengthFilter implements SpamFilter {
+ /**
+ * @var integer The maximum number of characters of wikitext to allow
through filter
+ */
+ protected $maxLength;
+
+ public function __construct( $maxLength = 25600 ) {
+ $this->maxLength = $maxLength;
+ }
+
public function enabled() {
return true;
}
@@ -19,8 +28,8 @@
* @return Status
*/
public function validate( AbstractRevision $newRevision,
AbstractRevision $oldRevision = null, Title $title ) {
- return strlen( $newRevision->getContentRaw() ) > 25600
- ? Status::newFatal( 'flow-error-content-too-long',
'25600' )
+ return $newRevision->getContentLength() > $this->maxLength
+ ? Status::newFatal( 'flow-error-content-too-long',
$this->maxLength )
: Status::newGood();
}
}
diff --git a/maintenance/FlowUpdateRevisionContentLength.php
b/maintenance/FlowUpdateRevisionContentLength.php
new file mode 100644
index 0000000..4ad119e
--- /dev/null
+++ b/maintenance/FlowUpdateRevisionContentLength.php
@@ -0,0 +1,147 @@
+<?php
+
+use Flow\Container;
+use Flow\Model\AbstractRevision;
+use Flow\Model\UUID;
+
+require_once ( getenv( 'MW_INSTALL_PATH' ) !== false
+ ? getenv( 'MW_INSTALL_PATH' ) . '/maintenance/Maintenance.php'
+ : dirname( __FILE__ ) . '/../../../maintenance/Maintenance.php' );
+
+/**
+ * @ingroup Maintenance
+ */
+class FlowUpdateRevisionContentLength extends LoggedUpdateMaintenance {
+ /**
+ * Map from AbstractRevision::getRevisionType() to the class that holds
+ * that type.
+ * @todo seems this should be elsewhere for access by any code
+ *
+ * @var string[]
+ */
+ static private $revisionTypes = array(
+ 'post' => 'Flow\Model\PostRevision',
+ 'header' => 'Flow\Model\Header',
+ 'post-summary' => 'Flow\Model\PostSummary',
+ );
+
+ /**
+ * @var Flow\DbFactory
+ */
+ protected $dbFactory;
+
+ /**
+ * @var Flow\Data\ManagerGroup
+ */
+ protected $storage;
+
+ /**
+ * @var ReflectionProperty
+ */
+ protected $contentLengthProperty;
+
+ /**
+ * @var ReflectionProperty
+ */
+ protected $previousContentLengthProperty;
+
+ public function __construct() {
+ parent::__construct();
+ $this->mDescription = "Updates content length for revisions
with unset content length.";
+ }
+
+ public function getUpdateKey() {
+ return __CLASS__;
+ }
+
+ public function doDBUpdates() {
+ // Can't be done in constructor, happens too early in
+ // boot process
+ $this->dbFactory = Container::get( 'db.factory' );
+ $this->storage = Container::get( 'storage' );
+ // Since this is a one-shot maintenance script just reach in
via reflection
+ // to change lenghts
+ $this->contentLengthProperty = new ReflectionProperty(
+ 'Flow\Model\AbstractRevision',
+ 'contentLength'
+ );
+ $this->contentLengthProperty->setAccessible( true );
+ $this->previousContentLengthProperty = new ReflectionProperty(
+ 'Flow\Model\AbstractRevision',
+ 'previousContentLength'
+ );
+ $this->previousContentLengthProperty->setAccessible( true );
+
+ $dbw = $this->dbFactory->getDb( DB_MASTER );
+ // Walk through the flow_revision table
+ $it = new EchoBatchRowIterator(
+ $dbw,
+ /* table = */'flow_revision',
+ /* primary key = */'rev_id',
+ $this->mBatchSize
+ );
+ // Only fetch rows with current and previous content length set
to 0
+ $it->addConditions( array(
+ 'rev_content_length' => 0,
+ 'rev_previous_content_length' => 0,
+ ) );
+ // We only need the id and type field
+ $it->setFetchColumns( array( 'rev_id', 'rev_type' ) );
+
+ $total = $fail = 0;
+ foreach ( $it as $batch ) {
+ $dbw->begin();
+ foreach ( $batch as $row ) {
+ $total++;
+ if ( !isset(
self::$revisionTypes[$row->rev_type] ) ) {
+ $this->output( 'Unknown revision type:
' . $row->rev_type );
+ $fail++;
+ continue;
+ }
+ $om = $this->storage->getStorage(
self::$revisionTypes[$row->rev_type] );
+ $revId = UUID::create( $row->rev_id );
+ $obj = $om->get( $revId );
+ if ( !$obj ) {
+ $this->output( 'Could not load
revision: ' . $revId->getAlphadecimal() );
+ $fail++;
+ continue;
+ }
+ if ( $obj->isFirstRevision() ) {
+ $previous = null;
+ } else {
+ $previous = $om->get(
$obj->getPrevRevisionId() );
+ if ( !$previous ) {
+ $this->output( 'Could not
locate previous revision: ' . $obj->getPrevRevisionId()->getAlphadecimal() );
+ $fail++;
+ continue;
+ }
+ }
+
+ $this->updateRevision( $obj, $previous );
+ $om->put( $obj );
+ $this->output( '.' );
+ }
+ $dbw->commit();
+ $this->storage->clear();
+ $this->dbFactory->waitForSlaves();
+ }
+
+ return true;
+ }
+
+ protected function updateRevision( AbstractRevision $revision,
AbstractRevision $previous = null ) {
+ $this->contentLengthProperty->setValue(
+ $revision,
+ mb_strlen( $revision->getContent( 'wikitext' ) )
+ );
+ if ( $previous !== null ) {
+ $this->previousContentLengthProperty->setValue(
+ $revision,
+ mb_strlen( $previous->getContent( 'wikitext' ) )
+ );
+ }
+ }
+}
+
+$maintClass = 'FlowUpdateRevisionContentLength';
+require_once( RUN_MAINTENANCE_IF_MAIN );
diff --git a/tests/phpunit/Formatter/RevisionFormatterTest.php
b/tests/phpunit/Formatter/RevisionFormatterTest.php
new file mode 100644
index 0000000..6769810
--- /dev/null
+++ b/tests/phpunit/Formatter/RevisionFormatterTest.php
@@ -0,0 +1,163 @@
+<?php
+
+namespace Flow\Tests\Formatter;
+
+use Flow\FlowActions;
+use Flow\Formatter\FormatterRow;
+use Flow\Formatter\RevisionFormatter;
+use Flow\Model\PostRevision;
+use Flow\Model\Workflow;
+use RequestContext;
+use Title;
+use User;
+
+/**
+ * @group Flow
+ */
+class RevisionFormatterTest extends \MediaWikiTestCase {
+ protected $user;
+
+ public function setUp() {
+ parent::setUp();
+ $this->user = User::newFromName( '127.0.0.1', false );
+ }
+
+ public function testMockFormatterBasicallyWorks() {
+ list( $formatter, $ctx ) = $this->mockFormatter();
+ $result = $formatter->formatApi( $this->generateRow( 'my new
topic' ), $ctx );
+ $this->assertEquals( 'new-post', $result['changeType'] );
+ $this->assertEquals( 'my new topic',
$result['content']['content'] );
+ }
+
+ public function testFormattingEditedTitle() {
+ list( $formatter, $ctx ) = $this->mockFormatter();
+ $row = $this->generateRow();
+ $row->previousRevision = $row->revision;
+ $row->revision = $row->revision->newNextRevision(
+ $this->user,
+ 'replacement content',
+ 'edit-title',
+ $row->workflow->getArticleTitle()
+ );
+ $result = $formatter->formatApi( $row, $ctx );
+ $this->assertEquals( 'edit-title', $result['changeType'] );
+ $this->assertEquals( 'replacement content',
$result['content']['content'] );
+ }
+
+ public function testFormattingContentLength() {
+ $content = 'something something';
+ $nextContent = 'ברוכים הבאים לוויקיפדיה!';
+
+ list( $formatter, $ctx, $permissions, $templating, $usernames,
$actions ) = $this->mockFormatter( true );
+
+ $row = $this->generateRow( $content );
+ $result = $formatter->formatApi( $row, $ctx );
+ $this->assertEquals(
+ strlen( $content ),
+ $result['size']['new'],
+ 'New topic content reported correctly'
+ );
+ $this->assertEquals(
+ 0,
+ $result['size']['old'],
+ 'With no previous revision the old size is 0'
+ );
+
+ $row->previousRevision = $row->revision;
+ // @todo newNextRevision feels too generic, there should be an
editTitle method?
+ $row->revision = $row->currentRevision =
$row->revision->newNextRevision(
+ $this->user,
+ $nextContent,
+ 'edit-title',
+ $row->workflow->getArticleTitle()
+ );
+ $result = $formatter->formatApi( $row, $ctx );
+ $this->assertEquals(
+ mb_strlen( $nextContent ),
+ $result['size']['new'],
+ 'After editing topic content the new size has been
updated'
+ );
+ $this->assertEquals(
+ mb_strlen( $content ),
+ $result['size']['old'],
+ 'After editing topic content the old size has been
updated'
+ );
+ }
+
+ public function generateRow( $plaintext = 'titlebar content' ) {
+ $row = new FormatterRow;
+ $row->workflow = Workflow::create( 'topic',
Title::newMainPage() );
+ $row->rootPost = PostRevision::create( $row->workflow,
$this->user, $plaintext );
+ $row->revision = $row->currentRevision = $row->rootPost;
+
+ return $row;
+ }
+
+ protected function mockActions() {
+ return $this->getMockBuilder( 'Flow\FlowActions' )
+ ->disableOriginalConstructor()
+ ->getMock();
+ }
+
+ protected function mockPermissions( FlowActions $actions ) {
+ $permissions = $this->getMockBuilder(
'Flow\RevisionActionPermissions' )
+ ->disableOriginalConstructor()
+ ->getMock();
+ // bit of a code smell, should pass actions directly in
constructor?
+ $permissions->expects( $this->any() )
+ ->method( 'getActions' )
+ ->will( $this->returnValue( $actions ) );
+ // perhaps another code smell, should have a method that does
whatever this
+ // uses the user for
+ $permissions->expects( $this->any() )
+ ->method( 'getUser' )
+ ->will( $this->returnValue( $this->user ) );
+
+ return $permissions;
+ }
+
+ protected function mockTemplating() {
+ $templating = $this->getMockBuilder( 'Flow\Templating' )
+ ->disableOriginalConstructor()
+ ->getMock();
+ $templating->expects( $this->any() )
+ ->method( 'getModeratedRevision' )
+ ->will( $this->returnArgument( 0 ) );
+ $templating->expects( $this->any() )
+ ->method( 'getContent' )
+ ->will( $this->returnCallback( function( $revision,
$contentFormat ) {
+ return $revision->getContent( $contentFormat );
+ } ) );
+
+ return $templating;
+ }
+
+ protected function mockUserNameBatch() {
+ return $this->getMockBuilder( 'Flow\Repository\UserNameBatch' )
+ ->disableOriginalConstructor()
+ ->getMock();
+ }
+
+ // @todo name seems wrong, the Formatter is real everything else is
mocked
+ public function mockFormatter( $returnAll = false ) {
+ $actions = $this->mockActions();
+ $permissions = $this->mockPermissions( $actions );
+ // formatting only proceedes when this is true
+ $permissions->expects( $this->any() )
+ ->method( 'isAllowed' )
+ ->will( $this->returnValue( true ) );
+ $templating = $this->mockTemplating();
+ $usernames = $this->mockUserNameBatch();
+ $formatter = new RevisionFormatter( $permissions, $templating,
$usernames, 3 );
+
+ $ctx = RequestContext::getMain();
+ $ctx->setUser( $this->user );
+
+
+ if ( $returnAll ) {
+ return array( $formatter, $ctx, $permissions,
$templating, $usernames, $actions );
+ } else {
+ return array( $formatter, $ctx );
+ }
+ }
+}
diff --git a/tests/phpunit/Model/PostRevisionTest.php
b/tests/phpunit/Model/PostRevisionTest.php
index 003df25..6a74d6c 100644
--- a/tests/phpunit/Model/PostRevisionTest.php
+++ b/tests/phpunit/Model/PostRevisionTest.php
@@ -4,7 +4,10 @@
use Flow\Model\PostRevision;
use Flow\Model\UUID;
+use Flow\Model\Workflow;
use Flow\Tests\PostRevisionTestCase;
+use User;
+use Title;
/**
* @group Flow
@@ -30,4 +33,21 @@
$roundtripRow = UUID::convertUUIDs( $roundtripRow, 'binary' );
$this->assertEquals( $row, $roundtripRow );
}
+
+ public function testContentLength() {
+ $content = 'This is a topic title';
+ $nextContent = 'Changed my mind';
+
+ $title = Title::newMainPage();
+ $user = User::newFromName( '127.0.0.1', false );
+ $workflow = Workflow::create( 'topic', $title );
+
+ $topic = PostRevision::create( $workflow, $user, $content );
+ $this->assertEquals( 0, $topic->getPreviousContentLength() );
+ $this->assertEquals( mb_strlen( $content ),
$topic->getContentLength() );
+
+ $next = $topic->newNextRevision( $user, $nextContent,
'edit-title', $title );
+ $this->assertEquals( mb_strlen( $content ),
$next->getPreviousContentLength() );
+ $this->assertEquals( mb_strlen( $nextContent ),
$next->getContentLength() );
+ }
}
diff --git a/tests/phpunit/PostRevisionTestCase.php
b/tests/phpunit/PostRevisionTestCase.php
index 10294a4..03edf01 100644
--- a/tests/phpunit/PostRevisionTestCase.php
+++ b/tests/phpunit/PostRevisionTestCase.php
@@ -112,6 +112,8 @@
'rev_edit_user_wiki' => null,
'rev_edit_user_id' => null,
'rev_edit_user_ip' => null,
+ 'rev_content_length' => 0,
+ 'rev_previous_content_length' => 0,
// flow_tree_revision
'tree_rev_descendant_id' =>
$this->workflow->getId()->getBinary(),
diff --git a/tests/phpunit/SpamFilter/ContentLengthFilterTest.php
b/tests/phpunit/SpamFilter/ContentLengthFilterTest.php
new file mode 100644
index 0000000..d749566
--- /dev/null
+++ b/tests/phpunit/SpamFilter/ContentLengthFilterTest.php
@@ -0,0 +1,58 @@
+<?php
+
+namespace Flow\Tests\SpamFilter;
+
+use Flow\Model\PostRevision;
+use Flow\Model\Workflow;
+use Flow\SpamFilter\ContentLengthFilter;
+use User;
+use Title;
+
+/**
+ * @group Flow
+ */
+class ContentLengthFilterTest extends \MediaWikiTestCase {
+ /**
+ * @var SpamRegex
+ */
+ protected $spamFilter;
+
+ public function spamProvider() {
+ return array(
+ array(
+ 'With content shorter than max length allow
through filter',
+ // expect
+ true,
+ // content
+ 'blah',
+ // max length
+ 100
+ ),
+
+ array(
+ 'With content longer than max length dissalow
through filter',
+ // expect
+ false,
+ // content
+ 'blah',
+ // max length
+ 2
+ ),
+ );
+ }
+
+ /**
+ * @dataProvider spamProvider
+ */
+ public function testSpam( $message, $expect, $content, $maxLength ) {
+ $title = Title::newFromText( 'UTPage' );
+ $user = User::newFromName( '127.0.0.1', false );
+ $workflow = Workflow::create( 'topic', $title );
+ $topic = PostRevision::create( $workflow, $user, 'title
content' );
+ $reply = $topic->reply( $workflow, $user, $content );
+
+ $spamFilter = new ContentLengthFilter( $maxLength );
+ $status = $spamFilter->validate( $reply, null, $title );
+ $this->assertEquals( $expect, $status->isOK() );
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/165683
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6996acf45aebbf6fda2ce85f0f10d3d9d1690fa6
Gerrit-PatchSet: 20
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <[email protected]>
Gerrit-Reviewer: EBernhardson <[email protected]>
Gerrit-Reviewer: Matthias Mullie <[email protected]>
Gerrit-Reviewer: Springle <[email protected]>
Gerrit-Reviewer: Werdna <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
