issues page: problems with query filter form values vs. URL

yup. there are a *ton* of bugs in how the project module's issue query/search interface "works". ;) moving this to the right issue queue. i'm about to go out of town for 3 weeks, so i have no time to deal with this (or the dozen or so similar issues in the queue) anytime soon. it'll probably take a major, sustained effort at reorganizing the issue code. i'll probably try to tackle it after converting project to use comments for issue followups (http://drupal.org/node/18920).

we might just rip all of this issue query code out and re-do it all with the views module or something...

we might just rip all of this issue query code out and re-do it all with the views module or something...

Sounds like a good idea. We have two general problems here: (a) filtering items in a list (the top of page selects that filter what issues are listed) and (b) advanced search. The first is the sort of thing that views excels at. AFAIK we don't yet have this sort of top of page filter--do we? If not, this might look like a contrib views module. For the second, I think ideally we'd have a core search API could be extended in this way, rather than having to bypass the core search and build something custom. This would require a fair bit of work on core search, though.

as far as i can tell from my own experience with this stuff, the sorts of bug reports i've seen, and a brief inspection of related code in the past, most of the bugs stem from confusion in the query code between 4 possible sources of filtering information:

1. the URL path itself (e.g. http://drupal.org/project/issues/project) which selects a specific project to filter on
2. arguments to the URL PATH (e.g. http://drupal.org/project/issues?projects=3060&states=8,13,14) which can filter various things
3. the values of the form elements on the page that are selected and then submitted
4. what page of the query you're on

the issue query code is not careful about which of these values takes precedence in case of conflicts, how to save certain values from one (which don't conflict) when primarily building a query from the other, etc. frankly, it's a huge mess. ;) the pager stuff in particular doesn't seem to play nicely with the URL-related search filters. i think the only way to get the pager stuff to work reliably is to select your query based on the form elements, not the URL (which doesn't make much sense, but i think that's how it goes given the current code base).

in general, if you attempt to refine your queries using only one of those methods, not a combination of them, it usually works pretty well. so, if you like the form elements to select what you want, then strip all the goo out of the URL. if you just want a specific query, you can get the URL right and keep hitting reload...

good luck,
-derek

btw, here's an older, related (basically duplicate) version of this bug:
http://drupal.org/node/33262

minimal changes to 4.7, please. ;)
there's a separate issue about views support (http://drupal.org/node/76725)
see comments there.

thanks for taking a stab at this!
-derek

i was thinking about this approach in the past. i'm not sure i like it, though. i agree that stripping off the 'foo' and going back to /project/issues is probably the right thing if the user selects 'all'. however, i kind of like that the 'foo' stays there if you're still searching foo. ideally, the #action would be conditional on the submitted value of $form['projects']. if you change the project in the form to 'baz', you get sent to /project/issues/baz. if you leave it 'foo', you stay on /project/issues/foo. if you set to 'all', you just goto /project/issues. i think that'd be best, if it doesn't make the code too ugly. ;)

the reason i like keeping the project name in the URL as much as possible is that it helps people bookmark a specific project's issue queue more easily. i use this all the time, and it'd be a pain in the ass if the URL only included the project the *1st* time you visited the issue queue for a project (e.g. following a link from the project node) but then as soon as you did anything on that form (like hit 'search' to refresh the page instead of your browser's reload button), the URL was gone.

furthermore, in playing with this patch, the behavior seems to be slightly broken in subtle ways. :( for example, if you specify certain states in the URL, like so:

?q=project/issues/event&states=1,2,3,4,5,6,7
(no clean URLs on my test site right now)

in the current code, those states remain in your URL, even after searching, and that makes it clear where the options in the "Status" dropdown are coming from. with your patch, as soon as you hit the search button again, that part of the URL gets wiped clear, though the result is still remembered by the form in terms of the options for Status. given that there's no way on this page to select a set of status values other than the URL (which is another issue ;) but currently the state of the world), i'm reluctant to make a change that tends to clear those parts of your URL away. maybe it's just me, and i'm the only one who does a lot of per-status querying of the issue queues via URLs, but it currently works fine, and your patch would make it harder to do.

so, i'm tentatively setting to "needs work". i could still be convinced to commit this one as is, but i'd rather some of these issues were addressed, first.

once again, thanks for your help combatting the massive project + project_issues issue queues! ;)
-derek

and 'foo' stays there if you're searching for 'bar', or 'baz'... i can see what you're saying, but the current form doesn't guarantee usable urls beyond the first search either, unless you to stay in the same project. (i've got no idea what percentage of people just search on one project in a session.)

exactly. that's the bug this issue is trying to fix. ;)

but i just want to check that you'd rather have a redirect based solution before going any further.

no, i don't want a redirect. ;) my rough idea for how this could work without a redirect was almost right, and luckily, chx was around in IRC to fill in the missing pieces of the puzzle.

ideally here's what should happen:

1. we'd add a reference to $form['#action'] to the $form['projects'] array (so we could modify #action later)
2. in the validate hook, once we made sure the project was legal, we'd change the #action
3. that should send us to the right place once the submit is finally done and we're gathering/displaying the results

unfortunately, there are 2 problems with this approach:

• i can't seem to get any kind of validation working on that damn form! i spent almost an hour in IRC w/ eaton, killes and chx, and had no luck. :( something very screwy is happening with that form. :(
• eaton claims that the ordering of how the form is rendered, posted, validated, etc, etc makes chx's proposed plan impossible. ;) eaton claims validation happens *after* we post to the #action.

so, i'm not sure the above could work at all. i'll take another look at this validate crap (since i'd like to understand what's going wrong, anyway), and see if i can get my funky change-the-#action approach to fly. if not, i guess we'll have to mess with a redirect and drupal_goto(). however, i'd like to avoid that if this doesn't spiral into a major time-sink.

feel free to roll a goto-based patch if you want to start on that. i'm sure yours will be done and working before mine. ;) i'll decide once i see the goto approach how much effort it's worth spending on the alternative...

thanks!
-derek

i was just defering to chx's vastly greater knowledge of FAPI + drupal, and he was confused about exactly what i was trying to do. everyone now agrees my previous scheme is impossible. ;) please carry on with the goto solution for when JS degrades, and if you're so inspired, feel free to make a JS version that avoids the goto.

thanks!
-derek

p.s. in IRC, chx previously mentioned this:
chx: you can do a fast internal redirect by setting $_GET['q'] (and calling menu-set-active)
not sure how much we care, but justin, if you care to investigate this approach, i just wanted to share.

previous patch included 1 hunk that was from http://drupal.org/node/57023, which no longer applies cleanly. this patch just includes the new code for this bug. i'm about to review, but wanted to post a clean patch in case anyone else wants to review/test this. also, moving to the right issue queue...

changing the title to more accurately reflect the scope of what we're trying to fix here.

sadly, i never set this issue as duplicate with the older http://drupal.org/node/33262 i mentioned above. but now that we've got patches in here, and lots of other issues pointing at this one, i just marked #33262 as the duplicate with this.

alas, this is invalid php:

function project_projects_select_options($issues=TRUE, $key_prefix=NULL, &$project_urls=NULL) {

for no apparently good reason, you can't give default values to variables you pass by reference. the above yields a syntax error on my test site (php 4.4.2).
unfortunately, there are about 6 call sites that invoke this method w/ no args, yet the project_urls variable needs to be passed by reference so you can get both the list of projects and the uri <-> node mapping array from the same DB query and function call. :( probably, you'll have to make the project_urls the first arg, w/o a default, and change all the call sites that don't care to be project_projects_select_options(NULL) that's a little lame, but probably the lesser evil.

i made that much of a change, but these patches are still broken. ;) the URL is getting changed, but:

• there's a bunch of extra junk stuffed into the URL i'd rather remain hidden. for example, on my test site, if i goto /project/issues (where i see issues from all projects), then select a given project ('event', in this case) and hit "search", my URL becomes: project/issues/event?js_enabled=1&states=1,2,8,13,14,4&categories=0&priorities=0. what's the "js_enabled=1" doing in there? ;) furthermore, i'd rather all the state/prio crap didn't clutter the main URL in the browser.
• that url, even though it's got the "event" in there, isn't working, and the search form is still showing me all projects, and a project column, not what i see if i directly navigate to "project/issues/event" myself. the problem is my test site doesn't have clean urls enabled, so you end up with two '?' characters in the URL, which confuses everything.

seems like project_issue.js needs to be smarter about 2 things:

1. sites without clean URLs enabled (if there's already a ?q= at the front of the URL, the one you're adding in project_issue.js breaks things).
2. not adding junk to the URL for specific states, categories, priorities, etc, unless they were already in the previous URL. so, if i manually navigate to /project/issues/event?states=1,2,3,4,5,6,7,8,9,10,11,12,13,14, it should preserve that states argument, but it shouldn't add its own version of the current states the the URL automatically -- it just makes the URLs ugly and hard to follow.

that said, the little '#' link on that page should continue to include the full URL with all the ugly args such that you've got a link to the exact query you've selected. that way, you'd have access to the full URL if that's what you really wanted, but in general, the URL wouldn't become totally unwieldy.

unfortunately, my attempts to manually disable project_issue.js's behavior of adding all that to the URL ends up just blowing away the search values. :( JS is something i know almost nothing about. ;) perhaps there's some smart way to get your code to interact with the $query object that's so heavily used in the rest of the code...

also, i'm awfully scared by a lot of the comments in these patches. there's no way i could commit this as-is, even if all of the above were fixed. ;) i'm fully in support of the general direction of the patch, but some of the specifics should be cleaned up. e.g. this whole buisness of "we should probably re-post the URI so we don't have to search for it again". it's 3:45am here, and i need sleep, so i can't quite wrap my head around what you're talking about. can you roll a patch that does it that way, remove the scary comments, and myself (and others) can review, test, and work with that?

thanks for all your work on this so far! this is going to be great once it's finally working. ;)
-derek

patch to project.module from #23 doesn't apply cleanly, since i the patch includes a fix i already committed.
here's an updated patch that's clean, in case anyone else wants to test this. ;)

the patches all apply cleanly now, and don't cause php errors. ;) that's nice.

sadly, this still isn't quite right, yet. :( with the patches applied, the UI elements other than selecting the project no longer work -- you can't filter on status, category, etc. whatever you select, once you hit submit, you go back to <all> and all issues from that project are displayed.

also, once i disable the JS, if i'm on the main project/issue page, and select a specific project, the URL goes back to having tons of cruft:
project/issues/signup&states=1,8,13,14,2,3,4,5,6,7&categories=0&priorities=0
in that case, at least the UI elements work to filter (and the URL gets updated), but we've previously discussed why it was nicer to keep the main URL more clear, and keep the specific URL to that particular query in the # link.

i have no time to sort this out right now, since i've got other fires to put out. justin, hopefully you can fix this and submit new patches. please test both with and without project_issue.js.

thanks!
-derek

clean version of latest patch

well, this is highly strange. the patches all apply cleanly (i'll include a list below of the current links to each, since i know it's getting confusing). when i install them all on a shared hosting test site, they appear to behave great. the one exception is that the URL is still weird when using the pager, though that's been broken for a long time -- they're no worse with these patches than without. if your browser has JS enabled, everything is happy and slick, the URL always moves to match the currently selected project in the dropdown, etc. if project_issue.js isn't there or disabled, your URL is still updated via a redirect, though unfortunately the entire query filter arguments have to be stuffed into the URL itself. the URL keeps changing if you use the form elements to alter the query, so it's not that bad. just a little less visually easy to work with. however, non-JS is such a small audience, i'm fine with this as the "degrading" behavior -- everything is still correct, the URL is just more verbose.

however, a completely crazy thing happened when i installed all this on my laptop (macos 10.4.7, Apache/1.3.33 (Darwin) PHP/4.4.2, mysql 4.1.19). the $_POST ends up completely empty after project_issue.js does its thing. :( justin and i debugged this extensively -- we walked through the .js and threw in alert() calls to make sure things were happening as expected. we used tcpflow to capture the raw data on the TCP socket, and the post variables are indeed being sent over the network interface. however, by the time we hit index.php, $_POST is empty (though only for this particular form -- other forms continue to work fine). JS is otherwise working flawlessly on my machine -- collapsible fieldsets are no problem, etc. basically, it appears that somewhere between apache and php, the POST data is getting lost with this .js in use. again, the identical code, .js and all, works fine on some shared hosting test sites (php4 and php5). we're basically completely stumped. however, it seems pretty clear that it's a problem with my laptop, not the code itself, which is why i'm setting this back to "needs review".

can some other folks please install the following 3 patches on an otherwise clean 4.7 project and project_issue installation? the main thing to see is that once you create a few issues for a few projects, when you switch the "project" drop-down on the /project/issue page to a specific project (instead of "all"), that the URL goes to /project/issue/foo. once you're there, make sure you can filter by something else (e.g. select just 'bug reports' in the 'category' drop-down). if those 2 things are working, please let us know. i'd *love* to commit this patch, but now that i've had such a nightmare on my local test environment, i'm scared. :(

for your convinence, here are the current patches you want to use:

http://drupal.org/files/issues/issue.search_4.patch.txt
http://drupal.org/files/issues/project_projects_select.patch_2.txt
http://drupal.org/files/issues/project_issue.js_0.txt (install this as modules/project_issue/project_issue.js)

please share your experiences, and details of your server/browser configuration(s).

thanks!
-derek

I'm experiencing the same problem here... I'm also on a Mac, but different versions of stuff (Apache 2, PHP 5, MySQL 5).

Filtering by projects works fine, but trying to filter by any of the other properties (status, etc.) it just returns the form.

print_r($_POST); just returns Array().

another datapoint: the webserver for the drupal sites at my day job. works like a charm:
Linux 2.6.9
Apache 2.0 Handler
mysql 4.1.20
PHP Version 4.3.9

oy vey. in IRC, chx declared:

chx: dww: you caught a bug in PHP 4.4.2 for MacOSX
chx: dww: it's not the first time the Drupal project discovers a PHP bug
Derek: though, webchick had the same problem w/ php5 she claims.
chx: there are common code parts in php4 and php5
...
chx: submit a php bug report
chx: i myself submitted three in the last year
chx: one was accepted and fixed :)

chx, justin and i quickly decided that since mac is a pretty rare server platform, and that the non-JS version of justin's patch works fine on macs, that the solution will be that the code in issue.inc that's attempting to load the project_issue.js file should just conditionally check for "Darwin" in $_SERVER['SERVER_SOFTWARE'], and if it's there, don't attempt to load the .js. yes, that's definitely a hack, but this seems like a lesser evil to either a) allowing these bugs to persist in the common case, b) waiting for the php bug to be solved, or c) spending a ton of time attempting to find another way to hack around the php bug.

this is highly crappy, but i'm really not sure what else to do given the circumstances.

another thing i just noticed which "needs work" is that the new JS-enabled code loses the default set of issue states defined near the end of project_issue_query_parse():

    $query->states = array(1, 2, 8, 13, 14, 4);

that stuff no longer works with the new system. those states are selected the *first* time you visit a given issue page (/project/issue or /project/issue/foo) but the default value for "Status" is filled in as <all> and so as soon as you hit "search" you get everything (and this particular set of states isn't included as a choice in the drop-down, either). seems like the default value should remain that set of states, and <all> can be one of the options if that's what you really want.

needs review.

sadly, i'm out the door for vacation, so this will either have to wait until next week (i get back sunday 2006-08-27), or nedjo will have to review, test and commit it. ;)

thanks for all the work on this, justin -- this will be a great change once it gets in!

-derek

finally had a chance to review/test this again. overall, it's great. i just rolled a new patch to clean up a few cosmetic problems with the patches (re-worded some comments, removed some stray whitespace/formatting, etc). also, to make it easier for others to review/test this, i'm rolling all 3 changes into a single patch (including the new project_issue.js file). if this weren't such a big change, i'd just commit it directly. however, given the scale, and my relative lack of JS knowledge, i'd really like to get dries or nedjo to review/comment on this before it goes in. so i'm leaving it as needs review, but with a big dww-reviewed-and-tested-this-and-thinks-its-RTBC +1. ;)

i spoke to dries via IM about this. in a nutshell:

well, i can imagine that similar forms have similar problems
and using a javascript solution feels 'weird' to me
i think elsewhere we just submit to a 'catch all' form action handler
and then do an additional goto based on the form data
...
i don't have strong feelings about this
it's just something that you might regret later ;-)
it's not build using jquery, for example
so it means more work (and potential clean-up) in future
...
i don't have problems with this patch

so, basically, he thinks it's RTBC as-is, but perhaps we'd all be happier in the long run with a non-JS approach. justin, i know you've offered to work on trying to clean-up the non-JS version, anyway. given the macos php bug, the fact that not all browsers will have JS enabled, the fact that dries is ambivalent at best about using the JS for this (and even suggested the post-to-a-catch-all-then-redirect solution), and given that we already have most of the goto-based approach done in this patch already, how much work do you think it would be to try to clean-up the non-JS case (to do something fancy with the form values instead of stuffing them into the URL) and just rip out the JS entirely? if that's only an hour or two of work, perhaps that'd be the best solution in the long run. however, if it's a major undertaking, i'd be happy to just commit what we've got here and revist any of this in the future if it turns out we want/need to change it later.

thanks,
-derek

sweet! ;) it even works on my macos laptop test site. ;) i think this is probably going to be the way to go. now i just have to pour through the code with a fine-toothed comb, but i'm hopeful this will be RTBC.

