Modernize token handling (fixes #99)

[Xymph]

The action token handling is deprecated since MW v1.24. This PR restructures all three Wikimate classes to use the current meta tokens approach. That includes the login token, which was only introduced in MW v1.27. Wikimate deployments prior to that version can continue to use the v0.12.0 tag.

[Xymph]

Re. cc9bc21, these Logger lines were introduced way back in v0.4 but were never active as far as I could find, and it's not even clear which Logger library they relate too. Hence this little house-keeping.

[Xymph]

Re. 0a00740, this large(r) commit updates all three classes at once in order not to leave Wikimate in an inconsistent state between commits.

[Xymph]

Re. 5044fd7, this method, in Wikimate tradition, is kept simple and supports only the two token types needed elsewhere in the library. So no patrol, rollback, watch, etc. Nor the option to request more than one token at once ("watch|patrol"), we can cross that bridge if and when we get to it.

[Xymph]

This adds error checking to the preceding query(), analog to such invocations elsewhere.

[waldyrious]

Any reason why this comment was not added to the code itself?

[Xymph]

My remark should have been a (multiline) comment for new code lines 874 through 880. All other places where query() was called were followed by the code now also in the new lines. Not sure why it was previously missing, this was old code, but I don't think it's meaningful to spend more in-code comments on it.

[waldyrious]

Got it, thanks for the context.

[Xymph]

@waldyrious As before (well, in 2016-2017 :) ) a second pair of eyes on these commits would be appreciated.

[waldyrious]

Wikimate deployments prior to that version can continue to use the v0.12.0 tag.

Don't you mean "MediaWiki deployments..."?

[waldyrious]

Awesome job in separating the commits! I can't review the code to any meaningful depth, since I haven't used PHP nor the MW API in quite a while. The superficial aspects (commit messages and separation, code formatting, wording in comments, etc.) all look good to me. I'd only repeat one of the comments I've left inline, regarding this PR-level comment:

Re. 5044fd7, this method, in Wikimate tradition, is kept simple and supports only the two token types needed elsewhere in the library. So no patrol, rollback, watch, etc. Nor the option to request more than one token at once ("watch|patrol"), we can cross that bridge if and when we get to it.

Here too, wouldn't it make sense to add the comment to the code itself? It would be cumbersome for someone reading the code in the future to have to locate a pull request discussion thread to obtain context for why that method is implemented that way.

[Xymph]

Wikimate deployments prior to that version can continue to use the v0.12.0 tag.

Don't you mean "MediaWiki deployments..."?

Sorry that wasn't clear, I meant: Wikimate usage against MW deployments prior...

[Xymph]

Here too, wouldn't it make sense to add the comment to the code itself? It would be cumbersome for someone reading the code in the future to have to locate a pull request discussion thread to obtain context for why that method is implemented that way.

Good point, how about these two sentences?

   * For now this method, in Wikimate tradition, is kept simple and supports
   * only the two token types needed elsewhere in the library.  It also
   * doesn't support the option to request multiple tokens at once.

Thanks for the other remarks, the feedback is welcome.

If this is okay and there are no other items, then I'll commit that and wrap it up with a merge-commit (to preserve individual commit messages).

[Xymph]

Unless you are very busy, @waldyrious, perhaps you missed my follow-ups because I didn't include a ping, like now.

[Xymph]

I guess it's good enough, so wrapping up now.

[waldyrious]

Hey @Xymph — I was actually out for a few days, sorry I wasn't able to respond in time. In any case, yeah, I didn't have any additional feedback and the changes you've made look good to me^*, so here's my late LGTM! 😛

_{* I personally would have tended to include a bit more of the context you had in the PR comment ("So no patrol, rollback, watch, etc." and the example for multiple tokens, i.e. ("watch|patrol")) into the code comment, but as you say, we need to strike a balance between completeness and verbosity.}

[Xymph]

That's okay, I saw some activity in your heatmap early this week so figured you were busy elsewhere, but thanks for the LGTMs here and on #101. :)

For the token() method, I can add the link to the API page so readers can see examples there of what isn't supported?

[waldyrious]

I saw some activity in your heatmap early this week

That's my plight with GH notifications 😅 Sometimes I am able to come by just to report a quick issue or respond to a comment that requires no further action, but I always fear that other people who are waiting on me (for things that do require a more attentive look) will think I'm ignoring them.

Furthermore, I do get mobile notifications when I'm mentioned, but if I do open them to respond, the thread leaves my notifications inbox. So it's not even easy to drop a quick "I'm aware of this but can't take care of it right away" message without messing with my ability to keep track of the threads I need to come back to later...

For the token() method, I can add the link to the API page so readers can see examples there of what isn't supported?

That's a good idea, yeah :)

[Xymph]

For the token() method, I can add the link to the API page so readers can see examples there of what isn't supported?

That's a good idea, yeah :)

Ok, done in fafa2d8

[reedy]

Thanks! I was coming here to ask about doing this for https://phabricator.wikimedia.org/T280806, but now there's a release... Just need to wait for the various bots to update :)

[Xymph]

Thanks! I was coming here to ask about doing this for https://phabricator.wikimedia.org/T280806, but now there's a release... Just need to wait for the various bots to update :)

Our pleasure. The next release will address another Phabricator ticket per issue #92 / PR #112. Btw, we're looking for additional reviewing eyes there, in case that's something you could spend a little time on (but no problem if you can't).

[reedy]

Just need to get people to stop using versions 0.11 and 0.12 now ;)