Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.So - another bug trophy to cwgordon7: in his testing and my mentoring for this GHOP task: http://drupal.org/node/201139
neither trigger_nodeapi NOR actions_do takes $node by reference. Thus, the $node is not modified
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | revert-by-ref-203846-18.patch | 2.06 KB | pwolanin |
| #14 | by-ref-203846-5.patch | 3.18 KB | jvandyk |
| #4 | by-ref-203846-4.patch | 1.98 KB | pwolanin |
| #2 | by-ref-203846-2.patch | 1.96 KB | pwolanin |
| #1 | by-ref-203846-1.patch | 716 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedstarting patch
Comment #2
pwolanin CreditAttribution: pwolanin commentedoops- didn't get it all in the patch
Comment #3
Gábor Hojtsy- Why was $object optional for actions_do()? Isn't it used as optional somewhere?
- We don't shorten names like $taxo
Comment #4
pwolanin CreditAttribution: pwolanin commentedI don't see any place where $object is omitted in the call to actions_do(). However, we should get Eaton or someone else who knows this code well to take a look at this problem.
new patch - fixes taxo naming
Comment #5
jvandyk CreditAttribution: jvandyk commented$object is optional for actions_do() because there is no requirement that an action act on an object. If you had an action that empties a certain directory, for example, it has no need of any parameters. You would call it by
actions_do('empty_my_directory');
Comment #6
Gábor HojtsyThis pushes the patch back to the drawing board.
Comment #7
pwolanin CreditAttribution: pwolanin commented@jvandyk - perhaps so, but then you can't pass be reference, AFAICT.
Since the actions module for Drupal 5 doesn't seem require PHP 5, how does it work?
Comment #8
jvandyk CreditAttribution: jvandyk commentedHere are our choices:
1. Apply the patch in #4. Pro: triggers/actions now works in PHP 4. I tested on PHP 4.4.7. Con: because parameters passed by reference cannot have default values, calling actions_do() gets messier. In my example above, we would now have to include a second parameter to actions_do() because $object would no longer be optional.
actions_do('empty_my_directory', $dummy);
2. Actions and triggers are PHP5-only, since PHP5 uses implicit call-by-reference.
Comment #9
jvandyk CreditAttribution: jvandyk commentedactions.module for Drupal 5 has a different architecture and does not apply here.
Comment #10
pwolanin CreditAttribution: pwolanin commentedWell - unless the Drupal 6 requirements will be PHP 5+ only, I think we need to fix this even if the code is a little messier.
Comment #11
Gábor HojtsyLet's add some code comments on actions_do() then about this.
Comment #12
chx CreditAttribution: chx commentedhttp://buytaert.net/php-is-dead-long-live-php
We can easily wrap actions.inc include in a php version check and trigger.info could get a php = 5.0 line. Thoughts?
Comment #13
Gábor Hojtsychx: I don't think this quote from Dries maps to Drupal 6's action feature.
Comment #14
jvandyk CreditAttribution: jvandyk commentedPatch with comments. Comment cleanup was necessary anyway because the comments still referenced the old-style function calls.
Comment #15
Gábor HojtsyThanks. As I have said, I don't think we should opt for a PHP 5 only feature here, so I committed what we can do for now. This is not looking ideal, so it give more fuel for the API cleanup that PHP 5 can get us in Drupal 7.
Comment #16
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #17
pwolanin CreditAttribution: pwolanin commentedthis patch should probably be reversed for 7.x since it works for PHP 5.2
Comment #18
pwolanin CreditAttribution: pwolanin commentedhere's a patch that selectively reverts the code changed for PHP4 support (does not revert the comment cleanup, for example).
Comment #19
Dries CreditAttribution: Dries commentedAs Drupal 7 will be PHP5.2+ only, this patch no longer needs to be applied -- does it?
Comment #20
pwolanin CreditAttribution: pwolanin commented@Dries - the latest patch is to partially roll-back the patch applied to 6.x. We had to patch 6.x to add back PHP 4.4 compatibility. For 7.x, this is not needed, so the last patch would move 7.x back to using simpler/cleaner PHP 5.2-compatible code.
Updating the title to clarify.
Comment #21
Dries CreditAttribution: Dries commentedAh, got it now. Thanks. Committed.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.