Project loading could use the static cache

My guess is that this was added in an attempt to prevent a non-project nid or an invalid nid from being loaded, but given that FAPI validates the select box used for selecting the project, I don't think this is necessary. I haven't tested this, but the code looks fine and should be a simple fix, assuming we can't come up with a reason for why we need to specify the type of node to load. If there is a reason to have this code there, we might want to add a code comment in case this comes up again.

@earnie: No, my point was that the reason the current code passes 'nid' and 'type' as elements of an array to node_load() might have been because, at some time in the past, such extra validation, if you will, was necessary to make sure that node_load() was actually loading a valid project node.

I don't see how this would be necessary now, given that FAPI should validate form submissions such that at the point this code is executed the nid of the project that should be passed to node_load() will certainly be a project node, and thus Damien's patch should be committed.

Re commenting, my point was that if dww or hunmonk can think of/remember a reason why Damien's patch *shouldn't* be committed, then we should add a code comment explaining why the more obvious method of just passing the nid to node_load() isn't sufficient at this point.

So setting back to CNR so dww and/or hunmonk can respond.

Sadly, CVS archeology is failing to come up with a useful explanation here. :( The code in question comes from revision 1.92 of comment.inc. The CVS log only says:

revision 1.92
date: 2007-09-30 03:24:43 +0000;  author: thehunmonkgroup;  state: Exp;  lines: +12 -14;
more validation fixes for comment followups.

It was committed in the wake of #18920: make project_issue.module use comment.module., so it was definitely related to that, but there's no discussion in that issue about this code. The only history of this change seems to live entirely in hunmonk's brain. ;) Let's hope he remembers, and comments here ASAP.

That said, please note that for some (unholy) reason, we're inspecting $_POST directly, so the usual FAPI validation hasn't necessarily run. :( If we actually need to keep this code at all (I'd love to hear why), then we could enforce the same validation while still using the static node cache. First, we'd want to make sure that field is a number before we call node_load(). Then, after we get back the $project object, we'd check $project->type to make sure it's 'project_project'. That'd accomplish the same thing the existing code is trying to accomplish, without the huge performance hit.

I'm tempted to mark this CNW for the extra validation (and/or to rip out checking $_POST entirely), but sadly, I don't yet understand this code well enough to be sure, and would like to just hear what hunmonk has to say about it instead of investing more time to really dig into this.

yes, i remember exactly why i put that code in there. in the case where the project dropdown was changed to a different project, the node_load was still returning the project nid of the original project.

the details of how this occurred escape me, but i do recall trying my best to find a solution that didn't involve inspecting $_POST, and ended up falling back to that with the comment you see there. the array version of node_load() there was most likely put into place as an extra security measure.

the bottom line is there are a number of places in project_issue that are ugly b/c of the IFAC change, specifically because of core limitations. most of these should be able to be cleaned up in 6.x, esp. the FAPI-related ones.

i'm postponing this issue until 6.x. if somebody really wants to spend more time looking for a non-$_POST solution for 5.x, be my guest -- just be mindful of the thing i was trying to fix in the first place.

since we're trusting $_POST here for the pid, i want to keep the type filtering so we don't have any chance of getting a non-project node.

please consider this issue dead until 6.x, thx.

@hunmonk: we can ensure we don't get a non-project node as I described above -- just test $node->type after node_load() and before we use it. I don't see any reason to postpone this.