thanks!
-derek

drat, almost there, but i found a problem with the advanced search form for specific projects. if you're on the general advanced search form (e.g. ?q=project/issues/search) things seem to work ok, but if you goto a project-specific advanced search page (e.g. ?q=project/issues/search/foo) all hell breaks loose. no matter what you select, it sends you back to the general ?q=project/issues page, and the status drop-down now has a blank entry selected by default (for no status, i suppose), and no issues are found. :(

seems like the session/redirect mojo is getting confused by the project-specific advanced search form, and we're ending up with input we're not expecting and doing the wrong things with it. a quick look indicates that project_issue_build_form_url() is getting confused in this case since $_POST['edit']['projects'] isn't set (in fact, $_POST is totally empty), yet the URL already contains the project name. i don't have time right now to fully debug this and roll a new patch, but i suspect it'd be pretty easy for justin to fix, given this info.

one other minor gripe: it'd be nice if the permalink # thingy used the same url style with the project name instead of a &projects=2 argument. e.g.:

?q=project/issues/foo&states=1,8,13,14,2,3,4,6,7&categories=bug&priorities=2

instead of:

?q=project/issues&projects=2&states=1,8,13,14,2,3,4,6,7&categories=bug&priorities=2

i'd be happy to just handle this part as a separate patch. but, if you're re-rolling anyway and it's not much trouble, this would be nice to help keep things consistent.

thanks!
-derek

ahh, another tricky problem. :( the "my issues" page is still screwy with this patch. basically, http://drupal.org/node/56200 is still broken. in a way, it's slightly worse now, since the "user" part of the URL gets stripped off and replaced with a project name if you filter on a specific project. but, the current behavior is just as broken, so this isn't a new bug. however, we'll probably have to change some of the code in this patch to really get #56200 fixed. just wanted to put a note here to get us thinking about this so we can try to come up with a reasonable approach while we consider the final version of this patch.

code looks great, solves all the problems i've seen with previous iterations of the patch. i got killes and webchick to look this over, and everyone's happy. applied to TRUNK and 4.7. woo hoo! ;) just getting killes to update d.o now.