There is a d6 style node_load in the trigger module. See patch.

There is another in a comment in form.inc, I don't know whether that comment as a whole needs changing, or to just change that load to an EFQ.

$ grep -rHn "node_load(array" . --exclude-dir=sites
./includes/form.inc:4126: *   $node = node_load(array('uid' => $uid, 'type' => $type));
Files: 
CommentFileSizeAuthor
#9 1366716_1.patch2.12 KBgoogletorp
PASSED: [[SimpleTest]]: [MySQL] 39,379 pass(es).
[ View ]
trigger_node_load.patch529 bytesnaught101
PASSED: [[SimpleTest]]: [MySQL] 37,013 pass(es).
[ View ]

Comments

naught101’s picture

Status:Active» Needs review

whoops

xjm’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+Needs tests

Couple thoughts:

  1. Since #764558: Remove Trigger module from core is still NR, we need to do this in D8 first, no? See: _trigger_normalize_user_context()
  2. We definitely need a test here since the current code is invalid but nothing is failing. What's a scenario where this might cause a bug?
naught101’s picture

heh, don't ask me, I've never used the code, I didn't even try to find out what it's for. I just noticed it in passing :)

xjm’s picture

Version:8.x-dev» 7.x-dev

#764558: Remove Trigger module from core has been committed, so moving back to D7. :)

droplet’s picture

trigger_node_load.patch queued for re-testing.

catch’s picture

Priority:Major» Normal
Status:Needs review» Needs work

Looks like this runs if you assign a user action to a node context, or something.

Also that arg() stuff is pre-Drupal 6 code, this should just be menu_get_object().

While this is going to break horribly if you set it up like that, clearly no-one is actually running into this, and the workaround is "don't set up a trigger like that" , so I don't think we can call it major.

xjm’s picture

  1. Let's start with a test that creates a node, calls _trigger_normalize_user_context() with the appropriate arguments, and determines whether the correct data is returned.
  2. Then we can add the cleanup catch suggests in #6.
  3. Finally, we can try to find steps to reproduce a functional bug associated with this.
googletorp’s picture

Assigned:Unassigned» googletorp

I'll give this a stab.

googletorp’s picture

Assigned:googletorp» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 39,379 pass(es).
[ View ]

1. I added a test for testing _trigger_normalize_user_context(). I'm a bit unsure if I went about it the right way. Since the function looks that the active path and that always will be the patch job running the test - I overwrote $_GET['q'] - but I'm not sure if that is the correct way of doing it.

2. I cleaned up the code a but using menu_get_object() as suggested by catch.

3. I have taken a look at when _trigger_normalize_user_context is called - which only seems to be when the user logs in out etc. I have a hard time seeing when this could be an issue - since you need to be viewing a node - but I haven't really used the trigger module myself so I don't really now what the underlying functions are supposed to do.

YesCT’s picture

Status:Needs review» Needs work

since tests were added, removing needs tests tag.

Also, is a tests only patch appropriate here, to get the testbot to show the tests will fail without the fix? I think so, so needs work for that.