Posted by csevb10 on July 26, 2010 at 10:21pm
4 followers
| Project: | Drupal.org Redesign |
| Component: | Search |
| Category: | task |
| Priority: | normal |
| Assigned: | csevb10 |
| Status: | closed (fixed) |
| Issue tags: | drupal.org redesign, drupal.org redesign solr, drupal.org redesign sprint 1 |
Issue Summary
Make the project_solr_browse_projects_form (in project_solr) work for both the D&E pages (which it does currently) and the the standard search page.
This may involve changes to project_solr or drupalorg modules (or both) as needed.
Comments
#1
Tagging correctly.
#2
Attached are 2 patches for review.
1. small modification to project_solr to allow the form to be called from other locations.
2. a larger patch for drupalorg_search that creates the form in a block and handles submission issues.
#3
include_once drupal_get_path('module', 'project_solr') .'/ProjectSolrQuery.php';seems a bit odd to put into a submit handler. i'm just curious why we aren't including this file in a more top level fashion in project_solr?else if ($op == 'view' && $delta == 'drupalorg_search_browse' && apachesolr_has_searched() && ($response = apachesolr_static_response_cache()) && ($query = apachesolr_current_query())) {is a pretty beefy line of code to be missing a code comment ;)#4
a. This is included in the submit handler because we're calling out to create a ProjectSolrQuery. This handler gets called from the form context (where we don't include ProjectSolrQuery because we're not actually using it explicitly) and from the page context where we pull the form (where we do call ProjectSolrQuery explicitly and therefore the include_once exists. I can move this to drupalorg_search where we're calling the submit, but it seemed more centralized to do it here in case this submit was used in any other context.
b. You're right. I was simply staying consistent with the other else if statements in the file, and I shouldn't have. I've attached attached a version with every else if in the file changed to elseif.
c. It's pretty much the same as all of the other lines above for blocks. Some have slight differences, but all an all, they're pretty similar. I've left this alone because this is pretty common for solr-driven blocks.
d. It is quite a bit of work, but similar to project_solr's implementation. project_solr only checks the im_vid value (which the generic search bypasses for meta_type). That being said, I turned it into a helper function in this context.
#5
posting a relevant conversation i had with csevb10 regarding this issue:
[3:31pm] hunmonk: csevb10: drupalorg_search_form_project_solr_browse_projects_form_alter() is pretty hackish -- is there no better way to do some of that stuff in there?[3:30pm] csevb10: hunmonk: not disagreeing, that's why I wanted someone to look at it
[3:30pm] csevb10: hunmonk: the path rewrite seems reasonable as does the submit handler - in my opinion, it's just that last part for text that is the real hack aspect
[3:31pm] csevb10: hunmonk: and there very well might be something better, but I couldn't think of it
[3:31pm] hunmonk: csevb10: well, i'd prefer some other way to get the context besides arg()
[3:31pm] csevb10: hunmonk: I'm open to suggestions, though
[3:31pm] hunmonk: csevb10: but if that's not possible...
[3:32pm] csevb10: hunmonk: it's kinda standard to the url conventions of solr, but I can look around
[3:33pm] hunmonk: csevb10: i'm not clear on why you have to put something in $form['text'] -- can you explain that whole thing a bit?
[3:35pm] csevb10: hunmonk: yeah. no problem.
[3:35pm] csevb10: basically, the submit handler actually checks and modifies the sorting algorithm
[3:35pm] csevb10: based on the presence/lack of presence of "text"
[3:36pm] csevb10: so I populate the field but suppress it, since we're not using it
[3:36pm] csevb10: so sorting can work as expected without having to duplicate effort
[3:35pm] hunmonk: csevb10: $filters = explode(' ', $form_state['redirect'][1]['filters']); <-- yikes. can't we put something in $form_state['storage'] instead?
[3:36pm] csevb10: hunmonk: I just explode there so that I don't have to do all sorts of rework building the filters
[3:37pm] hunmonk: csevb10: i feel like we're hacking around a hack in the other submit handler
[3:37pm] csevb10: hunmonk: a bit. but it's relevant to how project_solr works for the submit
[3:38pm] hunmonk: csevb10: but $form_state['redirect'][1] doesn't seem very stable -- what if another redirect gets added?
[3:40pm] hunmonk: csevb10: i wasn't even aware that $form_state['redirect'] could be structured like that
[3:48pm] hunmonk: csevb10: oh i see, $form_state['redirect'][1] is the query array that eventually gets passed to url?
[3:50pm] hunmonk: csevb10: i'm really not a fan of that approach -- it's too likely to break if some other module does something to the redirect
[3:51pm] hunmonk: csevb10: and if there's really no other decent way to accomplish what you're doing there, we minimally need to explain what's happening
[3:55pm] hunmonk: csevb10: i hate to be a stick in the mud, but what it seems like here is that the project browsing form isn't well abstracted enough yet to handle this multi-use elegantly
#6
I committed the s/else if/elseif/ as a separate commit: http://drupal.org/cvs?commit=403158
I also just committed #875418: Split up drupalorg_search_block() into separate functions
So, here's a new patch. All of hunmonk's prior concerns are totally valid. csevb10 and I agree. But, it's late the night before a launch, and we don't want to start from scratch on how all this code *should* be right now. So, we're just going to plow ahead to get *something* working and leave this ticket open to revisit the nasty parts and come up with a cleaner solution when we're less stressed for time. ;)
That said, I fixed a few things in the previous patch:
A) drupalorg_search_get_parent_term() and the whole "parent_term" terminology is misleading. It's really "project_type". Renamed accordingly.
B) Since we're hard-coding the tids for 'module' and 'theme', it's rather pointless to be doing this test:
+ if ($term->vid == $project_vid) {;)
C) Added PHPDoc for the drupalorg_search_get_project_type_term() helper function.
D) Fixed/added a few other minor code comments to be more clear/accurate.
So, really, the only thing that still sucks in here is the form alter and submit handler. That's the main thing that needs to be fixed going forward.
#7
Here are the issues for the follow-up fixes:
#875436: Change "parent_term" to "project type" in project_solr
#875438: Make project browsing solr navigation form reusable
Since those are now open, I think we can safely close this once we commit it. Giving this a better title based on what this issue is actually doing.
#8
Also, I just committed project-866510.patch to HEAD of project_solr.
Bill and I are just in the process of testing the rest of this on a solr dev site. Once it's working, I'll commit to drupalorg_search. Stay tuned...
#9
I made 1 small change because response handles fq differently for single and multi-facet filtering.
This allows the fq to be a single filter (such as ss_meta_type=theme only with no additional filtering)
#10
Nice catch. Rerolled that with a code comment for clarity.
Also, the newline after break; is part of the core coding standards, so I left that in.
#11
Testing this uncovered another bug in project_solr: #875478: Move 'drupal_core' field alias stuff into prepare_query() -- that's now coded, tested, and committed... Getting closer. ;)
#12
More testing confirms this is working. There's a slightly weird interaction with the "Or filter by ..." block, but that's for another issue. One tiny other change is that I renamed the delta to drupalorg_search_project_browse. With that, committed to drupalorg HEAD. Hurray!
#13
Automatically closed -- issue fixed for 2 weeks with no activity.