Page MenuHomePhabricator
Create Task
Maniphest T245964

Split up and servicify ChangeTags class
Closed, ResolvedPublic
Actions

Description

Pending:

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/597913/
Move defineTag, undefineTag, deleteTagEverywhere, and purgeTagCacheAll to new ChangeTagStore service

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/598222/
Move getSoftwareTags, listSoftwareActivatedTags, listDefinedTags, listExplicitlyDefinedTags, and listSoftwareDefinedTags to new ChangeTagLookup service

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/598226/
Move modifyDisplayQuery and makeTagSummarySubquery to new ChangeTagQueryUtils service

Remaining work to split up the class into narrow services tracked at P11294

Details

SubjectRepoBranchLines +/-
TagsDef: inject ChangeTagsStoremediawiki/coremaster+25 -6
Add a ChangeTagLookup service for looking up different lists of tagsmediawiki/coremaster+722 -108
Add a new ChangeTagQueryUtils service for adding tag info to queriesmediawiki/coremaster+499 -76
specials: Migrate off some calls to method ChangeTags::modifyDisplayQuerymediawiki/coremaster+40 -14
Add a ChangeTagStore service for defining and undefining tagsmediawiki/coremaster+516 -68
changetags: Move ::modifyDisplayQuery from ChangeTags to ChangeTagsStoremediawiki/coremaster+112 -76
Start using ChangeTagsStoremediawiki/coremaster+51 -22
changetags: Move more functions from ChangeTags to ChangeTagsStoremediawiki/coremaster+339 -282
changetags: Move a bit more from ChangeTags to ChangeTagsStoremediawiki/coremaster+82 -59
Introduce ChangeTagsStore service and move some functions of ChangeTagsmediawiki/coremaster+444 -229
Customize query in gerrit

Related Objects
Search...

Event Timeline

DannyS712 created this task.Feb 24 2020, 3:44 AM
Restricted Application added a project: User-DannyS712. · View Herald TranscriptFeb 24 2020, 3:44 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.Feb 24 2020, 3:44 AM
Peter.ovchyn subscribed.Feb 24 2020, 3:53 PM

Encapsulating a global state into a class is always a good idea.
At the moment I'm not quite familiar with that area, so can't give you more valuable suggestion.

But I'd appreciate it if you add me as Reviewer.

DannyS712 updated the task description. (Show Details)May 11 2020, 8:56 AM
DannyS712 updated the task description. (Show Details)May 17 2020, 11:44 PM
matej_suchanek subscribed.May 21 2020, 6:55 AM

::tagShortDescriptionMessage, ::tagDescription, ::tagLongDescriptionMessage, ::truncateTagDescription, ::isTagNameValid
should be moved to a ChangeTag (singular) object that represents a specific tag, including providing its description methods

The patch for T27909 does this (sort of).

gerritbot added a comment.May 22 2020, 2:32 AM

Change 597913 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a ChangeTagStore service for defining and undefining tags

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

gerritbot added a project: Patch-For-Review.May 22 2020, 2:32 AM
DannyS712 moved this task from Next to In progress on the User-DannyS712 board.May 22 2020, 2:34 AM
DannyS712 updated the task description. (Show Details)
DannyS712 added a comment.May 22 2020, 2:44 AM
In T245964#6157388, @gerritbot wrote:

Change 597913 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a ChangeTagStore service for defining and undefining tags

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

Next up: ChangeTagLookup, which replaces:

  • getSoftwareTags
  • listSoftwareActivatedTags
  • listDefinedTags
  • listExplicitlyDefinedTags
  • listSoftwareDefinedTags
DannyS712 updated the task description. (Show Details)May 22 2020, 3:02 AM
DannyS712 added a project: Platform Team Workboards (External Code Reviews).May 22 2020, 4:09 AM
gerritbot added a comment.May 24 2020, 6:18 AM

Change 598222 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a ChangeTagLookup store for looking up different lists of tags

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

DannyS712 updated the task description. (Show Details)May 24 2020, 6:23 AM
DannyS712 renamed this task from Factor some static methods out of the ChangeTags class to Split up and servicify ChangeTags class.May 24 2020, 6:29 AM
DannyS712 updated the task description. (Show Details)
DannyS712 added a project: Dependency injection.
gerritbot added a comment.May 24 2020, 9:10 AM

Change 598226 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a new ChangeTagQueryUtils service for adding tag info to queries

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

DannyS712 updated the task description. (Show Details)May 24 2020, 9:13 AM
DannyS712 added a comment.May 24 2020, 9:22 AM

Notes to help with review:

  • Each patch adds a narrow service for a clear group of methods in ChangeTags that go together, rather than trying to add one service for everything
  • Each patch only adds the new service and updates the ChangesTags method to use it, making no other replacements and making no official deprecations
  • Each patch includes 100% code coverage for the new service with pure unit testing
DannyS712 mentioned this in T157658: Factor out a backend from EditPage.Jun 4 2020, 3:14 AM
Ostrzyciel subscribed.Jul 8 2020, 8:47 AM
• eprodromou triaged this task as Medium priority.Sep 8 2020, 9:02 PM
DannyS712 removed DannyS712 as the assignee of this task.Sep 11 2020, 9:00 PM
Umherirrender added a parent task: T259960: Inject services into API modules and special pages.Oct 15 2020, 7:41 PM
Daimona subscribed.Oct 17 2020, 10:32 AM
matej_suchanek added a comment.Feb 20 2022, 9:59 AM
In T245964#6154623, @matej_suchanek wrote:

