Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

EditPage: Disentangle edit summary and section title

Browse source 
Previously, `$this->summary` was used for two different purposes.
Usually it was just the summary. But when `$this->section` was 'new',
then it was actually the section title most of the time – unless
`$this->sectiontitle` was also set (in which case it took priority),
and until it was replaced by the real edit summary (near the end of
the processing, after copying the section title to the page content
and before saving changes).

Unsurprisingly some of the code didn't handle this duality correctly,
causing T191722 and T311533.

Now `$this->summary` is always the summary, and when `$this->section`
is 'new', then `$this->sectiontitle` is always the new section title.

The only place where this duality remains is in the input attributes
and query parameters, where 'wpSummary' is still used for both the
summary and the section title inputs (only one of them can appear,
depending on whether `$this->section` is 'new'). It would be an
unreasonable backwards-compatibility break to change this, and the
code handling this is somewhat isolated from the rest of the logic.

Bug: T191722
Bug: T311533
Change-Id: I5313ca9a045d112ece390b011a34192220e2abc1
This commit is contained in:
Bartosz Dziewoński 2022-07-15 02:46:52 +02:00
parent 2d8a15d8d2
commit 990fd8f0b2
8 changed files with 91 additions and 132 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

143
includes/EditPage.php
View file

@ -36,7 +36,7 @@ use MediaWiki\EditPage\Constraint\EditRightConstraint;
use MediaWiki\EditPage\Constraint\IEditConstraint;
use MediaWiki\EditPage\Constraint\ImageRedirectConstraint;
use MediaWiki\EditPage\Constraint\MissingCommentConstraint;
use MediaWiki\EditPage\Constraint\NewSectionMissingSummaryConstraint;
use MediaWiki\EditPage\Constraint\NewSectionMissingSubjectConstraint;
use MediaWiki\EditPage\Constraint\PageSizeConstraint;
use MediaWiki\EditPage\Constraint\SelfRedirectConstraint;
use MediaWiki\EditPage\Constraint\SpamRegexConstraint;
@ -75,6 +75,7 @@ use OOUI\ButtonWidget;
use OOUI\CheckboxInputWidget;
use OOUI\DropdownInputWidget;
use OOUI\FieldLayout;
use Wikimedia\Assert\Assert;
use Wikimedia\Message\MessageValue;
use Wikimedia\ParamValidator\TypeDef\ExpiryDef;
use Wikimedia\ScopedCallback;
@ -305,6 +306,9 @@ class EditPage implements IEditObject {
/** @var string|null */
public $sectiontitle = null;
/** @var string|null */
private $newSectionAnchor = null;
/** @var string|null
* Timestamp from the first time the edit form was rendered.
*/
@ -1134,23 +1138,30 @@ class EditPage implements IEditObject {
$this->unicodeCheck = $request->getText( 'wpUnicodeCheck' );
$this->summary = $request->getText( 'wpSummary' );
if ( $this->section === 'new' ) {
# Allow setting sectiontitle different from the edit summary.
# Note that wpSectionTitle is not yet a part of the actual edit form, as wpSummary is
# currently doing double duty as both edit summary and section title. Right now this
# is just to allow API edits to work around this limitation, but this should be
# incorporated into the actual edit form when EditPage is rewritten (T20654, T28312).
if ( $request->getCheck( 'wpSectionTitle' ) ) {
$this->sectiontitle = $request->getText( 'wpSectionTitle' );
if ( $request->getCheck( 'wpSummary' ) ) {
$this->summary = $request->getText( 'wpSummary' );
}
} else {
$this->sectiontitle = $request->getText( 'wpSummary' );
# If the summary consists of a heading, e.g. '==Foobar==', extract the title from the
# header syntax, e.g. 'Foobar'. This is mainly an issue when we are using wpSummary for
# section titles. (T3600)
# (This may be no longer desirable, now that this code is an API for other editing interfaces?)
$this->sectiontitle = preg_replace( '/^\s*=+\s*(.*?)\s*=+\s*$/', '$1', $this->sectiontitle );
}
# If the summary consists of a heading, e.g. '==Foobar==', extract the title from the
# header syntax, e.g. 'Foobar'. This is mainly an issue when we are using wpSummary for
# section titles.
$this->summary = preg_replace( '/^\s*=+\s*(.*?)\s*=+\s*$/', '$1', $this->summary );
# Allow setting sectiontitle different from the edit summary.
# Note that wpSectionTitle is not yet a part of the actual edit form, as wpSummary is
# currently doing double duty as both edit summary and section title. Right now this
# is just to allow API edits to work around this limitation, but this should be
# incorporated into the actual edit form when EditPage is rewritten (T20654, T28312).
if ( $request->getCheck( 'wpSectionTitle' ) ) {
$this->sectiontitle = $request->getText( 'wpSectionTitle' );
$this->sectiontitle = preg_replace( '/^\s*=+\s*(.*?)\s*=+\s*$/', '$1', $this->sectiontitle );
$this->setNewSectionSummary();
} else {
$this->sectiontitle = null;
$this->summary = $request->getText( 'wpSummary' );
}
$this->edittime = $request->getVal( 'wpEdittime' );
@ -1285,8 +1296,7 @@ class EditPage implements IEditObject {
// preloadtitle parameter in the URL (T15100)
if ( $this->section === 'new' && $request->getCheck( 'preloadtitle' ) ) {
$this->sectiontitle = $request->getVal( 'preloadtitle' );
// Once wpSummary isn't being use for setting section titles, we should delete this.
$this->summary = $request->getVal( 'preloadtitle' );
$this->setNewSectionSummary();
} elseif ( $this->section !== 'new' && $request->getVal( 'summary' ) !== '' ) {
$this->summary = $request->getText( 'summary' );
if ( $this->summary !== '' ) {
@ -2071,42 +2081,31 @@ class EditPage implements IEditObject {
}
/**
* Return the summary to be used for a new section.
*
* @return string[] array with two values, the summary and the anchor text
* Set the edit summary and link anchor to be used for a new section.
*/
private function newSectionSummary(): array {
$newSectionSummary = $this->summary;
$newSectionAnchor = '';
private function setNewSectionSummary(): void {
Assert::precondition( $this->section === 'new', 'This method can only be called for new sections' );
Assert::precondition( $this->sectiontitle !== null, 'This method can only be called for new sections' );
$services = MediaWikiServices::getInstance();
$parser = $services->getParser();
$textFormatter = $services->getMessageFormatterFactory()->getTextFormatter(
$services->getContentLanguage()->getCode()
);
if ( $this->sectiontitle !== null ) {
if ( $this->sectiontitle !== '' ) {
$newSectionAnchor = $this->guessSectionName( $this->sectiontitle );
// If no edit summary was specified, create one automatically from the section
// title and have it link to the new section. Otherwise, respect the summary as
// passed.
if ( $this->summary === '' ) {
$messageValue = MessageValue::new( 'newsectionsummary' )
->plaintextParams( $parser->stripSectionName( $this->sectiontitle ) );
$newSectionSummary = $textFormatter->format( $messageValue );
}
if ( $this->sectiontitle !== '' ) {
$this->newSectionAnchor = $this->guessSectionName( $this->sectiontitle );
// If no edit summary was specified, create one automatically from the section
// title and have it link to the new section. Otherwise, respect the summary as
// passed.
if ( $this->summary === '' ) {
$messageValue = MessageValue::new( 'newsectionsummary' )
->plaintextParams( $parser->stripSectionName( $this->sectiontitle ) );
$this->summary = $textFormatter->format( $messageValue );
}
} else {
if ( $this->summary !== '' ) {
$newSectionAnchor = $this->guessSectionName( $this->summary );
// This is a new section, so create a link to the new section
// in the revision summary.
$messageValue = MessageValue::new( 'newsectionsummary' )
->plaintextParams( $parser->stripSectionName( $this->summary ) );
$newSectionSummary = $textFormatter->format( $messageValue );
}
$this->newSectionAnchor = '';
}
return [ $newSectionSummary, $newSectionAnchor ];
}
/**
@ -2210,7 +2209,6 @@ class EditPage implements IEditObject {
$constraintRunner->addConstraint(
$constraintFactory->newSpamRegexConstraint(
$this->summary,
$this->section,
$this->sectiontitle,
$this->textbox1,
$this->context->getRequest()->getIP(),
@ -2299,14 +2297,8 @@ class EditPage implements IEditObject {
if ( $this->sectiontitle !== null ) {
// Insert the section title above the content.
$content = $content->addSectionHeader( $this->sectiontitle );
} else {
// Insert the section title above the content.
$content = $content->addSectionHeader( $this->summary );
}
[ $newSectionSummary, $anchor ] = $this->newSectionSummary();
$this->summary = $newSectionSummary;
$result['sectionanchor'] = $anchor;
$result['sectionanchor'] = $this->newSectionAnchor;
}
$pageUpdater = $this->page->newPageUpdater( $pstUser )
@ -2370,10 +2362,9 @@ class EditPage implements IEditObject {
|| ( $this->editRevId !== null && $this->editRevId != $latest )
) {
$this->isConflict = true;
[ $newSectionSummary, $newSectionAnchor ] = $this->newSectionSummary();
if ( $this->section === 'new' ) {
if ( $this->page->getUserText() === $requestUser->getName() &&
$this->page->getComment() === $newSectionSummary
$this->page->getComment() === $this->summary
) {
// Probably a duplicate submission of a new comment.
// This can happen when CDN resends a request after
@ -2401,12 +2392,6 @@ class EditPage implements IEditObject {
}
}
if ( $this->sectiontitle !== null ) {
$sectionTitle = $this->sectiontitle;
} else {
$sectionTitle = $this->summary;
}
$content = null;
if ( $this->isConflict ) {
@ -2426,14 +2411,14 @@ class EditPage implements IEditObject {
$content = $this->page->replaceSectionAtRev(
$this->section,
$textbox_content,
$sectionTitle,
$this->sectiontitle,
$this->editRevId
);
} else {
$content = $this->page->replaceSectionContent(
$this->section,
$textbox_content,
$sectionTitle,
$this->sectiontitle,
$this->edittime
);
}
@ -2445,7 +2430,7 @@ class EditPage implements IEditObject {
$content = $this->page->replaceSectionAtRev(
$this->section,
$textbox_content,
$sectionTitle
$this->sectiontitle
);
}
@ -2493,8 +2478,8 @@ class EditPage implements IEditObject {
if ( $this->section === 'new' ) {
$constraintRunner->addConstraint(
new NewSectionMissingSummaryConstraint(
$this->summary,
new NewSectionMissingSubjectConstraint(
$this->sectiontitle,
$this->allowBlankSummary
)
);
@ -2524,9 +2509,7 @@ class EditPage implements IEditObject {
# All's well
$sectionAnchor = '';
if ( $this->section === 'new' ) {
[ $newSectionSummary, $anchor ] = $this->newSectionSummary();
$this->summary = $newSectionSummary;
$sectionAnchor = $anchor;
$sectionAnchor = $this->newSectionAnchor;
} elseif ( $this->section !== '' ) {
# Try to get a section anchor from the section source, redirect
# to edited section if header found.
@ -2679,7 +2662,7 @@ class EditPage implements IEditObject {
$this->hookError = $failed->getHookError();
} elseif (
$failed instanceof AutoSummaryMissingSummaryConstraint ||
$failed instanceof NewSectionMissingSummaryConstraint
$failed instanceof NewSectionMissingSubjectConstraint
) {
$this->missingSummary = true;
} elseif ( $failed instanceof MissingCommentConstraint ) {
@ -3745,7 +3728,7 @@ class EditPage implements IEditObject {
$labelText = $this->context->msg( $isSubjectPreview ? 'subject' : 'summary' )->parse();
$this->context->getOutput()->addHTML(
$this->getSummaryInputWidget(
$this->summary,
$isSubjectPreview ? $this->sectiontitle : $this->summary,
$labelText,
[ 'class' => $summaryClass ]
)
@ -3759,19 +3742,19 @@ class EditPage implements IEditObject {
* @return string
*/
private function getSummaryPreview( bool $isSubjectPreview ): string {
// FIXME When $isSubjectPreview is true, this previews the summary anyway, despite the label
// stating "Preview of subject:". This seems weird, but it was clearly intentional in b49d9f5ae3
// (2008) and unchanged since then. Admittedly, previewing just the subject (section title)
// would be redundant since it's included in the main preview. However, live preview
// (mediawiki.action.edit.preview.js) does just that. Someone should decide and make them both
// the same, I'm just keeping it unchanged for now.
// avoid spaces in preview, gets always trimmed on save
$summary = trim( $this->summary );
if ( $summary === '' || ( !$this->preview && !$this->diff ) ) {
return "";
}
if ( $isSubjectPreview ) {
$summary = $this->context->msg( 'newsectionsummary' )
->rawParams( MediaWikiServices::getInstance()->getParser()
->stripSectionName( $summary ) )
->inContentLanguage()->text();
}
$message = $isSubjectPreview ? 'subject-preview' : 'summary-preview';
$commentFormatter = MediaWikiServices::getInstance()->getCommentFormatter();
@ -3947,11 +3930,11 @@ class EditPage implements IEditObject {
$textboxContent = $this->toEditContent( $this->textbox1 );
if ( $this->editRevId !== null ) {
$newContent = $this->page->replaceSectionAtRev(
$this->section, $textboxContent, $this->summary, $this->editRevId
$this->section, $textboxContent, $this->sectiontitle, $this->editRevId
);
} else {
$newContent = $this->page->replaceSectionContent(
$this->section, $textboxContent, $this->summary, $this->edittime
$this->section, $textboxContent, $this->sectiontitle, $this->edittime
);
}
@ -4403,10 +4386,8 @@ class EditPage implements IEditObject {
}
}
# If we're adding a comment, we need to show the
# summary as the headline
if ( $this->section === "new" ) {
$content = $content->addSectionHeader( $this->summary );
$content = $content->addSectionHeader( $this->sectiontitle );
}
// @phan-suppress-next-line PhanTypeMismatchArgument Type mismatch on pass-by-ref args

3
includes/editpage/Constraint/EditConstraintFactory.php
View file

@ -188,7 +188,6 @@ class EditConstraintFactory {
/**
* @param string $summary
* @param string $section
* @param ?string $sectionHeading
* @param string $text
* @param string $reqIP
@ -197,7 +196,6 @@ class EditConstraintFactory {
*/
public function newSpamRegexConstraint(
string $summary,
string $section,
?string $sectionHeading,
string $text,
string $reqIP,
@ -207,7 +205,6 @@ class EditConstraintFactory {
$this->loggerFactory->getLogger( 'SpamRegex' ),
$this->spamRegexChecker,
$summary,
$section,
$sectionHeading,
$text,
$reqIP,

26
includes/editpage/Constraint/NewSectionMissingSummaryConstraint.php → includes/editpage/Constraint/NewSectionMissingSubjectConstraint.php
View file

@ -23,37 +23,37 @@ namespace MediaWiki\EditPage\Constraint;
use StatusValue;
/**
* For a new section, do not allow the user to post with an empty summary unless they choose to
* For a new section, do not allow the user to post with an empty subject (section title) unless they choose to
*
* @since 1.36
* @since 1.39
* @internal
* @author DannyS712
*/
class NewSectionMissingSummaryConstraint implements IEditConstraint {
class NewSectionMissingSubjectConstraint implements IEditConstraint {
/** @var string */
private $userSummary;
private $subject;
/** @var bool */
private $allowBlankSummary;
private $allowBlankSubject;
/** @var string|null */
private $result;
/**
* @param string $userSummary
* @param bool $allowBlankSummary
* @param string $subject
* @param bool $allowBlankSubject
*/
public function __construct(
string $userSummary,
bool $allowBlankSummary
string $subject,
bool $allowBlankSubject
) {
$this->userSummary = $userSummary;
$this->allowBlankSummary = $allowBlankSummary;
$this->subject = $subject;
$this->allowBlankSubject = $allowBlankSubject;
}
public function checkConstraint(): string {
if ( !$this->allowBlankSummary && trim( $this->userSummary ) == '' ) {
if ( !$this->allowBlankSubject && trim( $this->subject ) == '' ) {
// TODO this was == in EditPage, can it be === ?
$this->result = self::CONSTRAINT_FAILED;
} else {
@ -67,6 +67,8 @@ class NewSectionMissingSummaryConstraint implements IEditConstraint {
if ( $this->result === self::CONSTRAINT_FAILED ) {
// From EditPage, regarding the fatal:
// or 'missingcommentheader' if $section == 'new'. Blegh
// For new sections, the subject is also used for the summary,
// so we report missing summaries if the section is missing
$statusValue->fatal( 'missingsummary' );
$statusValue->value = self::AS_SUMMARY_NEEDED;
}

26
includes/editpage/Constraint/SpamRegexConstraint.php
View file

@ -43,7 +43,7 @@ class SpamRegexConstraint implements IEditConstraint {
/** @var string */
private $summary;
/** @var string */
/** @var ?string */
private $sectionHeading;
/** @var string */
@ -62,7 +62,6 @@ class SpamRegexConstraint implements IEditConstraint {
* @param LoggerInterface $logger for logging hits
* @param SpamChecker $spamChecker
* @param string $summary
* @param string $section
* @param ?string $sectionHeading
* @param string $text
* @param string $reqIP for logging hits
@ -72,28 +71,15 @@ class SpamRegexConstraint implements IEditConstraint {
LoggerInterface $logger,
SpamChecker $spamChecker,
string $summary,
string $section,
?string $sectionHeading,
string $text,
string $reqIP,
Title $title
) {
if ( $section == 'new' ) {
// $wgSpamRegex is enforced on this new heading/summary because, unlike
// regular summaries, it is added to the actual wikitext.
// sectiontitle is only set if the API is used with `sectiontitle`, otherwise
// the summary is used which comes from the API `summary` parameter or the
// "Add Topic" user interface
$sectionHeadingToCheck = $sectionHeading ?? $summary;
} else {
// No section heading to check
$sectionHeadingToCheck = '';
}
$this->logger = $logger;
$this->spamChecker = $spamChecker;
$this->summary = $summary;
$this->sectionHeading = $sectionHeadingToCheck;
$this->sectionHeading = $sectionHeading;
$this->text = $text;
$this->reqIP = $reqIP;
$this->title = $title;
@ -101,12 +87,8 @@ class SpamRegexConstraint implements IEditConstraint {
public function checkConstraint(): string {
$match = $this->spamChecker->checkSummary( $this->summary );
if ( $match === false ) {
// $wgSpamRegex is enforced on this new heading/summary because, unlike
// regular summaries, it is added to the actual wikitext.
// EditPage has already determined, based on if this is the API with `sectiontitle`,
// or action=edit, or the API with `summary`, what will be the section title.
// If the section isn't new, the $this->sectionHeading is an empty string
if ( $match === false && $this->sectionHeading !== null ) {
// If the section isn't new, the $this->sectionHeading is null
$match = $this->spamChecker->checkContent( $this->sectionHeading );
}
if ( $match === false ) {

10
tests/phpunit/includes/EditPageConstraintsTest.php
View file

@ -208,7 +208,7 @@ class EditPageConstraintsTest extends MediaWikiLangTestCase {
$user = $this->getTestUser()->getUser();
$permissionManager = $this->getServiceContainer()->getPermissionManager();
// Needs edit rights to pass EditRightConstraint and reach NewSectionMissingSummaryConstraint
// Needs edit rights to pass EditRightConstraint and reach NewSectionMissingSubjectConstraint
$permissionManager->overrideUserRightsForTesting( $user, [ 'edit' ] );
$edit = [
@ -489,21 +489,21 @@ class EditPageConstraintsTest extends MediaWikiLangTestCase {
);
}
/** NewSectionMissingSummaryConstraint integration */
public function testNewSectionMissingSummaryConstraint() {
/** NewSectionMissingSubjectConstraint integration */
public function testNewSectionMissingSubjectConstraint() {
// Require the summary
$this->mergeMwGlobalArrayValue(
'wgDefaultUserOptions',
[ 'forceeditsummary' => 1 ]
);
$page = $this->getExistingTestPage( 'NewSectionMissingSummaryConstraint page does exist' );
$page = $this->getExistingTestPage( 'NewSectionMissingSubjectConstraint page does exist' );
$title = $page->getTitle();
$user = $this->getTestUser()->getUser();
$permissionManager = $this->getServiceContainer()->getPermissionManager();
// Needs edit rights to pass EditRightConstraint and reach NewSectionMissingSummaryConstraint
// Needs edit rights to pass EditRightConstraint and reach NewSectionMissingSubjectConstraint
$permissionManager->overrideUserRightsForTesting( $user, [ 'edit' ] );
$edit = [

1
tests/phpunit/unit/includes/editpage/Constraint/EditConstraintFactoryTest.php
View file

@ -92,7 +92,6 @@ class EditConstraintFactoryTest extends MediaWikiUnitTestCase {
SpamRegexConstraint::class,
$factory->newSpamRegexConstraint(
'EditSummary',
'new',
'SectionHeading',
'Text',
'RequestIP',

12
tests/phpunit/unit/includes/editpage/Constraint/NewSectionMissingSummaryConstraintTest.php → tests/phpunit/unit/includes/editpage/Constraint/NewSectionMissingSubjectConstraintTest.php
View file

@ -19,25 +19,25 @@
*/
use MediaWiki\EditPage\Constraint\IEditConstraint;
use MediaWiki\EditPage\Constraint\NewSectionMissingSummaryConstraint;
use MediaWiki\EditPage\Constraint\NewSectionMissingSubjectConstraint;
/**
* Tests the NewSectionMissingSummaryConstraint
* Tests the NewSectionMissingSubjectConstraint
*
* @author DannyS712
*
* @covers \MediaWiki\EditPage\Constraint\NewSectionMissingSummaryConstraint
* @covers \MediaWiki\EditPage\Constraint\NewSectionMissingSubjectConstraint
*/
class NewSectionMissingSummaryConstraintTest extends MediaWikiUnitTestCase {
class NewSectionMissingSubjectConstraintTest extends MediaWikiUnitTestCase {
use EditConstraintTestTrait;
public function testPass() {
$constraint = new NewSectionMissingSummaryConstraint( 'Summary', false );
$constraint = new NewSectionMissingSubjectConstraint( 'Subject', false );
$this->assertConstraintPassed( $constraint );
}
public function testFailure() {
$constraint = new NewSectionMissingSummaryConstraint( '', false );
$constraint = new NewSectionMissingSubjectConstraint( '', false );
$this->assertConstraintFailed( $constraint, IEditConstraint::AS_SUMMARY_NEEDED );
}

2
tests/phpunit/unit/includes/editpage/Constraint/SpamRegexConstraintTest.php
View file

@ -62,7 +62,6 @@ class SpamRegexConstraintTest extends MediaWikiUnitTestCase {
$logger,
$spamChecker,
$summary,
'new',
$sectionHeading,
$text,
'Request-IP',
@ -95,7 +94,6 @@ class SpamRegexConstraintTest extends MediaWikiUnitTestCase {
$logger,
$spamChecker,
$summary,
'',
$sectionHeading,
$text,
'Request-IP',