Change 983241 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/extensions/ReadingLists@master] REST Handlers for managing reading lists

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

Change 983241 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] REST Handlers for managing reading lists

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

Added the following parameters that were previously exposed by the underlying Action API endpoint, but not by RESTBase:

Confirmed. Doesn't look that bad to fix, based on a quick initial triage.

Daniel and I had a fast look at this. We may have missed something, but it looks like the underlying data is still available via ReadingListRepository. The two functions of interest are:

ReadingListRepository::getListsByDateUpdated
ReadingListRepository::getListEntriesByDateUpdated

Previously, based on our quick analysis, it looks like under the RESTBase/Action API implementation of the lists/changes/since data from two Action API modules was combined. In the Action API implementation, setting the changedsince parameter set activates MODE_CHANGES in both modules:

ApiQueryReadingListEntries
ApiQueryReadingLists

The relevant bit in RESTBase appears to be in lists.yaml.

From a MW REST perspective, we have ListsChangesSinceHandler::execute, which ultimately calls ReadingListRepository:::getListsByDateUpated. There isn't any corresponding code to call ReadingListRepository::getListEntriesByDateUpdated.

So it looks like the fix is to modify ListsChangesSinceHandler::execute (and maybe a bit of related code) to make that additional call for entry change information, and include it in the response.

Update: I'm working through these change locally. Previously, we had some handlers that were aware of lists, and others that were aware of entries. As it turns out, the ListsChangesSinceHandler will need to be aware of both. I could do a quick-and-dirty version with lots of copy and paste, but I'd rather rearrange things in a clean way. All that is pretty straightforward, just code shuffling.

I do notice that continuation is a little interesting in this case, so I'll want reviewers to pay special attention to that, when the change is ready.

Change #1166509 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/extensions/ReadingLists@master] Reading Lists: add missing "entries" values to response

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

In the process of fixing this, I've come across an oversight in the original port of the endpoints.

For compatibility with the existing RESTBase/Action API implementation, the /changes/since endpoint should never have accepted "name" as a valid sort value.

The relevant Action API code is:

		if ( $mode === self::$MODE_CHANGES && $sort === 'name' ) {
			// We don't have the right DB index for this. Wouldn't make much sense anyway.
			$errorMessage = $this->msg( 'apierror-readinglists-invalidsort-notbyname', static::$prefix );
			$this->dieWithError( $errorMessage, 'invalidparammix' );
		}

I did not mirror this check when creating the MW REST API equivalent, so the MW REST version of /changes/since has been accepting "name" as a valid sort value since that endpoint was released. The fix is to have the endpoint return an error if "name" is passed for the sort value (and the code for that is trivial).

I'm very tempted to just fix this as a bug as part of the larger fix, without modifying endpoint version. But it is technically a breaking change to the behavior of that endpoint.

Any objections to treating this as a fix and not a change?

In T351149#11001516, @BPirkle wrote:

Any objections to treating this as a fix and not a change?

No objections here -- neither of the apps uses a sort parameter when making that call.

In T351149#11004565, @Dbrant wrote:

In T351149#11001516, @BPirkle wrote:

Any objections to treating this as a fix and not a change?

No objections here -- neither of the apps uses a sort parameter when making that call.

Thanks. The change I posted for review bumps the patch version (0.1.0 => 0.1.1) in the module definition file, but doesn't change the url in any way. So callers will be unaffected, but we'll have a way to tell, by looking at the OpenAPI spec, if a particular wiki has this change.

Change #1166509 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Reading Lists: add missing "entries" values to /changes/since response

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