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));
CommentFileSizeAuthor
#9 1366716_1.patch2.12 KBgoogletorp
trigger_node_load.patch529 bytesnaught101
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naught101’s picture

naught101 CreditAttribution: naught101 commented
Status: Active » Needs review

whoops

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
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

naught101 CreditAttribution: naught101 commented

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

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 8.x-dev » 7.x-dev

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

droplet’s picture

droplet CreditAttribution: droplet commented

trigger_node_load.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
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

xjm
she/her
Primary language English
CreditAttribution: xjm commented
  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

googletorp CreditAttribution: googletorp commented
Assigned: Unassigned » googletorp

I'll give this a stab.

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Assigned: googletorp » Unassigned
Status: Needs work » Needs review
FileSize
1366716_1.patch2.12 KB

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

YesCT CreditAttribution: YesCT commented
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.