Page MenuHomePhabricator

TablePager does not let you specify multi-column ordering correctly
Open, Needs TriagePublic

Description

Suppose you're using TablePager to paginate some data with few unique columns. For instance, let's say that our table has mytable_id (unique), mytable_name (not unique) and mytable_date (not unique), and you want to allow sorting by name and date. You cannot do this by simply overriding isFieldSortable because the fields are not unique. According to the documentation in IndexPager::getIndexField(), the following should work:

public function getIndexField(): array {
	return [
		'mytable_name' => [ 'mytable_name', 'mytable_id' ],
		'mytable_date' => [ 'mytable_date', 'mytable_id' ],
	];
}

but it doesn't: it always uses the first element (mytable_name), regardless of what the actual sorting is. This was also discovered and reported by @matej_suchanek at T191694#6469506. This seems to be caused by an inconsistent naming in TablePager vs IndexPager, since the former uses "sort" but the latter uses "order".

AbuseFilter has an example of this, and we have another in the CampaignEvents extension.

Event Timeline

As Workround: In ImageListPager::class the choose for the needed columns is done directly in the function and only the needed column names are returned, which seems still to work since added in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/618855
But needs some extra work.

As Workround: In ImageListPager::class the choose for the needed columns is done directly in the function and only the needed column names are returned, which seems still to work since added in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/618855
But needs some extra work.

Thanks for the suggestion! Indeed, the following seems to work:

public function getIndexField(): array {
	return [ [
		'mytable_name' => [ 'mytable_name', 'mytable_id' ],
		'mytable_date' => [ 'mytable_date', 'mytable_id' ],
	][$this->mSort] ];
}

but as you said, this is really a workaround and I'd hope for a better solution... I'm also a bit confused by the documentation of IndexPager::getIndexField(), so I'm not sure if I'm doing everything right...