Last modified: 2009-11-01 14:19:28 UTC
Much as I hate it, the API should support the system of "tags" for entries in Recent Changes. To get the same functionality as the UI, this means: * adding a "tag" field anywhere revisions are output (e.g. recent changes, user contributions, page history) * some way to filter list=recentchanges by tag
Bumping priority on this to High since it would also serve as a partial workaround for the Abuse Filter's complete workflow breakage that is bug 18374, which is sorely needed for recent changes patrolling.
Forgot one other feature that would be needed: some sort of query to enumerate the possible tags, as [[Special:Tags]] does now.
Created attachment 6314 [details] patch against r53005 * adds list=tags * adds 'tags' as an option for rcprop, rvprop, ucprop and leprop Haven't done filtering by tag.
ApiQueryTags seems to duplicate a lot of code from SpecialTags. I think that one common backend should be written, from which both the API and the special page fetch their data, rather than duplicating code in them both. Furthermore, this could use some consistency in coding style.
(In reply to comment #3) > Created an attachment (id=6314) [details] > patch against r53005 > > * adds list=tags > * adds 'tags' as an option for rcprop, rvprop, ucprop and leprop > > Haven't done filtering by tag. > Patch looks good overall, some more comments in addition to Bryan's: * You're querying the change_tag table using GROUP BY and COUNT(*). This is probably inefficient, so I recommend using valid_tags instead if at all possible * I don't see enough context in the patch, but you should test the LEFT JOINs you've added to make sure they don't try to join against the wrong table * The copyright notice in ApiQueryTags should include the author's name * Instead of 'continue' => array(), use 'continue' => null, same for 'end'. Also, 'end' is undocumented
(In reply to comment #4) > ApiQueryTags seems to duplicate a lot of code from SpecialTags. I think that > one common backend should be written, from which both the API and the special > page fetch their data, rather than duplicating code in them both. Agreed, I was trying to touch as little stuff as possible. Would that go in includes/ChangeTags.php? > * You're querying the change_tag table using GROUP BY and COUNT(*). This is > probably inefficient, so I recommend using valid_tags instead if at all > possible Here I did things the same way the special page does them. Not doing the group/count stuff loses the hit counts, which to be honest aren't all that important, and could be done away with. Is the output of Special:Tags cached? If so I guess that would explain the potentially expensive query. > * I don't see enough context in the patch, but you should test the LEFT JOINs > you've added to make sure they don't try to join against the wrong table Everything works when I test it, this is only on a tiny wiki though. > Also, 'end' is undocumented That would be because I forgot to remove it :)
(In reply to comment #6) > (In reply to comment #4) > > ApiQueryTags seems to duplicate a lot of code from SpecialTags. I think that > > one common backend should be written, from which both the API and the special > > page fetch their data, rather than duplicating code in them both. > > Agreed, I was trying to touch as little stuff as possible. Would that go in > includes/ChangeTags.php? > Yes, I think that would be the correct place. > > * You're querying the change_tag table using GROUP BY and COUNT(*). This is > > probably inefficient, so I recommend using valid_tags instead if at all > > possible > > Here I did things the same way the special page does them. Not doing the > group/count stuff loses the hit counts, which to be honest aren't all that > important, and could be done away with. Is the output of Special:Tags cached? > If so I guess that would explain the potentially expensive query. > If it's in the special page then it should probably be ok.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > ApiQueryTags seems to duplicate a lot of code from SpecialTags. I think that > > > one common backend should be written, from which both the API and the special > > > page fetch their data, rather than duplicating code in them both. > > > > Agreed, I was trying to touch as little stuff as possible. Would that go in > > includes/ChangeTags.php? > > > Yes, I think that would be the correct place. Yes, refactoring core code to be more API-friendly is almost always a good thing. > > > * You're querying the change_tag table using GROUP BY and COUNT(*). This is > > > probably inefficient, so I recommend using valid_tags instead if at all > > > possible > > > > Here I did things the same way the special page does them. Not doing the > > group/count stuff loses the hit counts, which to be honest aren't all that > > important, and could be done away with. Is the output of Special:Tags cached? > > If so I guess that would explain the potentially expensive query. > > > If it's in the special page then it should probably be ok. > Yeah, if the UI does it, have no fear of doing it in the API as well.
Created attachment 6416 [details] patch against r54266 * adds getHitCounts function to ChangeTags, caches result in same way as listDefinedTags * modifies SpecialTags to use this function * adds list=tags * adds 'tags' as an option for rcprop, rvprop, ucprop and leprop * adds rctag, rvtag, uctag and letag for filtering by tag
Looks ok, indices appear to be in place. Comitted in r54291.
Reopening because filtering is wrong (revisions can have multiple tags, or at least could if anything gave them some). c.f. http://www.mediawiki.org/wiki/Special:Code/MediaWiki/54291#c3443
Created attachment 6473 [details] patch against r55171 Fixes for r54291: * tag filtering queries change_tag table instead of tag_summary, so works correctly when one item has multiple tags * (rc|rv|le|uc)prop=tags gives multiple tags in structured format rather than comma-delimited string * removed unused 'end' parameter Breaking change w.r.t r54291.
Orig version reverted for now in r55332 to keep trunk clean.
Created attachment 6514 [details] patch against r55711 combine last two patches, since first was reverted. should work against trunk.
Some useful functionality for client apps is waiting on this, and my patch has been sitting here for 2 months, can someone take a look at it?
(In reply to comment #15) > Some useful functionality for client apps is waiting on this, and my patch has > been sitting here for 2 months, can someone take a look at it? > Looking good to me. Patch committed in r58399.
(In reply to comment #16) > Looking good to me. Patch committed in r58399. Thanks muchly. :)
