Page MenuHomePhabricator

Add a factory service for `ManualLogEntry` objects
Open, Needs TriagePublic

Description

I propose creating a factory for ManualLogEntrys that would injected the needed dependencies:

  • A DBLoadBalancer (::insert accepts an optional database, falling back to wfGetDB, and ::publish always uses wfGetDB)
  • A CommentStore (currently retrieved via CommentStore::getStore())
  • A ActorMigration (currently retrieved via ActorMigration::newMigration())
  • A LoggerInstance (currently retrieved via LoggerFactory::getInstance())
  • A HookContainer (currently runs hooks, will need the service once migration of callers is complete)

Event Timeline

Restricted Application added a project: User-DannyS712. · View Herald TranscriptMay 27 2020, 5:48 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.May 27 2020, 5:48 AM
DannyS712 updated the task description. (Show Details)
Pchelolo added a subscriber: Pchelolo.

ManualLogEntry implements a LogEntry interface, and has a sibling: DatabaseLogEntry with RCDatabaseLogEntry as a specialized dependent. DatabaseLogEntry is only created via static factory methods, not via direct construction.

I believe if you are creating a factories for LogEntry, it would be nice to eventually incorporate all LogEntry instantiation methods in there. And also to declare them returning LogEntry interface, and not a specific class. I am not proposing to more DatabaseLogEntry now, but having it planned would affect naming of the new factory. So, I propose to have LogEntryFactory::newManual( ... ) : LogEntry instead of ManualLogEntryFactory::createLogEntry( ... ) : ManualLogEntry.

Otherwise, plan is looking good.

ManualLogEntry implements a LogEntry interface, and has a sibling: DatabaseLogEntry with RCDatabaseLogEntry as a specialized dependent. DatabaseLogEntry is only created via static factory methods, not via direct construction.

I believe if you are creating a factories for LogEntry, it would be nice to eventually incorporate all LogEntry instantiation methods in there. And also to declare them returning LogEntry interface, and not a specific class. I am not proposing to more DatabaseLogEntry now, but having it planned would affect naming of the new factory. So, I propose to have LogEntryFactory::newManual( ... ) : LogEntry instead of ManualLogEntryFactory::createLogEntry( ... ) : ManualLogEntry.

Otherwise, plan is looking good.

Will do; is this then approved from a product point of view? (also I would use newManualEntry to have a clear noun in the method name, but thats just me)

Will do; is this then approved from a product point of view?

I think it makes sense, yes.

Actually, after a bit of more thinking, I wonder if LogEntry should instead be converted into a value object - only holding the fields with getters/setters and containing no logic, and database operations, and all the stuff that uses the dependency should be done via a separate service, or even a group of services.

DannyS712 added a comment.EditedMay 28 2020, 11:02 PM

Actually, after a bit of more thinking, I wonder if LogEntry should instead be converted into a value object - only holding the fields with getters/setters and containing no logic, and database operations, and all the stuff that uses the dependency should be done via a separate service, or even a group of services.

Sure:
LogEntryStore:

  • ::insertLogEntry to replace ManualLogEntry::insert
    • Injected dependencies: LoadBalancer, CommentStore, ActorMigration

would be a step in the right direction

That being said, given how widely used the class is I think it makes sense to start off by adding a factory, so that if later the database stuff is moved there is already a class to construct the objects from the database

Change 601340 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a new LogEntryFactory for constructing ManualLogEntry objects

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

Actually, after a bit of more thinking, I wonder if LogEntry should instead be converted into a value object - only holding the fields with getters/setters and containing no logic, and database operations, and all the stuff that uses the dependency should be done via a separate service, or even a group of services.

I second this. ManualLogEntry should be a real value object, and shouldn't know how to insert or publish itself. Additonally, I think that the whole hierarchy might be refactored, including DatabaseLogEntry and RCDatabaseLogEntry (who also are smart value objects). Finally, since we're here, I've always found the name "ManualLogEntry" to be funny. I don't have a better proposal rn, but perhaps a rename might be considered. And finally, the class should be namespaced as well.

Long story short, I think the getter-setter logic should be moved to a new namespaced-and-possibly-better-named class, with a related Store class, and ManualLogEntry should become deprecated in favour of the new classes. The ancestors of ManualLogEntry (LogEntry and LogEntryBase) can also probably be moved to the new namespace, but I think User and Title should be widened to UserIdentity and LinkTarget while doing so (and the old classes would stay, but become deprecated). Then see if it makes sense whether DatabaseLogEntry and RCDatabaseLogEntry should inherit from the new LogBase. If yes, then we'd probably need a Lookup, together with a Store (can be two different interfaces implemented by the same class like for Revision). If not, it's out of the scope of this task.

Also pointing it out that everything that I said above needs not happen in a massive single patch. Things can happen in incremental patches whenever it makes sense to do so. I just wouldn't waste time creating a factory for ManualLogEntry objects. I think I can also help with code review and perhaps writing some code.

Majavah added a subscriber: Majavah.Jan 9 2021, 2:59 PM
DannyS712 removed DannyS712 as the assignee of this task.Jan 9 2021, 11:48 PM

Change 601340 abandoned by DannyS712:
[mediawiki/core@master] Add a new LogEntryFactory for constructing ManualLogEntry objects

Reason:
per task, going a different direction

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

Change 655270 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] [POC] Introduce MutableLogEntry and LogEntryStore

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