Change 862934 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] WIP: Add PageTriageQueueManager

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

Change 863239 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] Introduce PageTriageQueueLookup

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

Change 863244 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] UpdatePageTriageQueue: Use QueueManager, add deleteByPageIds

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

Change 863245 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] QueueRecord: Define review status as const

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

Change 863249 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] PageTriageUtil: Use QueueLookup service

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

Change 863250 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] Hooks::isNewEnoughToNoIndex: Use QueueLookup

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

Hey @kostajh. Can you talk a little bit more about what you're doing? Are you rewriting the PageTriage class to be a QueueManager class, which is similar since they operate on the same SQL table (pagetriage_page), but whose methods more closely correspond to CRUD operations? What design pattern and what software architecture terms should I be googling to understand what you're doing? What are the benefits of the new style?

Do you have any concerns about breaking anything? We don't really have Selenium tests up yet and that might be a good way to double check that our refactoring doesn't break any happy paths.

Even when I get +2, you may need to get a member of your team to review those patches. I don't quite have my head wrapped around it yet.

I'm excited to learn though. I imagine this is the style that modern MediaWiki extensions should be written in, so I plan to follow along and learn it. Maybe long-term I can watch you fix up this extension, then I can use that as a blueprint to fix other extensions.

I see you put several hours/several patches in yesterday. Thanks for tackling this.

In T324211#8439899, @Novem_Linguae wrote:

Hey @kostajh. Can you talk a little bit more about what you're doing? Are you rewriting the PageTriage class to be a QueueManager class, which is similar since they operate on the same SQL table (pagetriage_page), but whose methods more closely correspond to CRUD operations? What design pattern and what software architecture terms should I be googling to understand what you're doing? What are the benefits of the new style?

Good questions! I am trying to map the functionality that implicitly exists in PageTriage to some more well-defined services and classes that have more narrow responsibility. So, where PageTriage.php currently handles CRUD operations *and* also exists as a data object *and* is strongly coupled to the underlying SQL tables, I'd like to see something like this:

• a service class for managing adding/updating/deleting items from the "queue" of pages that PageTriage shows to users. I've tentatively named this QueueManager.
• a service class for retrieving items from the queue, either by page ID, or by other characteristics. I've tentatively named this QueueLookup
• a value object for modeling items in the queue. I've tentatively named this QueueRecord.
• I'm not sure yet how to think about the ArticleCompileMetadata class (which has similar problems)

https://www.mediawiki.org/wiki/Architecture_guidelines#Separation_of_concerns_%E2%80%94_encapsulation_versus_value_objects is probably a good reference for this type of refactoring.

Overall these are exploratory patches; we could always change how things are modeled/named, and maybe change the level of abstraction, service boundaries, etc.

Do you have any concerns about breaking anything? We don't really have Selenium tests up yet and that might be a good way to double check that our refactoring doesn't break any happy paths.

A little? But I think with tests, we should be OK. For this type of refactoring, I think phpunit tests will be more useful.

Even when I get +2, you may need to get a member of your team to review those patches. I don't quite have my head wrapped around it yet.

Fair enough!

I'm excited to learn though. I imagine this is the style that modern MediaWiki extensions should be written in, so I plan to follow along and learn it. Maybe long-term I can watch you fix up this extension, then I can use that as a blueprint to fix other extensions.

Sounds good!

I see you put several hours/several patches in yesterday. Thanks for tackling this.

yw! Thank you for filing these refactoring tasks which motivated me to investigate and put some code out for review.

Change 862934 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Introduce PageTriageQueueManager

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

Change 863239 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Introduce PageTriageQueueLookup and QueueRecord

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

Change 863244 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] UpdatePageTriageQueue: Use QueueManager, add deleteByPageIds

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

Change 869849 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] Hooks: Remove unused parameter from addToPageTriageQueue

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

Change 863245 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] QueueRecord/PageTriage: Make review status an int

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

Change 863249 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] PageTriageUtil: Use QueueLookup service

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

Change 863250 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Hooks::isNewEnoughToNoIndex: Use QueueLookup

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

Change 869849 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Hooks: Remove unused parameter from addToPageTriageQueue

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

Change 870557 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/PageTriage@master] QueueManager: Add insert method and use it from PageTriage class

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

Change 870557 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] QueueManager: Add insert method and use it from PageTriage class

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

Ok, I think I finally wrapped my head around this design pattern. So the idea is...

• "Manager" classes do database write operations (create, update, delete)
• "Lookup" classes do database read operations, and are a factory for the Record classes
• "Record" classes are data objects

One thing that's confusing is that the names of the classes do not correspond to the SQL database tables they're touching. So for example, Queue touches pagetriage_page, and if we refactor ArticleMetadata the same way, then ArticleMetadata will touch pagetriage_page_tags. I wonder if there's an opportunity somewhere to make the names correspond to each other.

Overall, looks good, thanks for all the time you spent on this Kosta.

Below is Tgr's work in progress patch to convert PageTriage's ArticleMetadata class to use a better design pattern. The patch is abandoned, but noting here for the record. Could be a starting point for refactoring in the future.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/869342