Posted by dww on January 20, 2009 at 7:24pm
| Project: | Project issue tracking |
| Version: | 6.x-1.x-dev |
| Component: | Comments |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | drupal.org upgrade |
Issue Summary
One of the major bugs in the D6 port from the initial patch at #157693: Upgrade project issue to Drupal 6.x is that issue followup comments are completely broken in HEAD right now. Fixing this could be done in parallel with other porting-related tasks, so I'm splitting this off into its own issue.
Comments
#1
Just needed params and return to be adjusted to D6 FAPI. Patch attached.
#2
N.B. this still doesn't work 100%, but the above patch solves the fatal error. This is due to #361650 which I'm working on now; I suggest that this issue be considered "fatal error when attempting to comment on an issue" because the remaining problems seem to revolve around comment_upload.
#3
Cool, thanks for taking a stab at this. However, in testing your patch, I noticed that a huge part of the problem is that hook_comment() changed in D6, there's no longer a 'form' op, so the whole comment form is busted since we're not injecting the right form elements anymore...
#4
#82526-15: deprecate hook_comment('form') in favor of hook_form_alter() -- this is now documented:
http://drupal.org/node/114774/revisions/view/405000/421906
#5
Fixes hook_comment issue by moving the code from the 'form' op into project_issue_form_comment_form_alter. Now submits a comment fine and allows editing of issue settings properly; displays changes in the comment, as expected.
I get a SQL syntax warning from mail.inc as a result of submitting, but I don't believe that's a problem for this patch. I did change one character in mail.inc; _comment_upload_load_files was renamed to remove its leading underscore, so I changed it in mail.inc as well (was previously causing a fatal error during comment submit).
#6
Cool, we're getting closer. However, the change to project_issue_form() in #5 is wrong, since the $node comes first, then $form_state (according to the hook_form() docs).
However, since it's now working at all, I fixed the hook_form() problem myself and committed the rest of this to HEAD. Setting back to active for the following things I've observed that are still broken:
A) If you change any issue settings except the title, when you preview, you don't see your diffs.
B) If you change the project, the JS to re-populate the Version, Component, and Assigned fields fails with a JS error.
C) If you don't change anything about the issue, the validation doesn't prevent you from submitting an empty comment.
D) All comments are supposed to be automatically set to have a parent of 0 so that if you ever delete one, you only delete that instead of a subthread. Now, issue comments are showing up in the DB threaded, and if you delete one, you can wipe out a set.
E) Previewing the comment displays weird things by putting the comment it thinks you're replying to above the issue and metadata table.
#7
This should fix A, B and C. D and E are the same bug I think; coming tomorrow.
#8
Re: #7 -- Interesting, those woes with project_issue_metadata_changes() are coming from #98278: project* namespace bugs in $node. Not sure if the approach here is the right one, or if we should try to modify project_issue_metadata_changes() itself. That requires a closer look.
Anyway, I'll hold off on committing this until we refactor more of hook_comment() into hook_form_alter() -- I'd much rather inject #validate and #submit handlers via the form_alter() than monkey around with hook_comment() limitations and lameness...
Thanks again for working on this!
#9
OK. I haven't changed anything with project_issue_metadata_changes()
I was able to move validation in to a #validate handler, but couldn't get the 'insert' op of hook_comment into a #submit handler because there's no way to access the cid. Semantically, I'm OK with that--the validation definitely belongs in a form validator, but the actions we take in the 'insert' op of hook_comment are really directly related to database insertion.
This patch also raises a new error from comment_upload, to be dealt with in a subsequent patch, but it successfully addressed A, B, C, D and E concerns raised above.
#10
I need to review netaustin's patch, which I'm planning to do on the trip to Boston. I've got this issue and the patch cached on my laptop, and my copy of the d.o CVS repo is freshly rsync'ed. ;)
#11
Gave this a big workout on the flight to Boston. There are a few lingering problems, some quite fundamental. I'll post more details (and a patch that fixes some issues) once I can get my laptop online. Meanwhile, no one should spend time on this. Thanks!
#12
This is still broken because we need to rebuild the form when the project changes along the lines of http://drupal.org/node/331941. But, it's closer... so we should continue from here.
#13
Try this one. Now with pure #ahah support and no custom javascript. If you accept this patch (or some future derivative) you can remove project_issue.js.
#14
Improved #ahah support; no longer spits out JSON on page load, no longer posts duplicate comments. Tested many permutations of fields when previewing, when submitting directly, when replying to a previous comment, and when posting a new comment. This should be close to done. Read here for more detailed descriptions of the problems overcome.
I updated my working copy between the prior patch and this--truncate cache_form before testing.
#15
Excellent, that was most of the way there. I fixed a few minor things:
- The logic in project_issue_update_project() to set the new default value for the version selector that tried to compare the labels, not the keys, wasn't working for 2 reasons: The $rid in the new system is just the nid, not the version string, so we need to query for the old version string and compare that. Also, just setting #default_value isn't enough with all this form rebuilding mojo -- we also need to set #value for it to take effect.
- Removed some commented out debugging lines that were in there.
- Cleaned up a bit of whitespace.
Otherwise, it was all looking great, and worked under my testing, so I committed to HEAD.
Setting this back to active since when I tested with JS disabled, it doesn't degrade correctly at all. It's midnight here, and I really need to sleep now, so I have no energy/time to debug it. Can you work on this?
Thanks!
-Derek
#16
I'll take a look at what happens when JS is disabled tomorrow afternoon.
#17
subscribing
#18
I worked with the patch #14 and there are some issues I've found:
1) project_issue_form_comment_form_alter() alters all comment forms.
When I comment a Story (e.g.) the "Submit Issue" form is displayed with the 'Invalid project selected' error.
2) There was the 'Invalid project selected.' error when I click 'Report new bug' link (node/add/project_issue/short_name/bug).
It seems the line $pid = $node->project_issue['pid']; was forgotten from the old (D5) code.
3) There are a lot of notices I see working with the module.
I had to disable E_NOTICE in drupal_error_handler() to work normally (if ($errno & (E_ALL ^ E_NOTICE)) {...}).
4) Concerning disabled JS.
A project Foo has versions, a project Bar hasn't. If I switch the issue from Bar to Foo durning a preview phase then I'm unable to submit the issue. I've got an error 'You have to specify a valid version.'
The patch fixed (1) and (2).
#19
Thanks, I committed #18 to HEAD (after removing the stray whitespace hunks).
Re: 3) Yeah, oddly, I can't get my test site to display notices, which is most frustrating...
Re: 4) Indeed, that's what I'm talking about. If you turn off JS and try to switch the project at all, you can't submit.
#20
Another bug -- if you only change the "Assigned" field, the validation fails because it thinks nothing changed about the issue.
#21
Fixed as many notices as I found (put a patch in project for one too), made the form degrade gracefully if Javascript is off, in a way that limits FAPI rebuilding to only when the project itself changes. Fixed the "assigned" issue, so the issues in comments #19 and #20 are resolved.
dww: to turn notices on, you have to follow the normal php.ini steps AND ALSO change includes/common.inc:580 to "if ($errno & (E_ALL)) {"
The only outstanding problem with issue comments AFAIK is related to comment_upload.
#22
@netaustin: Yes, I've checked both places for enabling notices, and they're just not showing up in safari. I am seeing them in FF, though, so I'll test there for notice smashing...
The patch is now sort of working -- however, it lets you submit issues without a version, even though version is supposed to be required and
<none>is supposed to be invalid. Without the patch, it seems to validate version correctly.Haven't tried to debug, I have to review another patch right now...
I also haven't looked at the actual code in the patch, I might find something in there, too, but I'll save the detailed review until the above is working.
#23
We're not using HEAD for the Drupal.org upgrade. Removing tag.
#24
David missed -- wrong issue. ;) We are using project_issue HEAD for D6.
#25
Fixed above issue.
#26
attached takes austin's stuff and does all the following:
#27
Had some minor code style problems hunmonk fixed locally... please commit.
#28
* Splits form_alter into two version
* Fixes a few more notices
#30
use this one
#32
whoops.
#33
Committed this to HEAD last night. There are still some edge-cases that aren't quite right with comment_upload, but let's finish those off over at #361650: Port project_issue to D6 version of comment_upload.
#35
Automatically closed -- issue fixed for 2 weeks with no activity.