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.

In T253717#6172864, @Pchelolo wrote:

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.

In T253717#6174972, @Pchelolo wrote:

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

In T253717#6174972, @Pchelolo wrote:

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.

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