How do we know the review process is complete? Is there an established benchmark for improvement, or baseline criteria that need to be met, in order to begin endpoint implementation?

In T348489#9255576, @FGoodwin wrote:

How do we know the review process is complete? Is there an established benchmark for improvement, or baseline criteria that need to be met, in order to begin endpoint implementation?

Good question. That'll primarily be the responsibility for the person to whom the task is assigned (which, per sync discussion today is Wendy). The assignee should drive the task forward, ensuring that any necessary feedback from others has been collected and considered.

In practical terms, the reason that last week I asked yourself and Wendy to review this (and the reason I reviewed it myself) is that I thought we all probably needed to increase our familiarity with the code, and this would be a good chance. I suggest that each of us post whatever thoughts we have after that review as a comment on this task. Wendy can then take all that onboard in preparation for any code changes that may arise.

I'll post my thoughts shortly. In looking through ReadingListRepository, I also noticed a few things applicable to other tasks (and might even need another subtask). I'll post all that in the appropriate places.

Here are my notes/thoughts after reviewing ReadingListRepository (and related code):

• This is a general comment and not really a ReadingListRepository specific one, but I noticed the extension page on mediawiki.org (https://www.mediawiki.org/wiki/Extension:ReadingLists) claims support for 5.5+. However, extension.json says "MediaWiki": ">= 1.41" and discussion in Slack suggests that there's no compelling reason for us to restrict ourselves to supporting older versions of either php or mediawiki.
• the ReadingListRepository constructor comment defines $userId as int, but there is no type on the actual parameter. Should there be? Presumably, it wasn't there originally because php 5.5 didn't support that, but (per my first bullet point) we don't have to stick to that.
• on ReadingListRepository line 171, there's a small inconsistency in spacing around parenthesis. There is no functional effect, so this is purely a pedantic nitpick.
• Just as a note, trait ReadingListRow is only ever used (as in, used as a trait) by trait ReadingListRowWithMergeFlag. But it is referenced in various places. The "WithMergeFlag" version is never used (as a trait) that I can see, but it is referenced in several places. Per the comments in those traits, these exist only for documentation for the benefit of IDEs, so this is not unexpected. But it is the first time I recall seeing it. So I'm mentioning it in case it is unfamiliar to others as well.
• ApiTrait is used to create and interface with ReadingListRepository, and contains some Action API specifics. Do we want to refactor this trait? Or instead create a REST equivalent (either as a trait or as a base handler class)? Or maybe something else? The trait doesn't actually appear to perform database operations, but it does do things like translate between the db and the API for things like converting db query results to API responses.
• I'm getting an error running unit tests:MediaWiki\Extension\ReadingLists\Tests\UtilsTest::testGetDB, RuntimeException: Database backend disabled. I haven't yet looked into why. Could be something about my local configuration, I suppose. This isn't specifically related to ReadingListRepository, but I mention it here because the error is db-related. Again, it may turn out there are no changes necessary for this (I assume the test passes in CI), but as something to be aware of in case anyone else sees it too.

In summary, the only thing of real substance that I saw for this task was to consider how to do something along the lines of ApiTrait in a way suitable for the REST handlers (whether by adapting ApiTrait, or via some other approach). Other than that, I just saw a few minor nitpicky things.

Oh, one more thing. This isn't strictly related to either ReadingListRepository, or to the database layer even, but posting here so we don't forget: I had a transient test failure for ApiQueryReadingListsTest::testApiQuery() that appeared to be because $this->lastUpdate was captured a second (or more specifically, across the boundary of one second to the next) differently than the test data insertion:

-                'updated' => '2023-10-16T19:44:23Z'
+                'updated' => '2023-10-16T19:44:24Z'

How could we rearrange the test to be less susceptible to this effect?

It's a DAO class, it could be updated to incorporate the various changes to the DB framwork (IConnectionProvider, query builders, virtual database domains).

Per review, and per discussion in Code Mob, we have decided to make no changes to the ReadingListRepository class as part of the Reading List REST Interface changes. In summary, it seems preferable to not touch existing, working code, and the ReadingListRepository class itself should be usable from REST handlers as-is.

What still does need to be done is to create a convenient way to use ReadingListRepository from the to-be-created REST handlers. For the Action API modules exposed by the extension, ApiTrait served this purpose. But that trait is specific to the Action API (for example, its "factory" function accepts an ApiBase). So we'll need something different.

I will therefore close this task as declined and create a new task for that purpose.

It is worth mentioning that the things mentioned in @Tgr 's comment may still be worth doing. But we don't plan to do them as part of the current effort. I'll create a task for that as well, tagged for the extension. But API Platform does not plan to take that on as part of the current effort.