matthew.britton wrote:

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.

matthew.britton wrote:

Forgot one other feature that would be needed: some sort of query to enumerate the possible tags, as [[Special:Tags]] does now.

matthew.britton wrote:

patch against r53005

• adds list=tags
• adds 'tags' as an option for rcprop, rvprop, ucprop and leprop

Haven't done filtering by tag.

attachment api-tags.patch ignored as obsolete

Bryan.TongMinh wrote:

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

matthew.britton wrote:

(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 :)

Bryan.TongMinh wrote:

(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.

matthew.britton wrote:

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

attachment api-tags.patch ignored as obsolete

Bryan.TongMinh wrote:

Looks ok, indices appear to be in place. Comitted in r54291.

matthew.britton wrote:

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

matthew.britton wrote:

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.

attachment api-tags.patch ignored as obsolete

Orig version reverted for now in r55332 to keep trunk clean.

matthew.britton wrote:

patch against r55711

combine last two patches, since first was reverted. should work against trunk.

Attached:

matthew.britton wrote:

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.

matthew.britton wrote:

(In reply to comment #16)

Looking good to me. Patch committed in r58399.

Thanks muchly. :)