Page MenuHomePhabricator

[Epic] De-Spaghettify SpecialSearch class
Closed, ResolvedPublic

Description

This code has been shuffled around and adjusted, but structurally isn't much different than it was in 2004 when search rendering was pulled out of the SearchEngine class and moved into the SpecialSearch class. Since then various things have changed, but the class itself has never been refactored to make sense of what it actually does today. As such it is, quite frankly, a complete mess of things bolted ontop of each other.

Someone needs to go through this and try and figure out what the main ideas are, and refactor into something that has a clear and defined execution flow. Bonus points if somehow most of the logic can be separated from the rendering.

Event Timeline

Restricted Application added a project: Discovery-Search. · View Herald TranscriptNov 7 2016, 10:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
EBernhardson renamed this task from De-Spaghettifi SpecialSearch class to De-Spaghettify SpecialSearch class.Nov 7 2016, 10:13 PM

The current state of Special:Search is probably going to make our planned UI adjustments that much harder. Probably wouldn't be a bad idea to see what can be done to make things a bit nicer.

EBernhardson updated the task description. (Show Details)Nov 7 2016, 10:55 PM

For extra fun, it looks like there are 12 hooks. 11 of them appear to be used in one form or another, mostly by Wikibase, WikimediaIncubator, Translate, LiquidThreads, or CirrusSearch. Any refactoring will need to ensure it still supports the current hooks, or provides an upgrade path (and a patch for the extension) for the things extensions are doing with the hooks

Deskana added a subscriber: Deskana.Nov 8 2016, 6:09 PM

We discussed this today. This is currently an absolutely gigantic task. @EBernhardson said he'd look at this and break this down into smaller tasks, to make the task more actionable.

We discussed whether it's easier to just make a new SpecialSearch page for CirrusSearch which overrides the one in core. It sounds easier, but it'd break a ton of backwards compatibility. We probably don't want to do that. But that approach may be fine for running quick A/B tests, provided we integrate them into SpecialSearch afterwards.

Deskana renamed this task from De-Spaghettify SpecialSearch class to [Epic] De-Spaghettify SpecialSearch class.Nov 8 2016, 6:14 PM
Deskana triaged this task as High priority.

Breaking up this is important, since it will probably affect our ability to change the UI for the new cross-wiki search.

For extra fun, it looks like there are 12 hooks. 11 of them appear to be used in one form or another, mostly by Wikibase, WikimediaIncubator, Translate, LiquidThreads, or CirrusSearch.

I'd appreciated more thoroughness in your research with non-WMF extensions relying on those hooks as well!

EBernhardson added a comment.EditedNov 10 2016, 4:34 AM

For extra fun, it looks like there are 12 hooks. 11 of them appear to be used in one form or another, mostly by Wikibase, WikimediaIncubator, Translate, LiquidThreads, or CirrusSearch.

I'd appreciated more thoroughness in your research with non-WMF extensions relying on those hooks as well!

I checked the 166 extensions hosted by gerrit, finding external users is not a particularly easy task

mwjames added a comment.EditedNov 10 2016, 4:35 AM

I checked the 161 extensions hosted by gerrit, finding external users is not a particularly easy task

Deskana moved this task from Uncategorised to Technical on the CirrusSearch board.Dec 8 2016, 6:14 PM
Nemo_bis added a subscriber: Nemo_bis.
debt closed this task as Resolved.Jan 26 2017, 11:08 PM
debt claimed this task.
debt added a subscriber: debt.

This looks to be done - yay - closing!

Deskana reassigned this task from debt to EBernhardson.Jan 26 2017, 11:15 PM
Deskana moved this task from Up Next to Current work on the Discovery-Search board.
Deskana moved this task from in progress to Done on the Discovery-Search (Current work) board.