Steps to reproduce:
- Go to Special:AbuseFilter.
- Select "Include deleted filters" and update search.
- Click status in the table header to order it by status.
While all enabled filters are together, disabled and deleted ones are mixed.
matej_suchanek | |
Apr 7 2018, 12:48 PM |
F16789246: obrazek.png | |
Apr 7 2018, 12:48 PM |
Steps to reproduce:
While all enabled filters are together, disabled and deleted ones are mixed.
Confirmed. The reason is pretty simple to explain: the 'status' column puts together af_enabled and af_deleted, which are completely separated in the DB. The sorting is performed on af_enabled, which of course doesn't take into account af_deleted. The solution might be to add a condition to ORDER BY so that the two values are handled separately, but this requires overriding something from parent (TablePager/IndexPager). I'll give it a look.
Change 445932 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use af_deleted as secondary sorting for af_enabled
Change 445932 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use af_deleted as secondary sorting for af_enabled
Hm, apparently the results aren't mixed anymore. However, some results are missing -- specifically, if I click "next page" on the first page I go straight to the last page. The weird thing is, apparently it was already broken without the patches above.
Ah, this is actually pretty clear. Neither af_enabled nor af_deleted are unique indexes, but IndexPager::getIndexField warns:
Needless to say, it's really not a good idea to use a non-unique index for this! That won't page right.
And that's what's happening, in fact, the URL says "&offset=1". Perhaps we should add an extra sort key (af_id), but it might not be enough.
Investigation: I applied the following override in AbuseFilterPager and analyzed the queries, but it didn't work and always sorted on af_id (which is default):
public function getIndexField() { return array_filter( [ 'af_id' => [ 'af_id' ], 'af_enabled' => [ 'af_enabled', 'af_deleted', 'af_id' ], 'af_timestamp' => [ 'af_timestamp' ], 'af_hidden' => [ 'af_hidden', 'af_id' ], 'af_group' => [ 'af_group', 'af_id' ], 'af_hit_count' => [ 'af_hit_count', 'af_id' ], 'af_public_comments' => [ 'af_public_comments' ], ], [ $this, 'isFieldSortable' ], ARRAY_FILTER_USE_KEY ); }
The problem is incompatibility of core's TablePager and IndexPager. The following is from IndexPager::__construct:
$order = $this->mRequest->getVal( 'order' ); if ( is_array( $index ) && isset( $index[$order] ) ) { $this->mOrderType = $order; $this->mIndexField = $index[$order]; $this->mExtraSortFields = isset( $extraSort[$order] ) ? (array)$extraSort[$order] : []; }
And the following from TablePager::getStartBody:
// Make table header foreach ( $fields as $field => $name ) { if ( strval( $name ) == '' ) { $s .= Html::rawElement( 'th', [], "\u{00A0}" ) . "\n"; } elseif ( $this->isFieldSortable( $field ) ) { $query = [ 'sort' => $field, 'limit' => $this->mLimit ];
TablePager generates links with sort= but IndexPager uses order= for its paging. So TablePager needs to be updated first, or we need to use a hack to set order= before IndexPager's constructor.
Not actively working on this, but I we should continue working on this task later on as part of the overhaul.
Change 810412 had a related patch set uploaded (by Umherirrender; author: Umherirrender):
[mediawiki/extensions/AbuseFilter@master] Special:AbuseFilter: Include primary key for unique pagination
Change 810412 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Special:AbuseFilter: Include primary key for unique pagination