ManualLogEntries::publish do not store tags when published to `udp`

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

ManualLogEntries::publish do not store tags when published to `udp`

Piotr Miazga
Hi All,

I noticed that ManualLogEntry items could have tags only when those entries
are published to `rc` or `rcandudp`. Then the extensions can attach tags
via RecentChange_save hook and everything works perfectly. The problem I
encountered happens when the log entry is published to `udp` only[1], then
tags are ignored. The only way to apply tags to the LogEntry is to call
`ChangeTags::addTags()` after log entry creation.
In the AMC[2] project we would like to tag user actions with `advanced
mobile edit`[3].  Some of those actions, like "Thanks" are not published as
RecentChange, and we want to keep it as it is right now[4].

What is the reason for this?
Can I safely add support for tagging log entries that are published to
udp only inside the ManualLogEntry class?

Otherwise, I'll need to update Thanks extension code and
 - create a new hook that passes the LogEntry object, for
example, "ThanksLogEntryCreation"
 - in the MobileFrontend listen to that hook and call the
`ChangeTags::addTags( [ 'advanced mobile edit' ], null, null, $logId );`
Doesn't sound that bad, but then I'll have to apply the same code to other
places where we create log entries that are not published as RecentChange.
It sounds like a technical debt to me. Also, the current implementation
feels broken, as code quietly ignores tags when logs are published to `udp`
only.

It looks like the ChangeTags already supports adding tags to LogEntries
only - both rc_id and rev_id are nullable, and the only question is how to
tag logs published to the RecentChange (do we add tags to both LogEntry and
RecentChange objects?).
Additionally, I'd like to introduce a Taggable interface[6], that provides
a one way to tag objects (right now RecentChange exposes addTags() method
but the ManualLogEntry exposes setTags() method).


[1]
https://github.com/wikimedia/mediawiki/blob/278ac40e9609b0b226a85e020f5e574054e1d78f/includes/logging/LogEntry.php#L784
[2] https://www.mediawiki.org/wiki/Reading/Web/Advanced_mobile_contributions
[3] https://phabricator.wikimedia.org/T212959
[4] https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Thanks/+/493740/
[5]
https://github.com/wikimedia/mediawiki-extensions-Thanks/blob/ac1c3906efabd0f71a314f5261fb6f1ec11a49f0/includes/ApiThank.php#L79
[6] https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/493464/


--
*Piotr Miazga* (he/him)
Fullstack Engineer
Wikimedia Foundation <https://wikimediafoundation.org/>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: ManualLogEntries::publish do not store tags when published to `udp`

Brad Jorsch (Anomie)
On Mon, Mar 11, 2019 at 9:48 AM Piotr Miazga <[hidden email]> wrote:

> I noticed that ManualLogEntry items could have tags only when those
> entries are published to `rc` or `rcandudp`.


Hmm. Yes, it looks like the tags aren't being added in the `udp` case.
Looks like it was broken in
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/312743. Filed T218110
<https://phabricator.wikimedia.org/T218110> for that.


> Then the extensions can attach tags via RecentChange_save hook and
> everything works perfectly.


I note this is not really related to the fact that tags set on the
ManualLogEntry aren't stored when entries are published to `udp`, as that
hook doesn't directly deal with log entries at all.

To support tagging published log entries directly rather than via the
associated RecentChange entry, you'd probably want to add a
"ManualLogEntryPublish" hook or something like that.


> Additionally, I'd like to introduce a Taggable interface[6], that provides
> a one way to tag objects (right now RecentChange exposes addTags() method
> but the ManualLogEntry exposes setTags() method).
>

"Taggable" seems like it may be too generic.
"MediaWiki\ChangeTags\Taggable" could work.

--
Brad Jorsch (Anomie)
Senior Software Engineer
Wikimedia Foundation
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l