Trim whitespace from truncated heading titles in IDs

Bug: T356196
Change-Id: Iddcda0cee624fda7f78a05e0a3d70eaee2635da9
This commit is contained in:
Ed Sanders 2024-02-02 16:06:30 +00:00
parent 687284939c
commit 501046f38c
7 changed files with 204 additions and 43 deletions

View file

@ -1276,26 +1276,40 @@ class CommentParser {
/**
* Truncate user generated parts of IDs so full ID always fits within a database field of length 255
*
* nb: Text should already have had spaces replaced with underscores by this point.
*
* @param string $text Text
* @param bool $legacy Generate legacy ID, not needed in JS implementation
* @return string Truncated text
*/
private function truncateForId( string $text ): string {
return $this->language->truncateForDatabase( $text, 80, '' );
private function truncateForId( string $text, bool $legacy = false ): string {
$truncated = $this->language->truncateForDatabase( $text, 80, '' );
if ( !$legacy ) {
$truncated = trim( $truncated, '_' );
}
return $truncated;
}
/**
* Given a thread item, return an identifier for it that is unique within the page.
*
* @param ContentThreadItem $threadItem
* @param ContentThreadItemSet $previousItems
* @param bool $legacy Generate legacy ID, not needed in JS implementation
* @return string
*/
private function computeId( ContentThreadItem $threadItem, ContentThreadItemSet $previousItems ): string {
private function computeId(
ContentThreadItem $threadItem, ContentThreadItemSet $previousItems, bool $legacy = false
): string {
$id = null;
if ( $threadItem instanceof ContentHeadingItem && $threadItem->isPlaceholderHeading() ) {
// The range points to the root note, using it like below results in silly values
$id = 'h-';
} elseif ( $threadItem instanceof ContentHeadingItem ) {
$id = 'h-' . $this->truncateForId( $threadItem->getLinkableId() );
$id = 'h-' . $this->truncateForId( $threadItem->getLinkableId(), $legacy );
} elseif ( $threadItem instanceof ContentCommentItem ) {
$id = 'c-' . $this->truncateForId( str_replace( ' ', '_', $threadItem->getAuthor() ) ) .
$id = 'c-' . $this->truncateForId( str_replace( ' ', '_', $threadItem->getAuthor() ), $legacy ) .
'-' . $threadItem->getTimestampString();
} else {
throw new InvalidArgumentException( 'Unknown ThreadItem type' );
@ -1305,9 +1319,9 @@ class CommentParser {
// in one edit, or within a minute), add the parent ID to disambiguate them.
$threadItemParent = $threadItem->getParent();
if ( $threadItemParent instanceof ContentHeadingItem && !$threadItemParent->isPlaceholderHeading() ) {
$id .= '-' . $this->truncateForId( $threadItemParent->getLinkableId() );
$id .= '-' . $this->truncateForId( $threadItemParent->getLinkableId(), $legacy );
} elseif ( $threadItemParent instanceof ContentCommentItem ) {
$id .= '-' . $this->truncateForId( str_replace( ' ', '_', $threadItemParent->getAuthor() ) ) .
$id .= '-' . $this->truncateForId( str_replace( ' ', '_', $threadItemParent->getAuthor() ), $legacy ) .
'-' . $threadItemParent->getTimestampString();
}
@ -1324,7 +1338,9 @@ class CommentParser {
if ( $previousItems->findCommentById( $id ) ) {
// Well, that's tough
$threadItem->addWarning( 'Duplicate comment ID' );
if ( !$legacy ) {
$threadItem->addWarning( 'Duplicate comment ID' );
}
// Finally, disambiguate by adding sequential numbers, to allow replying to both comments
$number = 1;
while ( $previousItems->findCommentById( "$id-$number" ) ) {
@ -1416,6 +1432,10 @@ class CommentParser {
$id = $this->computeId( $threadItem, $result );
$threadItem->setId( $id );
$legacyId = $this->computeId( $threadItem, $result, true );
if ( $legacyId !== $id ) {
$threadItem->setLegacyId( $legacyId );
}
$result->updateIdAndNameMaps( $threadItem );
}

View file

@ -26,6 +26,7 @@ abstract class ContentThreadItem implements JsonSerializable, ThreadItem {
protected string $name;
protected string $id;
protected ?string $legacyId = null;
/** @var ContentThreadItem[] */
protected array $replies = [];
/** @var string|bool */
@ -247,6 +248,13 @@ abstract class ContentThreadItem implements JsonSerializable, ThreadItem {
return $this->id;
}
/**
* @return string|null Thread ID, before most recent change to ID calculation
*/
public function getLegacyId(): ?string {
return $this->legacyId;
}
/**
* @return ContentThreadItem[] Replies to this thread item
*/
@ -300,6 +308,13 @@ abstract class ContentThreadItem implements JsonSerializable, ThreadItem {
$this->id = $id;
}
/**
* @param string|null $legacyId Thread ID
*/
public function setLegacyId( ?string $legacyId ): void {
$this->legacyId = $legacyId;
}
public function addWarning( string $warning ): void {
$this->warnings[] = $warning;
}

View file

@ -182,7 +182,8 @@ class ThreadItemStore {
}
$language = MediaWikiServices::getInstance()->getContentLanguage();
$heading = $language->truncateForDatabase( $heading, 80, '' );
// Mirrors CommentParser::truncateForId
$heading = trim( $language->truncateForDatabase( $heading, 80, '' ), '_' );
$dbw = $this->dbProvider->getPrimaryDatabase();
@ -526,10 +527,21 @@ class ThreadItemStore {
foreach ( $threadItemSet->getThreadItems() as $item ) {
$itemIdsId = $this->findOrInsertId(
static function ( IReadableDatabase $dbw ) use ( $item, $method ) {
$ids = [ $item->getId() ];
if ( $item->getLegacyId() !== null ) {
// Avoid duplicates if the item exists under the legacy ID
// (i.e. with trailing underscores in the title part).
// The actual fixing of IDs is done by a maintenance script
// FixTrailingWhitespaceIds, as archived talk pages are unlikely
// to be edited again in the future.
// Once FixTrailingWhitespaceIds has run on and enough time has
// passed, we can remove all legacy ID code (again).
$ids[] = $item->getLegacyId();
}
return $dbw->newSelectQueryBuilder()
->from( 'discussiontools_item_ids' )
->field( 'itid_id' )
->where( [ 'itid_itemid' => $item->getId() ] )
->where( [ 'itid_itemid' => $ids ] )
->caller( $method )
->fetchField();
},

View file

@ -0,0 +1,112 @@
<?php
namespace MediaWiki\Extension\DiscussionTools\Maintenance;
use LoggedUpdateMaintenance;
use Wikimedia\Rdbms\IDatabase;
use Wikimedia\Rdbms\IExpression;
use Wikimedia\Rdbms\LikeValue;
$IP = getenv( 'MW_INSTALL_PATH' );
if ( $IP === false ) {
$IP = __DIR__ . '/../../..';
}
require_once "$IP/maintenance/Maintenance.php";
class FixTrailingWhitespaceIds extends LoggedUpdateMaintenance {
public function __construct() {
parent::__construct();
$this->addDescription( 'Fix comment IDs with trailing whitespace' );
$this->setBatchSize( 1000 );
$this->requireExtension( 'DiscussionTools' );
}
/**
* @inheritDoc
*/
public function doDBUpdates() {
$dbw = $this->getDB( DB_PRIMARY );
$this->output( "Fixing DiscussionTools IDs with trailing whitespace..\n" );
$total = 0;
$skippedIds = [];
do {
// Match things that are possibly heading IDs with trailing underscores,
// possibly followed by a timestamp.
// As we are using LIKE there's a small chance of false positives, but
// this will become no-ops as we use a stricter RegExp later.
// Trailing underscore
// _-%\_
$like1 = new LikeValue( $dbw->anyChar(), '-', $dbw->anyString(), '_' );
// Trailing underscore followed by short timestamp
// _-%\_-2%00
$like2 = new LikeValue( $dbw->anyChar(), '-', $dbw->anyString(), '_-2', $dbw->anyString(), '00' );
// Trailing underscore followed by long timestamp
// _-%\_-2%00.000Z
$like3 = new LikeValue( $dbw->anyChar(), '-', $dbw->anyString(), '_-2', $dbw->anyString(), '00.000Z' );
$itemIdQueryBuilder = $dbw->newSelectQueryBuilder()
->from( 'discussiontools_item_ids' )
->field( 'itid_itemid' )
->where(
$dbw->makeList( [
$dbw->expr( 'itid_itemid', IExpression::LIKE, $like1 ),
$dbw->expr( 'itid_itemid', IExpression::LIKE, $like2 ),
$dbw->expr( 'itid_itemid', IExpression::LIKE, $like3 ),
], IDatabase::LIST_OR )
)
->caller( __METHOD__ )
->limit( $this->getBatchSize() );
if ( $skippedIds ) {
$itemIdQueryBuilder->where(
'itid_itemid NOT IN (' . $dbw->makeList( $skippedIds ) . ')'
);
}
$itemIds = $itemIdQueryBuilder->fetchFieldValues();
if ( !$itemIds ) {
break;
}
foreach ( $itemIds as $itemId ) {
$fixedItemId = preg_replace(
'/^([hc]\-.*)_(\-([0-9]{14}|[0-9-]{10}T[0-9:]{6}00.000Z))?$/',
'$1$2',
$itemId
);
if ( $fixedItemId === $itemId ) {
// In the rare case we got a false positive from the LIKE, add this to a list of skipped IDs
// so we don't keep selecting it, and end up in an infinite loop
$skippedIds[] = $itemId;
}
$dbw->newUpdateQueryBuilder()
->update( 'discussiontools_item_ids' )
->set( [ 'itid_itemid' => $fixedItemId ] )
->where( [ 'itid_itemid' => $itemId ] )
->caller( __METHOD__ )->execute();
$this->waitForReplication();
$total += $dbw->affectedRows();
$this->output( "$total\n" );
}
} while ( true );
$this->output( "Fixing DiscussionTools IDs with trailing whitespace: done.\n" );
return true;
}
/**
* @inheritDoc
*/
public function getUpdateKey() {
return 'DiscussionToolsFixTrailingWhitespaceIds';
}
}
$maintClass = FixTrailingWhitespaceIds::class;
require_once RUN_MAINTENANCE_IF_MAIN;

View file

@ -1004,11 +1004,13 @@ Parser.prototype.buildThreadItems = function () {
/**
* Truncate user generated parts of IDs so full ID always fits within a database field of length 255
*
* nb: Text should already have had spaces replaced with underscores by this point.
*
* @param {string} text Text
* @return {string} Truncated text
*/
Parser.prototype.truncateForId = function ( text ) {
return trimByteLength( '', text, 80 ).newVal;
return trimByteLength( '', text, 80 ).newVal.replace( /^_+|_+$/g, '' );
};
/**

View file

@ -3777,7 +3777,7 @@
"headingLevel": 2,
"level": 0,
"name": "h-MareBG-2018-04-17T12:43:00.000Z",
"id": "h-Грешка_у_изразу:_непознати_интерпункцијски_-2018-04-17T12:43:00.000Z",
"id": "h-Грешка_у_изразу:_непознати_интерпункцијски-2018-04-17T12:43:00.000Z",
"warnings": [],
"replies": [
{
@ -3802,7 +3802,7 @@
],
"level": 1,
"name": "c-MareBG-2018-04-17T12:43:00.000Z",
"id": "c-MareBG-2018-04-17T12:43:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-MareBG-2018-04-17T12:43:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -3857,7 +3857,7 @@
],
"level": 1,
"name": "c-НиколаБ-2018-04-17T17:45:00.000Z",
"id": "c-НиколаБ-2018-04-17T17:45:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-НиколаБ-2018-04-17T17:45:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
},
@ -3884,7 +3884,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T17:50:00.000Z",
"id": "c-BokicaK-2018-04-17T17:50:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T17:50:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -3939,7 +3939,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T18:40:00.000Z",
"id": "c-BokicaK-2018-04-17T18:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T18:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -3994,7 +3994,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T18:48:00.000Z",
"id": "c-BokicaK-2018-04-17T18:48:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T18:48:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4049,7 +4049,7 @@
],
"level": 1,
"name": "c-НиколаБ-2018-04-17T18:56:00.000Z",
"id": "c-НиколаБ-2018-04-17T18:56:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-НиколаБ-2018-04-17T18:56:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4131,7 +4131,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T19:33:00.000Z",
"id": "c-BokicaK-2018-04-17T19:33:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T19:33:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4186,7 +4186,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T18:58:00.000Z",
"id": "c-BokicaK-2018-04-17T18:58:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T18:58:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4269,7 +4269,7 @@
],
"level": 1,
"name": "c-Obsuser-2018-04-17T19:32:00.000Z",
"id": "c-Obsuser-2018-04-17T19:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-Obsuser-2018-04-17T19:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4406,7 +4406,7 @@
],
"level": 1,
"name": "c-Obsuser-2018-04-17T20:32:00.000Z",
"id": "c-Obsuser-2018-04-17T20:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-Obsuser-2018-04-17T20:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
},
@ -4433,7 +4433,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T20:37:00.000Z",
"id": "c-BokicaK-2018-04-17T20:37:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T20:37:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4544,7 +4544,7 @@
],
"level": 1,
"name": "c-Obsuser-2018-04-17T21:13:00.000Z",
"id": "c-Obsuser-2018-04-17T21:13:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-Obsuser-2018-04-17T21:13:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
},
@ -4571,7 +4571,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-18T02:40:00.000Z",
"id": "c-BokicaK-2018-04-18T02:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-18T02:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4708,7 +4708,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-18T05:44:00.000Z",
"id": "c-BokicaK-2018-04-18T05:44:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-18T05:44:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4763,7 +4763,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-18T23:27:00.000Z",
"id": "c-BokicaK-2018-04-18T23:27:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-18T23:27:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
}

View file

@ -3788,7 +3788,7 @@
"headingLevel": 2,
"level": 0,
"name": "h-MareBG-2018-04-17T12:43:00.000Z",
"id": "h-Грешка_у_изразу:_непознати_интерпункцијски_-2018-04-17T12:43:00.000Z",
"id": "h-Грешка_у_изразу:_непознати_интерпункцијски-2018-04-17T12:43:00.000Z",
"warnings": [],
"replies": [
{
@ -3813,7 +3813,7 @@
],
"level": 1,
"name": "c-MareBG-2018-04-17T12:43:00.000Z",
"id": "c-MareBG-2018-04-17T12:43:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-MareBG-2018-04-17T12:43:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -3867,7 +3867,7 @@
],
"level": 1,
"name": "c-НиколаБ-2018-04-17T17:45:00.000Z",
"id": "c-НиколаБ-2018-04-17T17:45:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-НиколаБ-2018-04-17T17:45:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
},
@ -3894,7 +3894,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T17:50:00.000Z",
"id": "c-BokicaK-2018-04-17T17:50:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T17:50:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -3948,7 +3948,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T18:40:00.000Z",
"id": "c-BokicaK-2018-04-17T18:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T18:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4002,7 +4002,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T18:48:00.000Z",
"id": "c-BokicaK-2018-04-17T18:48:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T18:48:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4056,7 +4056,7 @@
],
"level": 1,
"name": "c-НиколаБ-2018-04-17T18:56:00.000Z",
"id": "c-НиколаБ-2018-04-17T18:56:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-НиколаБ-2018-04-17T18:56:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4136,7 +4136,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T19:33:00.000Z",
"id": "c-BokicaK-2018-04-17T19:33:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T19:33:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4191,7 +4191,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T18:58:00.000Z",
"id": "c-BokicaK-2018-04-17T18:58:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T18:58:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4273,7 +4273,7 @@
],
"level": 1,
"name": "c-Obsuser-2018-04-17T19:32:00.000Z",
"id": "c-Obsuser-2018-04-17T19:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-Obsuser-2018-04-17T19:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4410,7 +4410,7 @@
],
"level": 1,
"name": "c-Obsuser-2018-04-17T20:32:00.000Z",
"id": "c-Obsuser-2018-04-17T20:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-Obsuser-2018-04-17T20:32:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
},
@ -4437,7 +4437,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-17T20:37:00.000Z",
"id": "c-BokicaK-2018-04-17T20:37:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-17T20:37:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4546,7 +4546,7 @@
],
"level": 1,
"name": "c-Obsuser-2018-04-17T21:13:00.000Z",
"id": "c-Obsuser-2018-04-17T21:13:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-Obsuser-2018-04-17T21:13:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
},
@ -4573,7 +4573,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-18T02:40:00.000Z",
"id": "c-BokicaK-2018-04-18T02:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-18T02:40:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4710,7 +4710,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-18T05:44:00.000Z",
"id": "c-BokicaK-2018-04-18T05:44:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-18T05:44:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": [
{
@ -4765,7 +4765,7 @@
],
"level": 1,
"name": "c-BokicaK-2018-04-18T23:27:00.000Z",
"id": "c-BokicaK-2018-04-18T23:27:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски_",
"id": "c-BokicaK-2018-04-18T23:27:00.000Z-Грешка_у_изразу:_непознати_интерпункцијски",
"warnings": [],
"replies": []
}