$node namespace woes -- can we fix this in D6?
hello world, i've had a series of bugs in the project* issue queues[1] caused by the fact that the project* modules use $node->pid just like "pid" always means in the project* codebase: the project id, or the nid of the project node that the (release|issue|...) belongs to. however, book.module thinks $node->pid is the nid of the parent page in the book hierarchy. :( so, all hell breaks loose when you try to put a release node into a book outline. certainly other contribs might use $node->pid to mean all sorts of things, too, which would conflict with both project* and book.module. i could always just hack project* to always prefix everything it tries to stuff into or read from $node with some kind of custom variable name prefix, like calling this "$node->project_issue_pid" instead of just "$node->pid". however, given that the API isn't quite frozen for D6 yet, i'm wondering if there's anything smarter and better we could do via the core API, instead of relying on convention and luck to avoid this problem... 1) what's the right way to handle variable namespaces in the $node object? can/should we change hook_nodeapi('load') to force people to use a namespace of some kind when populating $node? maybe $node should be a nested array instead of an object, so that everyone touches: $node[$module_name][$variable_name] instead of just: $node->variable_name ?? that probably sucks for all sorts of reasons, i'm just quickly brainstorming here... 2) presumably this has come up before? anyone have pointers to relevant old issues or threads? i don't remember seeing anything like this come up before, but i've been known to miss the occasional thread on this list. ;) 3) anyone else interested in making a little push to try to do this for D6? 4) would the D6 core maintainers be willing to commit a patch if it materialized and was properly reviewed/tested/RTBC'ed? thanks, -derek (dww) [1] for the interested reader: http://drupal.org/node/98278 http://drupal.org/node/124846 http://drupal.org/node/145782
i could always just hack project* to always prefix everything it tries to stuff into or read from $node with some kind of custom variable name prefix, like calling this "$node->project_issue_pid" instead of just "$node->pid".
thata the drupal convention, and IMO it works quite well. Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
On May 29, 2007, at 6:23 PM, Moshe Weitzman wrote:
thata the drupal convention, and IMO it works quite well.
undocumented conventions that aren't expressed in the code of core are worthless, IMHO.
Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
so, you advocate "luck" as the solution to this problem. ;) fair enough. however, if core doesn't do this itself, we have to mention this in the developer docs, preferably in a few places like the doxygen for nodeapi('load'), the drupal coding convetions, etc, etc. if that's really the only answer here, i'll volunteer to submit the issue to the documentation queue... thanks, -derek p.s. project* was written by core maintainers, and has been doing this wrong for over 5+ years. if we can't trust the core maintainer's own contribs to provide best practices for this, who can we trust? ;) hence the need for docs and vastly more awareness of this problem by the rest of us 2nd class citizens.
The standard Moshe is talking about is somewhat implied here at the bottom in the naming convention section: http://drupal.org/coding-standards It should probably be explicitly spelled-out though in terms of stuffing data into node objects and the like. I think putting it in doxygen is critical. I real api.drupal.org 100 times a day. Handbook maybe once a week. :) I (heart) standards. -josh
On May 29, 2007, at 6:23 PM, Moshe Weitzman wrote:
thata the drupal convention, and IMO it works quite well.
undocumented conventions that aren't expressed in the code of core are worthless, IMHO.
Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
so, you advocate "luck" as the solution to this problem. ;) fair enough. however, if core doesn't do this itself, we have to mention this in the developer docs, preferably in a few places like the doxygen for nodeapi('load'), the drupal coding convetions, etc, etc. if that's really the only answer here, i'll volunteer to submit the issue to the documentation queue...
thanks, -derek
p.s. project* was written by core maintainers, and has been doing this wrong for over 5+ years. if we can't trust the core maintainer's own contribs to provide best practices for this, who can we trust? ;) hence the need for docs and vastly more awareness of this problem by the rest of us 2nd class citizens.
Josh Koenig, Partner josh@chapterthreellc.com AIM: chap3josh 1-888-822-4273
If you're referring to this in the coding standards: "Global Variables - If you need to define global variables, their name should start with a single underscore followed by the module/theme name and another underscore." It never occurred to me to interpret this for database columns. I read an implied "php" before global variables, thus global $example; // bad global $_mymodule_example; // good I even added a rule to coder to check for this, but had to add exclusions for a couple dozen core modules that didn't follow this convention. Doug Green 904-583-3342 www.douggreenconsulting.com Bringing Ideas to Life with Software Artistry and Invention... Providing open source software political solutions -----Original Message----- From: development-bounces@drupal.org [mailto:development-bounces@drupal.org] On Behalf Of Josh Koenig Sent: Wednesday, May 30, 2007 2:49 AM To: development@drupal.org Subject: Re: [development] $node namespace woes -- can we fix this in D6? The standard Moshe is talking about is somewhat implied here at the bottom in the naming convention section: http://drupal.org/coding-standards It should probably be explicitly spelled-out though in terms of stuffing data into node objects and the like. I think putting it in doxygen is critical. I real api.drupal.org 100 times a day. Handbook maybe once a week. :) I (heart) standards. -josh
On May 29, 2007, at 6:23 PM, Moshe Weitzman wrote:
thata the drupal convention, and IMO it works quite well.
undocumented conventions that aren't expressed in the code of core are worthless, IMHO.
Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
so, you advocate "luck" as the solution to this problem. ;) fair enough. however, if core doesn't do this itself, we have to mention this in the developer docs, preferably in a few places like the doxygen for nodeapi('load'), the drupal coding convetions, etc, etc. if that's really the only answer here, i'll volunteer to submit the issue to the documentation queue...
thanks, -derek
p.s. project* was written by core maintainers, and has been doing this wrong for over 5+ years. if we can't trust the core maintainer's own contribs to provide best practices for this, who can we trust? ;) hence the need for docs and vastly more awareness of this problem by the rest of us 2nd class citizens.
Josh Koenig, Partner josh@chapterthreellc.com AIM: chap3josh 1-888-822-4273
Moshe Weitzman wrote:
i could always just hack project* to always prefix everything it tries to stuff into or read from $node with some kind of custom variable name prefix, like calling this "$node->project_issue_pid" instead of just "$node->pid".
thata the drupal convention, and IMO it works quite well. Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
I disagree. book.module should be no better than a contrib module. It should use book_parent, book_etc.
Earl Miles wrote:
Moshe Weitzman wrote:
i could always just hack project* to always prefix everything it tries to stuff into or read from $node with some kind of custom variable name prefix, like calling this "$node->project_issue_pid" instead of just "$node->pid".
thata the drupal convention, and IMO it works quite well. Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
I disagree. book.module should be no better than a contrib module. It should use book_parent, book_etc.
hey, don't disagree with me. i agree with you. book variables should be named more specifically, along with rest of core and contrib. and yes, this convention should be documented better. we're on the same team, folks. i should have added that if someone proposes a good alternative, i'm sure it will be considered carefully. thats also the drupal convention.
book variables should be named more specifically, along with rest of core and contrib. and yes, this convention should be documented better. we're on the same team, folks.
i should have added that if someone proposes a good alternative, i'm sure it will be considered carefully. thats also the drupal convention.
Now that we have the beginnings of a schema API in core, we likely should be looking for a naming convention that facilitates schema-aware data transactions. See the point "We change the way we current load properties into objects to reflect their data structure" in this issue: http://drupal.org/node/145684
Moshe Weitzman wrote:
Earl Miles wrote:
Moshe Weitzman wrote:
i could always just hack project* to always prefix everything it tries to stuff into or read from $node with some kind of custom variable name prefix, like calling this "$node->project_issue_pid" instead of just "$node->pid".
thata the drupal convention, and IMO it works quite well. Contrib modules are second class citizens and should always prefix their variables, form fields, node elements, etc. this system works very well, when you use it.
I disagree. book.module should be no better than a contrib module. It should use book_parent, book_etc.
hey, don't disagree with me. i agree with you. book variables should be named more specifically, along with rest of core and contrib. and yes, this convention should be documented better. we're on the same team, folks.
Oh, my mistake. I thought you were actually saying that book using $node->pid and $node->parent was a good thing. I think this is a relatively minor change, but I'm not sure if we want to do it ahead of the new book stuff that's being done. If it apepars the new book module won't make it in, I think we should make a patch to fix this.
On May 29, 2007, at 8:11 PM, Moshe Weitzman wrote:
book variables should be named more specifically, along with rest of core and contrib. and yes, this convention should be documented better.
untested/unconfirmed, but i think there must be a serious bug in core, since both path.module and book.module assume they own $node-
pid. :( furthermore, the best place to document this convention is in the code for core. that's how i learned drupal, and where i regularly look for best practice on how to do anything...
so, we have 2 options: 1) http://drupal.org/node/145684 -- we make a push for parts of nedjo's great ideas about Data API and at least tackle the $node object stuff for D6 that way. 2) i submit a separate issue for D6 that fixes our bugs by moving all of core's loading/reading of stuff out of $node into arrays of values based on the module doing the loading. e.g. $node->book['pid'] $node->path['pid'] ... gerhard does that with event.module, and that's what i'll be doing to project* [1], and it seems cleaner/better than prefixing everything ($node->book_pid). what's it going to be? this bug is (clearly) driving me insane, so i'll throw a few hours at fixing it today. i just don't want to waste the effort. can a core maintainer please comment on what's most likely to make it into D6?
we're on the same team, folks.
agreed. sorry if my tone implied otherwise. i'm having a really bad couple of weeks, and i've been letting that leak into my interactions at work and in drupalandia. :( my apologies. -derek (dww) [1] http://drupal.org/node/98278
The patched version of book moves almost everything into the $node->book_link array, and it wouldn't be too hard to have all the data there (only 2 other fields). -Peter On 5/31/07, Derek Wright <drupal@dwwright.net> wrote:
On May 29, 2007, at 8:11 PM, Moshe Weitzman wrote:
book variables should be named more specifically, along with rest of core and contrib. and yes, this convention should be documented better.
untested/unconfirmed, but i think there must be a serious bug in core, since both path.module and book.module assume they own $node-
pid. :( furthermore, the best place to document this convention is in the code for core. that's how i learned drupal, and where i regularly look for best practice on how to do anything...
so, we have 2 options:
1) http://drupal.org/node/145684 -- we make a push for parts of nedjo's great ideas about Data API and at least tackle the $node object stuff for D6 that way.
2) i submit a separate issue for D6 that fixes our bugs by moving all of core's loading/reading of stuff out of $node into arrays of values based on the module doing the loading. e.g.
$node->book['pid'] $node->path['pid'] ...
gerhard does that with event.module, and that's what i'll be doing to project* [1], and it seems cleaner/better than prefixing everything ($node->book_pid).
what's it going to be? this bug is (clearly) driving me insane, so i'll throw a few hours at fixing it today. i just don't want to waste the effort. can a core maintainer please comment on what's most likely to make it into D6?
we're on the same team, folks.
agreed. sorry if my tone implied otherwise. i'm having a really bad couple of weeks, and i've been letting that leak into my interactions at work and in drupalandia. :( my apologies.
-derek (dww)
On May 31, 2007, at 1:33 PM, Peter Wolanin wrote:
The patched version of book moves almost everything into the $node->book_link array, and it wouldn't be too hard to have all the data there (only 2 other fields).
right, i saw that and appreciate it. however, i'm talking about a more drastic, core-wide purge of directly touching $node, not just fixing up the collision with book and path. perhaps that's too ambitious for D6... -derek
On 31 May 2007, at 10:42 PM, Derek Wright wrote:
right, i saw that and appreciate it. however, i'm talking about a more drastic, core-wide purge of directly touching $node, not just fixing up the collision with book and path. perhaps that's too ambitious for D6...
Doesn't nodeapi still use the $additions array ? couldn't we get away with changing how it merges the data? function node_invoke_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) { $return = array(); foreach (module_implements('nodeapi') as $name) { $function = $name .'_nodeapi'; $result = $function($node, $op, $a3, $a4); if (isset($result) && is_array($result)) { // one line change here. Gets rid of an array_merge too. $return[$name] = $result; } else if (isset($result)) { $return[] = $result; } } return $return; } obviously,the big work is changing everywhere that accesses the node object. And that goes all the way down to the theme layer.
I like that idea a lot. I'd think that a lot of changes needing to be done in other modules related to this could be done with search and replaces couldn't they? Doesn't seem like it should be too hard. adrian rossouw wrote:
On 31 May 2007, at 10:42 PM, Derek Wright wrote:
right, i saw that and appreciate it. however, i'm talking about a more drastic, core-wide purge of directly touching $node, not just fixing up the collision with book and path. perhaps that's too ambitious for D6...
Doesn't nodeapi still use the $additions array ?
couldn't we get away with changing how it merges the data?
function node_invoke_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) { $return = array(); foreach (module_implements('nodeapi') as $name) { $function = $name .'_nodeapi'; $result = $function($node, $op, $a3, $a4); if (isset($result) && is_array($result)) { // one line change here. Gets rid of an array_merge too. $return[$name] = $result; } else if (isset($result)) { $return[] = $result; } } return $return; }
obviously,the big work is changing everywhere that accesses the node object. And that goes all the way down to the theme layer.
-- Sean Robertson Web Developer NGP Software, Inc. seanr@ngpsoftware.com (202) 686-9330 http://www.ngpsoftware.com
This would be a little awkward at times, but seems like a nice solution because it's enforced at the level of the node module. -Peter
On 31 May 2007, at 10:42 PM, Derek Wright wrote:
right, i saw that and appreciate it. however, i'm talking about a more drastic, core-wide purge of directly touching $node, not just fixing up the collision with book and path. perhaps that's too ambitious for D6... Doesn't nodeapi still use the $additions array ?
couldn't we get away with changing how it merges the data?
function node_invoke_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) { $return = array(); foreach (module_implements('nodeapi') as $name) { $function = $name .'_nodeapi'; $result = $function($node, $op, $a3, $a4); if (isset($result) && is_array($result)) { // one line change here. Gets rid of an array_merge too. $return[$name] = $result; } else if (isset($result)) { $return[] = $result; } } return $return; }
obviously,the big work is changing everywhere that accesses the node object. And that goes all the way down to the theme layer.
On May 31, 2007, at 1:53 PM, adrian rossouw wrote:
couldn't we get away with changing how it merges the data?
that's basically what i had in mind. and yeah, changing all the call sites (and therefore, breaking most patches in the queue) is the problem. ;) i had a scheme for avoiding the majority of the call-site changes (and therefore, patch brokenness) by letting the stuff from {node} and {node_revisions} continue to live at the root of $node (so everywhere that does $node->nid, $node->title, etc wouldn't have to change). however, that's broken by {node}.comment vs. comment.module's data. :( anyway, please see http://drupal.org/node/148420 for more details on this, and for the continuation of this thread. now that we've got another 4 weeks before freeze, there's actually a chance in hell of getting this done for D6... ;) thanks, -derek
On May 31, 2007, at 5:49 PM, Derek Wright wrote:
On May 31, 2007, at 1:53 PM, adrian rossouw wrote:
couldn't we get away with changing how it merges the data?
that's basically what i had in mind. and yeah, changing all the call sites (and therefore, breaking most patches in the queue) is the problem. ;)
i had a scheme for avoiding the majority of the call-site changes (and therefore, patch brokenness) by letting the stuff from {node} and {node_revisions} continue to live at the root of $node (so everywhere that does $node->nid, $node->title, etc wouldn't have to change). however, that's broken by {node}.comment vs. comment.module's data. :(
anyway, please see http://drupal.org/node/148420 for more details on this, and for the continuation of this thread.
now that we've got another 4 weeks before freeze, there's actually a chance in hell of getting this done for D6... ;)
This is very closely related to the problems we were facing with the Node Rendering patch -- building structured data for a renderable node is downright hazardous when so much stuff is dumped at the top level of the node object... There might be an opportunity for synergy here. --Jeff
Quoting Derek Wright <drupal@dwwright.net>:
On May 31, 2007, at 1:33 PM, Peter Wolanin wrote:
The patched version of book moves almost everything into the $node->book_link array, and it wouldn't be too hard to have all the data there (only 2 other fields).
right, i saw that and appreciate it. however, i'm talking about a more drastic, core-wide purge of directly touching $node, not just fixing up the collision with book and path. perhaps that's too ambitious for D6...
Something like an empty object of the module name created by an empty class named Namespace might suffice? $node->myModule = new Namespace; $node->myModule->myVariable = $foo; Earnie
This problem with book can be solved for D6: http://drupal.org/node/146425 -Peter On 5/29/07, Earl Miles <merlin@logrus.com> wrote:
I disagree. book.module should be no better than a contrib module. It should use book_parent, book_etc.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Derek Wright schrieb:
1) what's the right way to handle variable namespaces in the $node object? can/should we change hook_nodeapi('load') to force people to use a namespace of some kind when populating $node? maybe $node should be a nested array instead of an object, so that everyone touches:
$node[$module_name][$variable_name]
instead of just:
$node->variable_name
I've gone this route in my rewrite of event.module. Everything is now in $node->event which is an array. Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGXTBzfg6TFvELooQRAm14AJ96xKg96l3V+pmPk1JqlkslcPh0/gCgx4So /zwn8ZGMKAzgN9K1LIe9uXk= =r+kU -----END PGP SIGNATURE-----
participants (12)
-
adrian rossouw -
Derek Wright -
Doug Green -
Earl Miles -
Earnie Boyd -
Gerhard Killesreiter -
Jeff Eaton -
Josh Koenig -
Moshe Weitzman -
Nedjo Rogers -
Peter Wolanin -
Sean Robertson