Provide work-around for: inconsistent placing of node properties by project_issue
deekayen - July 15, 2009 - 03:07
| Project: | Project issue file testing |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | closed |
Description
I submitted a new issue, attached a patch, and put it into needs review state. The status of the patch goes to ignored until I post a followup comment. Only after the followup comment does it queue for testing. Is this intended behavior? webchick thought it was weird.
Screenshot is of http://d6.drupal.org/node/367046
| Attachment | Size |
|---|---|
| ignored_patch_state.png | 64.57 KB |
#1
Not intended.
#2
Investigating locally.
#3
Looks like underlying project_issue code changed. Arg.
#4
Seems to be the fix.
Project issue removed the:
<?php$node->project_issue
?>
prefix from all their node fields.
#5
Committed.
Will try and get it deployed.
#6
Good only project_issue is now nice and inconsistent...seems during node creation it has not prefix, but later it does....
#7
Reverted.
#8
This is is blocking ATS 2.x, and if deployed I am going to guess it will break PIFT 1.x.
#9
Core is the root of the inconsistency here. Sometimes a $node is a node, other times, it's an array of form values. It's a huge pain in the ass. My efforts to get core to handle this correctly were thwarted and I gave up. So, project_issue is doing the best it can. You just have to suck it up and be inconsistent like core does.
#10
Just to clarify this is not an issue with array vs node. This is an issue with the placement of the project_issue variables...which up until a month or so ago was consistent.
PIFT 1.x is running on d.o and uses the format...PIFT 2.x runs on d6.d.o and uses the same format...PIFT 2.x stopped working on d6.d.o..due to an update of project_issue that I can confirm locally.
As such it would seem that it is an issue with project_issue. I understand the annoyances with the node vs array as I have run into that as well, but this obviously used to work.
I don't care which way it ends up I just think it should be consistent.
Before node is saved...all the project issue properties are on the root of the node.
<?php$node->pid
?>
After saving they are prefixed as they used to be always.
<?php$node->project_issue['pid']
?>
I would like to leave this as critical since it will break d.o if deployed and is blocking 2.x since we can't test it on d6.d.o.
Please confirm...and possibly move to project_issue. Having to check array vs node and prefix vs no-prefix is simply nuts.
#11
Feel free to debug. If you find something inconsistent in project_issue, feel free to move back with a patch. I'm way too deep in other things now to work on this. Sorry.
#12
The form is designed differently then the hook_load() data structure.
<?php
/*
* Implement hook_load() for project issue nodes.
*/
function project_issue_load($node) {
$additions = db_fetch_array(db_query(db_rewrite_sql('SELECT pi.* FROM {project_issues} pi WHERE pi.nid = %d', 'pi'), $node->nid));
// TODO: This need to be ripped out in D6.
$additions['comment_form_location'] = variable_get('project_issue_comment_form_location', NULL);
$issue = new stdClass;
$issue->project_issue = $additions;
return $issue;
}
?>
Where as the form uses:
<?php$form['project_info'] = array(
);
$form['issue_info'] = array(
);
?>
Thus during the creation phase the form makes a flat array since '#tree' => TRUE is not used. It would seem the solution would be to use #tree and make hook_load() follow the format of the form, as every module should.
#13
No. We want hook_load() to load everything in
$node->project_issue[*]. The form needs to do different things based on the UI (fieldsets, etc). It's silly to encode the data in the $node based on one particular UI. See #98278: project* namespace bugs in $node. Going to call this 'by design'. You can work-around this.#14
Considering how much you (and lots of others) dislike core's inconsistency I am not sure why you would be ok with this, but whatever. Makes building on top of project_issue more fun then working with the node API to begin with.
#15
Ugh.
#16
#17
#18
I still don't agree with this, but that's life. Inconsistent data structures are one of the biggest annoyances in existence. I run into them with cck, core, etc. When you do it sucks...just as you are annoyed. The way the node API and form API works is set in stone for 6.x. The module should work with those APIs and use tree so that the data structure is consistent.
If you think it should work a different way then change in core (I understand that can be a pain in the...), but don't fight it by implementing in a different manor. Having two separates arrays seems to make sense for the same reason they are display separately. They represent two different sets of information.
#19
Committed.
#20
Confirmed: http://d6.drupal.org/node/367058.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.