Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
A recent patch #505214: Make search module into a cleaner and more flexible framework added the ability to disable particular module searches (such as node). However, the search module still assumes that node module is the default and will redirect you there - and then the advanced search form will still show.
Comment | File | Size | Author |
---|---|---|---|
#59 | search-default-module.patch | 870 bytes | mfb |
#53 | 567100-cleanup-51.patch | 22.09 KB | pwolanin |
#51 | 567100-51.patch | 22.43 KB | jhodgdon |
#45 | 567100-45.patch | 22.51 KB | jhodgdon |
#42 | 567100-cleanup-42.patch | 21.74 KB | pwolanin |
Comments
Comment #1
ezra-g CreditAttribution: ezra-g commentedI'm taking a look at this during the code sprint.
Comment #2
ezra-g CreditAttribution: ezra-g commentedPart of the problem here is that search.module assumes that it should use node.module's search when the type of search is not specified, regardless of whether node.module is configured a as an active search module.
Instead, administrators should be able to specify which module from the active ones should be used as the default.
This patch adds a "Default search module option" to the search_admin_settings form and redirects to this module's search form when someone browses to /search or search/[junk], where junk is a module that isn't configured as active for search or doesn't exist.
Thanks, @pwolanin for discussing this at the code sprint.
Comment #4
sun.core CreditAttribution: sun.core commented+1, this is one of the obstacles I ran into in #375397: Make Node module optional
Comment #5
sun.core CreditAttribution: sun.core commentedNot critical. Please read and understand Priority levels of Issues, thanks.
Comment #6
jhodgdon#2: 567100.patch queued for re-testing.
Comment #7
jhodgdonThis looks like an excellent idea to get in, so I've requested a retest of the patch.
Comment #9
jhodgdonAnyone interested in updating the patch in #2 so it will apply? This looks like something we should fix for D7.
Comment #10
jhodgdonHere's a new patch that fixes the idea that node must always be the default search type. It still uses node if node is active, and otherwise it just defaults to the first one it finds in the list. It also puts up an error message if you try to search and there are no active modules.
Comment #11
jhodgdonJust a note that letting the UI decide which module is the default is a separate issue:
#248154: Allow UI to choose which search module is default tab
Comment #12
jhodgdonA patch I just added to #245103: Search page tabs not highlighting also fixes this issue (they were rather intertwined).
Comment #13
jhodgdonThe latest patch of that issue does not fix this issue. chx thought they should be left separate. But this one shouldn't be addressed until that other issue is patched.
Comment #14
jhodgdonMarking as needs work until the other patch is committed from #245103: Search page tabs not highlighting
Comment #15
jhodgdonChanging title to reflect the actual issue that needs to be fixed.
Comment #16
jhodgdonThe relevant patch from that other issue has landed (follow-up patch is not intertwined with this issue). So, it's time to revisit this.
Comment #17
jhodgdonHere's a new patch. Kind of similar to the old patch. With tests and everything.
Comment #18
jhodgdon#17: 567100-17.patch queued for re-testing.
Comment #19
pwolanin CreditAttribution: pwolanin commentedIs this code right? The path component in
$search_info['path']
which I think is passed into search_view() may be different than the module name. It just happens that for node and user they are the same.Comment #20
jhodgdonEeek. I think you may be right. Deserves a test, too (with a test module that defines a search with module 'foo', path 'bar', title 'baz').
Comment #21
jhodgdonGracious. Good catch, pwolanin -- the code in search_form() before the patch is also assuming that the path is the module name.
I'm definitely adding some tests for this!
Comment #22
jhodgdonHere's a brand new patch. It fixes the problem identified by pwolanin - that several places in the search module code, the module was assuming the search path was equal to the name of the module. That was left over from Drupal 6, which had that assumption, and there were a couple of places in the code where that hadn't been fixed. Plus my patch above introduced some more (or at least continued them).
I also added a bunch of tests that exercise this.
Comment #23
pwolanin CreditAttribution: pwolanin commentedA few comments:
Throughout this code it's still unclear to me where we are referencing module names, and where we are referencing the 'path' that comes in from the user.
For example, this code:
Should instead be like:
Also, I see a possible problem with this:
This should use a string which can never be a valid setting - e.g. it should include spaces or some other characters.
Comment #24
jhodgdonGood points. I think the new docblocks on these functions make clear what is a path and what is a module name, but I agree the argument names could make this even clearer. And you are right on the setting.
Comment #25
pwolanin CreditAttribution: pwolanin commentedI'm also a bit unfond of the pattern here where /search is the default search and we redirect to it, since that seem to suggests that as I enable/disable modules the actuall target of that form submission will change.
I think we want to be doing the opposite - the bare /search path will redirect to the default search path? That's the Drupal 6 behavior.
Comment #26
pwolanin CreditAttribution: pwolanin commentedI'm working on a new patch building on the above which handles this all a little differently.
Comment #27
jhodgdonGood luck with that. The reason we are redirecting to path /search/ is due to the strange way we're using hook_menu(). See
#245103: Search page tabs not highlighting
Comment #28
jhodgdonAlso, if you want to change that behavior, the other issue would be a better place than this one to do it.
Comment #29
jhodgdonpwolanin: I just want to save you a lot of time going down a fruitless path... see #245103 - the search tabs don't work without screwy hook_menu(), and then the redirect to path /search was necessary to not have two sets of tabs showing... read the comments on the issue or in the code to see why... I don't think you'll find another way to do it all that doesn't involve a redirect to /search for the default search path.
And again, if you do find a way, that other issue is the place to discuss it and/or change it, not here. I was asked on that issue not to bring this one into the patch, and let's do the same here.
Also, I think this is not as big of a problem or misdirection as you're thinking. Probably setting up which search modules are enabled should only happen once when the site is set up, so it shouldn't really be changing. So /search will go to whatever the default is... why is that a problem?
Comment #30
pwolanin CreditAttribution: pwolanin commented@jhodgdon - trying to get the tests to pass on the config form.
I'm not seeing the behavior you describe of 2 sets of tabs.
Comment #31
pwolanin CreditAttribution: pwolanin commentedHere's my new attempt. It adds radios to the search settings form to select the default - I think that's better than having a function whose return value seemingly can only be controlled by tweaking module weights.
Comment #32
pwolanin CreditAttribution: pwolanin commentedThinking about search_data() - I don't think there is any reason we need to check for non-empty params - seems like babysitting the code.
Also improved some code comments, etc.
Comment #33
jhodgdonRE #30 on two sets of tabs - you would need to go back to the code as it was in intermediate stages of #245103.
I'll test this patch and read it through more carefully a bit later this morning... I like the approach, but I'm skeptical about some of it working correctly at first glance (double tabs, etc.). But only trying it out will tell...
One thing I saw the first time through:
I don't think I like like the labeling on this form element. I would say "Determine" -> "Choose", since the administrator is making the choice, and maybe change the #title to "Default search module".
Comment #34
pwolanin CreditAttribution: pwolanin commentedIt all works well locally - but more testing certainly would be good.
Comment #35
jhodgdonIt does seem to be working correctly. I did some cleanup on the doc/comments, but the functionality is the same as in the patch in #32.
Comment #36
jhodgdonpwolanin: I'm for RTBC on the patch on #35. What say you?
Comment #37
pwolanin CreditAttribution: pwolanin commentedRobert Douglass looked it over too, so I think this it is.
Comment #38
jhodgdon#35: 567100-cleanup.patch queued for re-testing.
Comment #39
webchickOverall this looks good, no visual regressions from before. Two things:
I tested with an invalid path, and I get:
Error message
Notice: Undefined index: spoon in search_view() (line 30 of /Users/webchick/Sites/core/modules/search/search.pages.inc).
This means we're lacking test coverage for this branch.
What the heck is this? :P
40 critical left. Go review some!
Comment #40
pwolanin CreditAttribution: pwolanin commentedargh- patch doesn't apply
also, when putting in an invalid search path, webchick saw:
but that could be due to the hunk that failed to apply.
Comment #41
pwolanin CreditAttribution: pwolanin commentedI think some dope cleaned up up trailing white space in the doxygen. Hopefully this applies cleanly.
Comment #42
pwolanin CreditAttribution: pwolanin commentedoops - missed the new test module
Comment #43
pwolanin CreditAttribution: pwolanin commented@webchick - we do test searches against the word "pizza" - but probably jhodgdon can explain better.
Comment #44
jhodgdonProbably the tests need comments. My eyes are bugging out this morning... will take a look later.
Comment #45
jhodgdonHere's a patch with an added comment on the pizza line, as well as an additional test to make sure that we test that if you go to an invalid URL, you're redirected to the right URL.
As a note, with the latest patch attached, I cannot reproduce webchick's problem reported in #39.
Comment #46
pwolanin CreditAttribution: pwolanin commentedback to rtbc since the patch applies again, and jhodgdon was even energetic enough to add more test code.
Comment #47
jhodgdon#45: 567100-45.patch queued for re-testing.
Comment #48
jhodgdonWhat do we need to do to (please) get this committed?
Comment #49
pwolanin CreditAttribution: pwolanin commented#45: 567100-45.patch queued for re-testing.
Comment #51
jhodgdonStraight reroll. It looks like a drupal_add_css() line was removed from search_form(), causing patch not to apply. Not sure why/when, but whatever...
Comment #53
pwolanin CreditAttribution: pwolanin commentedlooks like the conflict was moving the css add to hook_init()
Comment #54
jhodgdonThis is a clean reroll of the RTBC patch above.
Comment #55
webchickOk, checked over this again tonight and looks good. It looks like a pretty massive and scary patch, but a good chunk is tests. Yay.
Committed to HEAD.
There are quite a few functions changing signatures here, so tagging as "API change". Please summarize what adjustments, if any, contrib modules will need to make as a result of this patch.
Comment #56
pwolanin CreditAttribution: pwolanin commentedThanks!
This is the only place modules might need a minor change
It was previously a no-op if the keys were NULL, or if the $type was not specified.
Since we can't assume 'node' is valid, there is no sane default to supply here, so the caller always needs to supply a value.
Comment #57
jhodgdonOf course, any module doing search_data will have already been supplying a module name to search their module's data... but OK, we can add a note to the upgrade page I guess? I am not sure an email notification of this api change is necessary. Actually or that documenting this change in the module upgrade guide is either.
search_view() has a change as well -- used to default to 'node', now defaults to the default search module (which is generally node unless node is disabled). Again, I don't know that this needs to be announced/documented as a change.
Another API change was an additional argument to search_get_info(), but since the behavior if you don't supply this argument is the same as it was before, and also this function is probably only used by search.module itself, I don't think we need to document that either.
Anything else that changed in the file that looks like an API change is really just a documentation change to clarify what was already happening.
Comment #58
rfayI will announce that search_view() and search_data() changed signatures.
Comment #59
mfbHere's a one-line cleanup to fix the default value (didn't seem worth filing a new issue :)
Comment #60
jhodgdonAre you seeing this not working? Why did you reopen this issue?
I think this is not necessary. I believe a while back there was a change to core so that on systems settings forms, the #default_value was used as the default for variable_get() and we didn't have to do variable_get() any more?
Marking it back to fixed until we get more information as to why you decided this was broken...
Comment #61
pwolanin CreditAttribution: pwolanin commentedit's not working right - the settings form shows node when another module is selected as default.
This patch fixes it.
Comment #62
mfbI can open a new issue if preferred (but it's a one-line fix and related to the title of this issue..) That change was rolled back so Drupal 7 settings forms still require variable_gets() unfortunately.
Comment #63
mfbComment #64
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #65
mfbI don't think this actually got committed? http://drupalcode.org/viewvc/drupal/drupal/modules/search/search.admin.i...
Comment #66
jhodgdonGood catch! It's definitely not committed.
Comment #67
marvil07 CreditAttribution: marvil07 commentedI can confirm that the last patch is still _not_ committed.
Comment #68
webchickOk, now it's committed to HEAD. ;) Thanks!