::tagShortDescriptionMessage, ::tagDescription, ::tagLongDescriptionMessage, ::truncateTagDescription, ::isTagNameValid
should be moved to a ChangeTag (singular) object that represents a specific tag, including providing its description methods

The patch for T27909 does this (sort of).

I have abandoned that patch as the feature was implemented in a different, simpler way. In the meantime, there were some additions and tweaks to the class (P11294 should be updated), so it's better to think again and start from scratch, rather than going through the rebase pain.

gerritbot added a comment.May 7 2023, 10:23 PM

Change 916816 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Introduce ChangeTagsStore service and move some functions of ChangeTags

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

Ladsgroup subscribed.EditedMay 7 2023, 10:27 PM

Don't make three different services. One to hold them all is enough. Adding more adds complexity, confusion and harder to onboard people. ChangeTags is a textbook "Table Gateway" pattern and good as is.

Don't break classes that don't need breaking. Interfaces should be narrow but classes can be busy and large ("deep module" from philosophy of software design book)

gerritbot added a comment.May 19 2023, 8:58 AM

Change 916816 merged by jenkins-bot:

[mediawiki/core@master] Introduce ChangeTagsStore service and move some functions of ChangeTags

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

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.10; 2023-05-23).May 19 2023, 9:00 AM
gerritbot added a comment.May 24 2023, 9:24 PM

Change 922937 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] changetags: Move a bit more from ChangeTags to ChangeTagsStore

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

gerritbot added a comment.May 31 2023, 10:27 AM

Change 922937 merged by jenkins-bot:

[mediawiki/core@master] changetags: Move a bit more from ChangeTags to ChangeTagsStore

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

ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.12; 2023-06-06); removed MW-1.41-notes (1.41.0-wmf.10; 2023-05-23).May 31 2023, 11:00 AM
gerritbot added a comment.Jun 2 2023, 11:29 AM

Change 926469 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] changetags: Move more functions from ChangeTags to ChangeTagsStore

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

gerritbot added a comment.Jun 8 2023, 8:05 AM

Change 926469 merged by jenkins-bot:

[mediawiki/core@master] changetags: Move more functions from ChangeTags to ChangeTagsStore

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

ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.13; 2023-06-13); removed MW-1.41-notes (1.41.0-wmf.12; 2023-06-06).Jun 8 2023, 9:01 AM
gerritbot added a comment.Jun 8 2023, 11:33 PM

Change 928682 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Start using ChangeTagsStore

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

gerritbot added a comment.Jun 9 2023, 1:19 PM

Change 928842 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] changetags: Move ::modifyDisplayQuery from ChangeTags to ChangeTagsStore

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

gerritbot added a comment.Jun 9 2023, 5:39 PM

Change 928682 merged by jenkins-bot:

[mediawiki/core@master] Start using ChangeTagsStore

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

gerritbot added a comment.Jun 15 2023, 12:31 PM

Change 928842 merged by jenkins-bot:

[mediawiki/core@master] changetags: Move ::modifyDisplayQuery from ChangeTags to ChangeTagsStore

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

ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.15; 2023-06-27); removed MW-1.41-notes (1.41.0-wmf.13; 2023-06-13).Jun 15 2023, 1:00 PM
gerritbot added a comment.Jun 15 2023, 11:18 PM

Change 930728 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] specials: Migrate some of calls to deprecated method ChangeTags::modifyDisplayQuery

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

gerritbot added a comment.Jun 16 2023, 10:05 AM

Change 597913 abandoned by DannyS712:

[mediawiki/core@master] Add a ChangeTagStore service for defining and undefining tags

Reason:

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

gerritbot added a comment.Jun 18 2023, 2:18 AM

Change 930728 merged by jenkins-bot:

[mediawiki/core@master] specials: Migrate off some calls to method ChangeTags::modifyDisplayQuery

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

gerritbot added a comment.Jun 22 2023, 12:19 PM

Change 598226 abandoned by DannyS712:

[mediawiki/core@master] Add a new ChangeTagQueryUtils service for adding tag info to queries

Reason:

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

gerritbot added a comment.Jun 22 2023, 12:19 PM

Change 598222 abandoned by DannyS712:

[mediawiki/core@master] Add a ChangeTagLookup service for looking up different lists of tags

Reason:

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

Maintenance_bot removed a project: Patch-For-Review.Jun 22 2023, 12:30 PM
Ladsgroup closed this task as Resolved.Jun 22 2023, 1:04 PM
Ladsgroup claimed this task.

All of those functions mentioned in the description have been moved to ChangeTagsStore. I did some clean ups of their usages but much more needs to be done and that's out of scope of this ticket.

gerritbot added a comment.Sep 10 2023, 6:37 PM

Change 956053 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] TagsDef: inject ChangeTagsStore

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

gerritbot added a project: Patch-For-Review.Sep 10 2023, 6:37 PM
gerritbot added a comment.Sep 11 2023, 11:28 AM

Change 956053 merged by jenkins-bot:

[mediawiki/core@master] TagsDef: inject ChangeTagsStore

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

Maintenance_bot removed a project: Patch-For-Review.Sep 11 2023, 11:30 AM
ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.26; 2023-09-12); removed MW-1.41-notes (1.41.0-wmf.15; 2023-06-27).Sep 11 2023, 12:00 PM
Jdforrester-WMF mentioned this in T360664: Stop using, deprecate, and then remove various static ChangeTags methods moved to ChangeTagsStore.Mar 21 2024, 4:22 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL