Page MenuHomePhabricator

Review and deploy DynamicPageListEngine
Open, LowPublic

Description

Per T76844: Deploy DynamicPageListEngine extension to Vietnamese Wiktionary, Review queue is needed for installing DynamicPageListEngine extension to Vietnamese Wiktionary.

Event Timeline

Bugreporter raised the priority of this task from to High.
Bugreporter updated the task description. (Show Details)
Bugreporter added subscribers: Ricordisamoa, greg, brion and 16 others.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 24 2015, 12:25 PM
Aklapper lowered the priority of this task from High to Low.Feb 24 2015, 1:28 PM
Aklapper removed a project: Tracking-Neverending.
Aklapper set Security to None.

Any extension that starts with "DynamicPageList" gives me a bad feeling :/. It looks like step 5 was skipped, I don't see the code in gerrit yet?

Mjbmr removed a subscriber: Mjbmr.Feb 24 2015, 4:19 PM
TTO added a subscriber: TTO.Oct 15 2015, 9:14 AM

English Wiktionary is currently holding a vote to install this extension on that site. Note that the vote has not concluded, but looks like it will pass.

It would be great to have a preliminary review to determine whether the extension has any chance of being installed on the WMF cluster. If the overall architecture looks generally good, then we can move ahead and get the code into Gerrit. Otherwise, there is no point wasting the effort of migrating the extension to Gerrit, only to be told "No" by the arch reviewer.

For anyone willing to quickly review this, the code is currently in a zip file at Sourceforge (ugh): http://sourceforge.net/projects/mwdatatable/files/misc/DynamicPageListEngine-1.0.0.zip/download

In T90573#1727507, @TTO wrote:

English Wiktionary is currently holding a vote to install this extension on that site.
[...]
there is no point wasting the effort of migrating the extension to Gerrit, only to be told "No" by the arch reviewer.

+1. Thanks. This is important.

As a precautious measure, I have added a link to meta:Limits to configuration changes in the "See Also" section of mw:Review queue.
Note that this is not any statement on likeability/probability of extension deployment but just for informational purposes.

Reedy added a comment.EditedOct 15 2015, 12:08 PM
In T90573#1727507, @TTO wrote:

English Wiktionary is currently holding a vote to install this extension on that site. Note that the vote has not concluded, but looks like it will pass.
It would be great to have a preliminary review to determine whether the extension has any chance of being installed on the WMF cluster. If the overall architecture looks generally good, then we can move ahead and get the code into Gerrit. Otherwise, there is no point wasting the effort of migrating the extension to Gerrit, only to be told "No" by the arch reviewer.
For anyone willing to quickly review this, the code is currently in a zip file at Sourceforge (ugh): http://sourceforge.net/projects/mwdatatable/files/misc/DynamicPageListEngine-1.0.0.zip/download

Annoying, it doesn't look to be hosted in version control to begin with...

I had a quick look, it will need a decent amount of cleanup at least (why does it have ?> everywhere!?). That's without looking at architecture and performance etc

Reedy edited subscribers, added: RV1971; removed: wikibugs-l-list.Dec 13 2015, 3:37 PM
Reedy added a comment.Dec 13 2015, 3:52 PM

I've just initially imported it to https://github.com/reedy/DynamicPageListEngine for people to look at the code more easily. Tagged initial v1.0.0 before making any further changes

Reedy added a comment.Dec 13 2015, 4:36 PM

PHPStorm doesn't see anything too bad, but stuff that does want cleaning up.

Some PHP4-isms that want fixing

public function &getFeatures() {

Some manual string building for SQL queries, should be reusing MW wrappers.

	public function __construct( array $params, array &$features ) {
		parent::__construct( $params, $features,
			'imagecontainer', NS_MAIN,
			'imagelinks', 'ilx', 'il_to',
			array( 'page_title = $table.il_to',
				'page_namespace = ' . NS_FILE,
				'$table.il_from = $id' ) );
	}

Various table aliasing, many times in one query etc

I'd like to see some of the various SQL queries that the extension is likely to run, and see how they look

	/**
	 * @brief Modify a given query.
	 *
	 * Add table aliases `{$tableAlias_}1, {$tableAlias_}2, ...` to
	 * DpleQuery::$tables_.
	 *
	 * @param DpleQuery $query Query object.
	 */
	public function modifyQuery( DpleQuery &$query ) {
		$dbr = $query->getDbr();
		$tableName = $dbr->tableName( $this->tableName_ );
		$n = 1;

		/** Add conditions based on @ref $linkedTitles_. */
		for ( $i = 0; $i < $this->linkedCount_; $i++ ) {
			$table = "$tableName AS {$this->tableAlias_}$n";

			$query->addTables( $table );

			$query->addJoinCond( $table, 'INNER JOIN',
				$this->transformJoinConds_( $dbr, $n,
					$this->linkedTitles_[$i] ) );

			$n++;
		}

		/** Add conditions based on @ref $notLinkedTitles_. */
		for ( $i = 0; $i < $this->notLinkedCount_; $i++ ) {
			$table = "$tableName AS {$this->tableAlias_}$n";

			$query->addTables( $table );

			$query->addJoinCond( $table, 'LEFT OUTER JOIN',
				$this->transformJoinConds_( $dbr, $n,
					$this->notLinkedTitles_[$i] ) );

			$query->addConds(
				array( "{$this->tableAlias_}{$n}.{$this->tableColumn_}"
					=> null ) );
			$n++;
		}
	}
Restricted Application added a subscriber: PokestarFan. · View Herald TranscriptJul 21 2017, 4:06 PM
Ejs-80 added a subscriber: Ejs-80.Feb 4 2019, 1:58 PM