Page MenuHomePhabricator

Announce or revert ResultWrapper iteration change
Closed, ResolvedPublic

Description

rMWd9f688698ce0: rdbms: clean up and refactor ResultWrapper classes changed iteration in ResultWrapper from 1-based to 0-based as an unintended side effect: previously, foreach ( $resultWrapper as $i => $row ) {} was roughly equivalent to for ( $i = 1, $row = ...; $i <= $resultWrapper->numRows(); $row = ..., $i++ ) {} and now it's roughly equivalent to for ( $i = 0, $row = ...; $i < $resultWrapper->numRows(); $row = ..., $i++ ) {}.

This has caused T226640: ReadingLists CI broken / T226762: [Bug] GET /data/lists doesn't include "next" in an incomplete response and might well affect other code. For a class that implements Traversable, the index range should be considered part of the public interface, so this is a B/C break and should be either announced as such or reverted. (Also, the behavior should be documented and there should be tests against it.) Since 0-based is more intuitive, even if the change was unintentional, I'll go ahead with the announcement route if no one objects for a few days.

Details

Related Gerrit Patches:

Event Timeline

Tgr created this task.Jul 10 2019, 1:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 10 2019, 1:40 PM
Krinkle added a project: Performance-Team.
aaron added a comment.EditedJul 10 2019, 7:12 PM

I strongly prefer 0-based.

I can do the unit test part (plus grepping for iteration code).

Change 521934 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] Add ResultWrapper/FakeResultWrapper tests

https://gerrit.wikimedia.org/r/521934

Change 522087 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Release notes for ResultWrapper indexing change

https://gerrit.wikimedia.org/r/522087

Change 521934 merged by jenkins-bot:
[mediawiki/core@master] Add ResultWrapper/FakeResultWrapper tests

https://gerrit.wikimedia.org/r/521934

Change 522087 merged by jenkins-bot:
[mediawiki/core@master] Release notes for ResultWrapper indexing change

https://gerrit.wikimedia.org/r/522087

Tgr closed this task as Resolved.Jul 12 2019, 9:51 AM
Tgr assigned this task to aaron.

Sent a mail to wikitech-l. I think that covers all that can be reasonably expected to be done to notify reusers (DB operations are ubiquitous and there's nothing specific in the way they are operated so I doubt grepping the code would be manageable).