Hi,

Modules in Drupal 6 should now use menu_get_object() to retrieve a loaded object based on the path. This only applies to certain types (e.g. node, user, menu, etc). The full list of types can be found at menu_get_object(). See http://drupal.org/node/114774#menu_get_object for more information.

The attached patch allows coder to check for lines like:

if (arg(0) == 'node' && is_numeric(arg(1) && ($node = node_load(arg(1))) {

and gives a warning suggesting using menu_get_object() instead. However, it doesn't currently warn about lines like:

if (arg(0) == 'node') {

as I was afraid it might give some false positives, i.e. in situations where the module is checking the path but doesn't actually attempt to load the object. This means we could miss some situations where the object is loaded on a subsequent line. We just need to choose which side we wish to err on.

Cheers,
Stella

CommentFileSizeAuthor
#3 coder_223908.patch1.94 KBstella
coder_menu_get_object.patch2.13 KBstella

Comments

stella’s picture

stella commented
Status: Active » Needs review
douggreen’s picture

douggreen commented

Why node just check for (user|node)_load\(arg\([0-9]\)\)?

stella’s picture

stella commented
StatusFileSize
new coder_223908.patch1.94 KB

Yes I don't see why that wouldn't work. I've recreated the patch, see attached.

Cheers,
Stella

douggreen’s picture

douggreen commented

That looks cleaner.

Often when I do this, I put arg(0) in a variable such as $arg or $arg0. Do you think this is common practice, and should we also check for variable names that begin with $arg?

stella’s picture

stella commented

We probably could check on $arg too, though there may be a couple of false positives. I don't know how widespread such $arg usage is, but I don't think you're the only one :)

Stella

douggreen’s picture

douggreen commented

let's avoid false positives

stella’s picture

stella commented

Ok, shall I check in the latest patch then?

douggreen’s picture

douggreen commented

I haven't tested it, but if it works for you, then yes please check it in.

stella’s picture

stella commented
Status: Needs review » Fixed

Checked in.

Cheers,
Stella

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.