Page MenuHomePhabricator

Add expiry support to remaining APIs that watch pages
Closed, ResolvedPublic

Description

There are a few remaining places where expiry support should or could be added:

  • ApiEditPage — This is what would be used when editing within VisualEditor, for instance. The code seems to trace through the EditPage class, and the watching happens with WatchAction::doWatchOrUnwatch(), which is probably being updated as part of T245565: Watchlist Expiry: Support for Static Watch Page (action=watch) [medium].
  • ApiBlock — The use-case is you can watch a user's talk page after blocking, say to monitor for unblock requests. I do see value in supporting expiries. The code seems to trace to the SpecialBlock class, where it calls WatchAction::doWatch() which already accepts an expiry. So having the API accept an expiry is probably not hard.
  • ApiProtect — There is an option to watch the page at the same time as protecting it. I think adding expiry support would be useful for admins, and fortunately it doesn't seem very hard; just add the new parameter, update ApiBase::setWatch() to accept it, and pass it to WatchAction::doWatchOrUnwatch().
  • ApiDelete — Same as with protect; we just need to adjust the API to accept the expiry and pass it to ApiBase::setWatch().
  • ApiRollback — Ditto, should be straightforward.
  • ApiMove — Ditto.
  • ...?

Important notes:

  • The block and protect APIs already have an expiry parameter (for the block expiry and protection expiry, respectively). For this reason I suggest all APIs be consistent and use the same name for the watchlist expiry parameter, something like watchlistexpiry. I think it's OK that the watch API (which we've already done) uses just expiry, though I suppose you could also support watchlistexpiry as an alias.
  • In the preferences, there are options to automatically watch pages for each of the above actions. Adding support for expiries to those can be done separately. The above code changes would only apply when the watchlist parameter is given a value of "watch" (user explicitly says they want to watch the page), as opposed to "preferences" (the default, goes by your preferences).
    • Further to this point, there are separate watch preferences for creating pages and editing existing pages, hence they may ultimately have separate preferences for the expiry. I don't think this will present any challenges since this happens transparently (when editing, if it's a new page it'll refer to that preference, rather than the "edit" preference). For the scope of this task, we're only concerned about when the user explicitly says they want to watch the page instead of falling back to preferences.
  • With all these changes, it really invites the idea of introducing a standardized parameter type for expiries to the action API. That is something we can work on later (or not at all). Related work is tracked at T248196: Consolidate logic for parsing expiries. For now just use a PARAM_TYPE of "string", and use WatchedItem::normalizeExpiry() to parse the expiry input, just like we did at https://gerrit.wikimedia.org/r/580522.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
MusikAnimal renamed this task from Add expiry support to remaining APIs that watch/unwatch pages to Add expiry support to remaining APIs that watch pages.Mar 24 2020, 6:37 PM
Anomie subscribed.

Sounds sensible to me.

You'll also want to add returning of the expiry to ApiQueryWatchlistRaw, and probably also ApiQueryWatchlist. Maybe also ApiQueryInfo when used with inprop=watched, but I wouldn't object to deciding to leave that one until someone actually asks for it.

  • The block and protect APIs already have an expiry parameter (for the block expiry and protection expiry, respectively). For this reason I suggest all APIs be consistent and use the same name for the watchlist expiry parameter, something like watchlistexpiry. I think it's OK that the watch API (which we've already done) uses just expiry, though I suppose you could also support watchlistexpiry as an alias.

I wouldn't bother with an alias on action=watch. It seems sensible to me for action=watch that directly manipulates the watchlist to have expiry where all modules that have watching as a side effect use watchlistexpiry.

  • With all these changes, it really invites the idea of introducing a standardized parameter type for expiries to the action API. That is something we can work on later (or not at all). Related work is tracked at T248196: Consolidate logic for parsing expiries. For now just use a PARAM_TYPE of "string", and use WatchedItem::normalizeExpiry() to parse the expiry input, just like we did at https://gerrit.wikimedia.org/r/580522.

I'd encourage you to at least add the expiry type to ParamValidator, even if you don't do the consolidation of the expiry parsing logic, to avoid adding duplication of the logic in ApiWatch::normalizeAndValidateExpiry(). I'll be happy to give you assistance in doing that.

@MusikAnimal Hey, there! I'm checking in to see why this task was changed from 'open' to 'stalled.' Thanks!

Hey, there! I'm checking in to see why this task was changed from 'open' to 'stalled.' Thanks!

Not stalled! Maybe you mean T248514? I left a comment at T248514#6059752. Just a more visual indicator that that task can't be worked on right now.

MusikAnimal claimed this task.

All that is left (that's being tracked) is T263336: Add watchlist expiry support to FlaggedRevs' stabilize API which should be somewhat straightforward for any interested developer. As it is not part of Core, and a largely unmaintained extension, we are not considering part of the Expiring-Watchlist-Items project. There are surely other extensions with APIs that lack watchlist expiry support, too.