Page MenuHomePhabricator

Disallow cloning of RequestContext objects
Closed, ResolvedPublic

Description

This came up in T323153, briefly discussed with @aaron and @tstarling during a collab hour.

As analyzed by @Umherirrender we have no bundled/deployed code that does this currently, and even across all Codesearch-indexed repos there was only 1 instance of a third party repo doing. It seems clearly a mistake and the alternative is easy once known, namely to use a DerivativeContext instead. Given the ease of alternatives, the only risk is re-introduction through not knowing among newer contributors and code reviewers missing it, and more specifically that we rely solely on code review to catch it when automatic detection is so easy.

Given that cost-benefit, it seems worthwhile to simply plug this hole and then it's naturally avoided/discovered.

Event Timeline

Krinkle added a project: good first task.

Flagging as easy task and putting on our radar to make sure I see it activate to offer guidance/review.

Change 886391 had a related patch set uploaded (by Wandji collins; author: Wandji collins):

[mediawiki/core@master] Disallow cloning RequestContext object and through an error.

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

Change 886391 merged by jenkins-bot:

[mediawiki/core@master] context: Disallow cloning RequestContext object and throw an error

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

As analyzed by @Umherirrender we have no bundled/deployed code that does this currently, and even across all Codesearch-indexed repos there was only 1 instance of a third party repo doing.

Apparently this analysis (T323153#8398142) was too restrictive; a codesearch with a more lenient regex readily finds the code in SpecialUndelete.php that caused T346995 (though hindsight makes it easier to write the right regex, of course). Alternatively, hard deprecating rather than immediately throwing a LogicException would also have avoided breaking production over this.