Page MenuHomePhabricator

Cleanup execution flow through Special:Search
Closed, ResolvedPublic


This will probably have to be done in multiple patches. Also this might be easier once T150390 has extracted the result rendering from the class. The high level idea:

  • SpecialSearch::goResult should only handle 'go', and not forward on to SpecialSearch::showResults. Make things easier to follow by only having a single patch from execute -> showResults. Maybe goResult should just return a boolean true if go was sucessfull, and false on failure.
  • SpecialSearch::showResults is currently ~225 lines, which can make it hard to keep in your head all the state the is around. Try and split a few functions out, for example the code that handles wgDisableTextSearch can probably move into its own function, and be called at the top level from SpecialSearch::execute instead.
  • Could powersearch be extracted into some sort of helper class that just has methods to apply powersearch options to the search engine, and render the powersearch box? Could this be generalized into a way to map a profile to what it does? Also could we rename powersearch to 'advanced' to match the profile= parameter and how it's shown in the interface at the same time?
  • Might be worth checking usage data, but we could also strip the 'startsWithImage' feature, which allows to search the file namespace by typing file:foo
  • Can various stateless helpers, such as makeSearchLink, shortDialog, be extracted? might not be worthwhile.
  • Maybe more, these are some initial things I noticed paging through.


Related Gerrit Patches:

Event Timeline

EBernhardson updated the task description. (Show Details)Nov 10 2016, 4:37 AM

Change 322013 had a related patch set uploaded (by EBernhardson):
Cleanup execution flow through SpecialSearch::execute()

Deskana moved this task from Uncategorised to Technical on the CirrusSearch board.Dec 8 2016, 6:14 PM

@lindseyanne @dr0ptp4kt Is there anyone in Editing or Reading that's suitably versed in core that could review this? Thanks. :-)

I believe a number of Reading Web engineers have interfaced with search-related things at one point or another. @ovasileva, would it be possible to discuss this with the team for consideration in the forthcoming sprint? @Deskana, with the freeze coming, is the urgency for review-and-merge something like sometime before the next official production train week?

I've copied and pasted this verbiage from another task.

@Deskana, with the freeze coming, is the urgency for review-and-merge something like sometime before the next official production train week?

No, not at all. There's no particular need to get it done before the holiday deployment freeze. Some time early next quarter would be fine. :-)

That works, tagging it with the reading-web backlog for review and will discuss with team about lining it early next quarter

Change 322013 merged by jenkins-bot:
Cleanup execution flow through SpecialSearch::execute()

Deskana closed this task as Resolved.Jan 30 2017, 6:22 PM