Make issue $node->project_issue[] namespace consistent
| Project: | Project issue tracking |
| Version: | 6.x-1.x-dev |
| Component: | Issues |
| Category: | task |
| Priority: | normal |
| Assigned: | dww |
| Status: | needs review |
We started this at #98278: project* namespace bugs in $node. We never finished the job due to how sucky and inconsistent core is during node hooks where $node is sometimes a true, loaded node object, and other times, it's an object of form values. This makes life difficult for PIFT (#519562: Provide work-around for: inconsistent placing of node properties by project_issue), it's complicating adding token support (#229063: project* should support tokens for pathauto and others), and generally causing special-cases and trouble.
Even though we don't just want to use #tree, since then the $node would be organized like the UI of the node form (e.g. with separate arrays for project_info and issue_info), we can define #parents and make everything end up in $form_state['values']['project_issue']. I got it all working for new issues and editing issues pretty easily, but that broke issue comments. I'll see if I can quickly sort that out, and if so, I'll post a patch...
#1
Initial patch that fixes things while editing issues. Previously, $node didn't include *any* issue values on edit, since we don't inject those form elements at all. That totally breaks #229063: project* should support tokens for pathauto and others for example. This patch propagates all of those values via 'value' form elements when an issue is being edited. Definitely doesn't solve the whole issue, but seems like a good start.
#2
FYI: here's my technical proposal for fixing everything else:
Issue node form itself:
a) keep the project_issue node form UI the same with the fieldsets. Do *not* use #tree in general.
b) values that are just getting propagated go into a #tree'ed $form['project_issue'].
c) values that are selected define #parents so that the value ends up in $form_state['values']['project_issue']* (exactly like we do in project_release).
Comments:
d) since we're already moving crap around out of the node form, start clean with a #tree'ed $form['project_issue'] array. move all the form elements we care about into there, and turn that into our "Edit issue settings" fieldset in terms of the UI.
e) fix all the validation/submit/preview etc logic to deal with $node->project_issue[] instead of project_info and other randomness and special-cases.
#3
I committed the patch from #1 to HEAD, since I needed it for #229063: project* should support tokens for pathauto and others. Back to active for the proposal in #2 and related fall-out.
#4
Wow, that sucked. ;) The code to integrate with comment module for issue followups is pretty nasty, and core's comment API sucks.
2.d) wasn't quite possible. We still need nested arrays to get the UI we want, so we can't just use #tree'ed $form['project_issue']. So, much of the $form structure of the altered comment form is the same, other than all the #parents definitions from the issue node form.
2.e) was mostly possible, but wow, what a tangled mess. :( I feel like this patch is still an improvement. But, it's still not exactly elegant and beautiful by my highest standards... However, I *really* don't have time right now for a more massive restructuring to try to conquer this in a cleaner way.
Things I've tested:
new issue
- previewing
- validation
- submit
new comment
- previewing
- changing metadata
- validation (not exactly right, but it was a bit spotty from before the patch, so this isn't a regression). it still all eventually works, but it doesn't always stop you at preview (sometimes, only once you try to submit do you get the validation error) and the invalid form element isn't always marked.
- submit
edit comment
- publish/unpublish does the right thing to the issue's metadata based on the previous comment
- publish/unpublish does the right thing to the issue's metadata based on the original issue if it's the only comment you're pub/unpub'ing
delete comment
- restores issue to previous metadata (from the original issue if you delete the last comment).
Yes, it'd be great to write automated tests for all of this... *sigh*