Problem/Motivation
I was looking at the code in Display Suite today (they already have an 8.x alpha, good job!). They have extended the NodeSearch plugin so that they can use their own display method for the search results.
Great idea, but it turns out the way they had to do it was to duplicate the whole execute() method, just to change the loop where it goes through and formats the results.
This is undesirable.
Proposed resolution
We could easily make this easier for them by separating out the NodeSearch::execute() method into two parts:
a) The part up to $query...->execute() where it finds the results
b) The part after that where it does
foreach ($find as $item)
and processes the results into an array to return, by rendering etc.
This could be done by either making a protected method called something like runTheSearchQuery() and calling that in execute(), or conversely by making a protected method called something like buildTheSearchResultsArrayToReturn() and calling that in execute(). In the first case, DS would call the new run method in their execute() and then proceed to format in a different way, and in the second case, DS would override the build method to format the results differently.
We could also do both -- break up the execute() into those two methods, making it easy for a contrib module to override either piece.
Note: I am not advocating those exact names for the methods. :) Nor do I have a strong opinion on which way is better. I'm going to ask the DS maintainers to comment here. But this would be an easy change that would help DS (and maybe other contrib modules), and also break a long function up into two more understandable pieces.
And it would have no backwards-incompatible API change, so no real downside.
Remaining tasks
Patch in #5 does substantially what is described here: NodeSearch::execute() is separated out into NodeSearch::findResults() and NodeSearch::prepareResults(). Presumably, a plugin like Display Suite's can now override just the prepareResults() step, and leave findResults() alone. Alternatively, a hypothetical plugin that has a different method for searching, but wants the default display of results, could just override the findResults() method.
Beta phase evaluation
Issue category | Task because it is just a small code refactor. |
---|---|
Issue priority | Not critical or major. |
Disruption | This change is not disruptive. No contrib/custom modules will need to change. But it should reduce fragility -- it allows Display Suite and potentially other search-related modules that want to extend the NodeSearch plugin to have less code, because they can either override the "find the results" part or the "format the results" part, without having to copy/paste a lot of code from NodeSearch to handle the other part that they are not overriding. |
User interface changes
None.
API changes
Not really. It's all internal to NodeSearch::execute(), which is calling the two new protected methods.
Comment | File | Size | Author |
---|---|---|---|
#25 | 2212335-separate-search-execute-25.patch | 2.9 KB | jhodgdon |
#25 | interdiff.txt | 2.23 KB | jhodgdon |
Comments
Comment #1
jhodgdonNote that this should not be completed until #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords is done.
Comment #2
jhodgdonHere's a simple patch, which I haven't tested but it should work, I hope.
Comment #3
jhodgdonI filed a related issue in the Display Suite module asking them to please review this patch and see if it serves their purposes.
Comment #5
jhodgdonWhoops, perhaps I should have tested it. :) Missed a local variable $keys needed in the 2nd method, so added line
to prepareResults(). No other changes. Didnt' make an interdiff file for it. Let's see if this one works.
Comment #6
jhodgdonI guess I never assigned this to myself. It still needs a review.
Comment #9
penyaskitoLooks like a cool idea.
Should we strictly compare $found to null?
Comment #10
jhodgdonI don't know why that would be necessary. The context is:
Any value that evaluates to FALSE would still mean "no results, so return an empty array".
Comment #12
penyaskitoI don't have any other remarks on this. If core maintainers are happy with #9 and #10, this is RTBC for me.
If I understood #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? correctly, this is acceptable in the beta period and even could be raised as critical as contrib needs ugly workarounds.
Comment #13
jhodgdonThanks! Yes, I think this can be committed. I'll update the issue summary just so it is clear what exactly this patch does. And unassigning myself to avoid "jhodgdon will commit this" confusion.
Comment #14
swentel CreditAttribution: swentel commentedNote: +1 on this commit - I've been working on DS search during DrupalCon and this patch makes our code ten times more cleaner :)
Comment #15
alexpottHow about changing the patch like this (a diff on top of the current patch)?
This way we have less conditions in the protected methods and stronger type hinting - making it easier to override and do the right thing.
Comment #16
jhodgdonI kind of like this idea, but Select::execute() method can return NULL (at least, it's documented to return NULL sometimes), so I don't think we'll get as much simplification as you think we will.
Comment #18
jhodgdonOK, trying to move this forward again.
So since Select::execute() can return NULL, I think this is a better way to do something along the lines of #15. Thoughts? Reviews please? It seems pretty simplified to me...
Comment #20
jhodgdonWhoops. findResults vs. findResults().
Comment #21
swentel CreditAttribution: swentel commentedI'm fine with this.
Comment #22
alexpottre #16 I don't understand what you mean. The point was to not random arrays of stuff between methods but objects with interfaces. The docs on the method say it all:
Results found from the search query, or NULL if query failed.
Well why call the method then? The point was not for simplification or performance it was for
With the patch in #20 all implementations have to begin with
Why?
Comment #23
alexpottComment #24
jhodgdonRE #22, the Select class itself is documented to sometimes return NULL from its own execute() method. I don't see how to get around checking for it being NULL, assuming that this documentation is correct?
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Query!Se...
So I believe I have to check for NULL somewhere... Either it needs to be checked in the findResults() method or the prepareResults() method, and in either case, any plugin extending NodeSearch that overrides the method in question will have to check this.
I personally think it's better to have this at the very top of a method that might be overridden so it's obvious, not buried at the bottom of the other one. Thoughts?
Comment #25
jhodgdonSigh. Doh. Here's one more go at this, sorry for misunderstanding again, hope I have it right this time!
Comment #27
jhodgdon@swentel, any chance of another review? Thanks!
Comment #28
pwolanin CreditAttribution: pwolanin commentedLooks good
Comment #29
jhodgdonAdding "beta phase" evaluation to summary.
Comment #30
alexpottCommitted bd02a26 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.