Page MenuHomePhabricator

Normalize WatchedItem expiry field
Closed, ResolvedPublic5 Estimated Story PointsOct 7 2020

Description

As it stands, WatchedItem class would accept and store any string on the expiry field. As a result ::getExpiry() is unpredictable and for a client to reliably use it they need to normalize it. A few bugs have surfaced because of this. E.g. T253937
Also, I'm not sure but this work might solve T260009(?)

Notes from Eng-meeting

  • Use ExpiryDef::normalizeExpiry() in WatchedItem constructor, throw exception if invalid
  • Store expiry as MWTimestamp or CovertibleTimestamp to ensure we compare dates in UTC (WatchedItem::isExpired())
  • getExpiry() to return formatted as TS_MW

Acceptance criteria

  • Expiry should be normalized and store as either MWTimestamp or CovertibleTimestamp (whatever makes more sense)
  • Creating a WatchedItem with an unsupported string should fail
  • getExpiry() to return formatted as TS_MW or whatever makes more sense
  • Confirm that the $wgLocaltimezone > UTC bug detailed in T260009 is resolved with the changes implemented in this ticket

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptAug 20 2020, 3:51 AM

I'm unsure about the time format; TS_MW != TS_ISO_8601 which is what ExpiryDef uses; I think they should match or formatting before comparing will still be needed. An option would be to add a format param to getExpiry()(?). What do you think we should do?

ARamirez_WMF set the point value for this task to 5.Aug 20 2020, 5:21 PM

Change 623052 had a related patch set uploaded (by HMonroy; owner: HMonroy):
[mediawiki/core@master] Normalize WatchedItem expiry field

https://gerrit.wikimedia.org/r/623052

ARamirez_WMF changed the subtype of this task from "Task" to "Deadline".

Change 623052 merged by jenkins-bot:
[mediawiki/core@master] Normalize WatchedItem expiry field

https://gerrit.wikimedia.org/r/623052

Change 632675 had a related patch set uploaded (by MusikAnimal; owner: HMonroy):
[mediawiki/core@REL1_35] Normalize WatchedItem expiry field

https://gerrit.wikimedia.org/r/632675

Change 632675 merged by jenkins-bot:
[mediawiki/core@REL1_35] Normalize WatchedItem expiry field

https://gerrit.wikimedia.org/r/632675

dom_walden added a subscriber: dom_walden.
  • Expiry should be normalized and store as either MWTimestamp or CovertibleTimestamp (whatever makes more sense)

Formats like "20201016142841" get interpreted as $wgLocaltimezone and converted to UTC.

Formats like "1 week" are also interpreted in $wgLocaltimezone and converted to UTC (I think, hard to tell).

It has not changed the format the expiries are stored in the database, so should not affect things like Special:Watchlist, Special:EditWatchlist and the watch star, which query the DB directly.

  • Creating a WatchedItem with an unsupported string should fail

Validation is much the same as before.

One of the only differences is that submitting an invalid expiry via action=watch or while editing time will raise an exception. Previously, it just created a permanent watch without an exception.

This would only happen if an invalid expiry option had been entered into MediaWiki:Watchlist-expiry-options or the respective translation (e.g. en.json, fr.json). I think only trusted users can edit these, although mistakes can happen like T258583.

  • getExpiry() to return formatted as TS_MW or whatever makes more sense

The caller of the function can specify which format they would like it to be in.

When updating a watched item, it is important that the new and old expiry values are in the same format when they are compared.

I tested updating a watched item's expiry via action=watch and while editing. It updated when the expiry was different, and did not update when the expiry did not change.

  • Confirm that the $wgLocaltimezone > UTC bug detailed in T260009 is resolved with the changes implemented in this ticket

This was fixed before this change. I made sure that it is still fixed after this change.

Also tested that a similar bug does not occur when editing a page and leaving the expiry time unchanged. (Such a bug did not occur before this change either.)

ifried added a subscriber: ifried.

The one difference (submitting an invalid expiry via action=watch or while editing) is acceptable behavior. This work is now complete, so I'm marking it as Done.

The one difference (submitting an invalid expiry via action=watch or while editing) is acceptable behavior. This work is now complete, so I'm marking it as Done.

@ifried what do you mean by submitting an invalid expiry via action=watch or while editing?

@dmaza I'm responding to the above comment from Dom: "One of the only differences is that submitting an invalid expiry via action=watch or while editing time will raise an exception. Previously, it just created a permanent watch without an exception."