We can update drupal_alter() for PHP5.

Test code (to put into index.php/debug.php):

$foo = array('foo' => 'bar');
$bar = new stdClass;
$bar->foo = 'bar';

echo "<pre>"; var_dump($foo, $bar); echo "</pre>\n";

function system_foo_alter($foo, $bar) {
  $foo['foo'] = 'baz';
  $foo['baz'] = TRUE;
  $bar->foo = 'baz';
  $bar->baz = TRUE;
}

drupal_alter('foo', $foo, $bar);

echo "<pre>"; var_dump($foo, $bar); echo "</pre>\n";

Result:

array(1) {
  ["foo"]=>
  string(3) "bar"
}
object(stdClass)#16 (1) {
  ["foo"]=>
  string(3) "bar"
}

array(1) {
  ["foo"]=>
  string(3) "bar"
}
object(stdClass)#16 (2) {
  ["foo"]=>
  string(3) "baz"
  ["baz"]=>
  bool(true)
}

Hence:

- $bar is altered by reference.

- $foo is not altered by reference, even when using &$foo in the function signature.

chx or anyone else to the rescue!

CommentFileSizeAuthor
#52 drupal.alter_.52.patch14.84 KBsun
Passed: 13818 passes, 0 fails, 0 exceptions View
#49 drupal.alter_.49.patch12.87 KBsun
Failed: Failed to install HEAD. View
#37 drupal.alter_.37.patch9.25 KBsun
Failed: Failed to install HEAD. View
#31 drupal_alter.png525.56 KBcatch
#26 micro.php_.txt2.19 KBcatch
#12 drupal.alter_.12.patch9.14 KBsun
Failed: Failed to install HEAD. View
#10 drupal.alter_.10.patch4.99 KBsun
Failed: Failed to install HEAD. View
#6 drupal.alter_.6.patch30.04 KBsun
Failed: Failed to apply patch. View
#2 drupal.alter_.2.patch29.72 KBsun
Failed: 13620 passes, 5 fails, 0 exceptions View
drupal.alter_.patch3.3 KBsun
Failed: 9236 passes, 117 fails, 51 exceptions View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Damien Tournoud’s picture

Status: Needs review » Needs work

- $bar is altered by reference.

$bar is an object. You never pass the object itself, but a reference to it.

- $foo is not altered by reference, even when using &$foo in the function signature.

There is no &$foo in the function signature.

sun’s picture

Status: Needs work » Needs review
FileSize
29.72 KB
Failed: 13620 passes, 5 fails, 0 exceptions View

I already mentioned that changing the function signature doesn't change the behavior with regard to arrays.

Anyway.

Here's a much cleaner approach that every single noob on this planet will understand.

- Removes some ugly code.

- Applies a consistent pattern.

- Saves us some cycles. (probably not much though)

- Also fixes a wrong usage of drupal_alter() in NodeAccessRecordsUnitTest

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review

HEAD was broken

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
30.04 KB
Failed: Failed to apply patch. View

This one fixes drupal_goto() tests.

I have no idea why session tests are failing (they also do for me locally), because all instances of drupal_alter() throughout core are converted now. :-/

chx’s picture

No, no, no. There is no need to do this. What you want and what PHP5 allows while PHP4 does not is &$foo = NULL and then call out based on func_num_args.

sun’s picture

@chx: Can you please explain that a bit more, post an example snippet, or just point to some docs on the net? I searched, but couldn't find any mentioning of the approach or trick you propose. (also no mention on http://www.phpwtf.org ;)

chx’s picture

function drupal_alter($type, &$data, &$arg1 = NULL, &$arg2 = NULL) 
  switch (func_num_args()) {
    case 2: $function($data); break;
    case 3: $function($data, $arg1); break;

sun’s picture

FileSize
4.99 KB
Failed: Failed to install HEAD. View

Thank you!

Given that core has one implementation that uses 5 arguments, a max of 7 looks reasonable.

drupal_alter('field_attach_view', $output, $obj_type, $object, $build_mode, $langcode);

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
9.14 KB
Failed: Failed to install HEAD. View

- Using syntax chx proposed.

- Added tests.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

I do prefer the syntax sun initially proposed, because it allows you to choose which parameter is alterable or not.

sun’s picture

FWIW, I'd also prefer #6 for the same reason. However, it's possible that performance gains of not using cufa() may outweigh this. (My machine is unsuitable to run benchmarks, sorry)

In the end, it only applies to non-objects anyway... and usually, drupal_alter() is not used for more than 7 arguments.... so, not sure whether the arguments really stand.

sun’s picture

Guys, we quickly need a decision here.

chx’s picture

We need more people in here.

bjaspan’s picture

subscribe

Crell’s picture

In previous benchmarking in other issues, skipping cufa() doesn't buy us all that much. I was trying to remove it from module_invoke_all(), I think, and we went up to 10 case statements. Given how slow cufa() is I was surprised at that, but that issue died because the benefit wasn't large enough to be worth the hideous code that it required. An alternate approach that passed in a lot of NULL values didn't fare any better (and also did crazy things to default value handling, because it borked all calls to func_num_args()). drupal_alter() is far less frequently called than module_invoke_all(), so I think avoiding cufa() here is an unnecessary micro-optimization.

Moving to drupal_alter($key, $array) instead of drupal_alter($key, ...) is inline with other such shifts we've made in that direction lately, including the DB layer. It also means that we get the functionality we want with vastly less code, which I also like. So I'm +1 to the approach in #6.

That said, "$data" should never be used as the name of a variable. :-)

chx’s picture

Hm, so it's not a DrupalWTF that even if you want to pass in a single argument which is the most common use case of drupal_alter you need to pass in an array(&$foo) instead of $foo? Seriously?

sun’s picture

Both approaches compared: http://drupalbin.com/11713

Dave Reid’s picture

#6 is my preferred approach. There are times I would not want things altered in the hook_alter implementations and it would be harmful to do so. Using #6 it's up to the caller to pass the arguments correctly once. The latest solution it's up to each and every single implementation to make sure they do it right. One is much easier than the other.

sun’s picture

That idea doesn't apply to objects though.

Damien Tournoud’s picture

That idea doesn't apply to objects though.

It does exactly the same: it's up to the caller to correctly clone the object it doesn't want to see altered.

catch’s picture

I did microbenchmarks of HEAD vs. sun's version vs. chx's version on the most commonly called use-case (in core, but seems likely it would apply to a lot of sites too) - a single argument to drupal_alter() with no actual implementations. Currently that's done by file_create_url() about eighteen times per request (mainly drupal_add_css()) - attached webgrind screenshot which just shows the current distribution of calls in vanilla HEAD.

What's important to point out here, is that we're not testing call_user_func_array() at all here - module_implements() is returning an empty array, but we get a 50% saving from the function calls to other things that are skipped in both sun's and chx's approaches (array_merge(), is_array() etc.)

100,000 iterations.

nothing: 0.0400650501251 seconds

function no_op(): 0.455811977386 seconds

drupal_alter() (HEAD): 4.44066381454 seconds

drupal_alter_sun(): 2.04327583313 seconds

drupal_alter_chx(): 2.15555810928 seconds

Attached script in case someone wants to play with it, would make sense to benchmark something with an actual implementation as well, but I there's no chance either approach would be slower than HEAD in any situation as far as I can see, and probably not much between them either.

I slightly prefer drupal_alter('hook_name', $thing_to_be_altered); over drupal_alter('hook_name', array(&$thing_to_be_altered)); because that's the most common case, but would be fine with either.

catch’s picture

FileSize
2.19 KB

Forgot to attach script..

mattyoung’s picture

.

bjaspan’s picture

This was a fast review so I may have missed the point.

I do not have the mastery of subtle PHP details that chx does, but I thought that & is for function parameter declarations, not function arguments.

I'm fine with the 7-arg approach in #12. I'm also happy with the cufa approach, because I don't really know the performance implications. If #12 is faster, then great.

Minor copy-and-paste error:

+/**
+ * Tests for URL generation functions.
+ */
+class DrupalAlterTestCase extends DrupalWebTestCase {

Crell’s picture

So it looks like performance- wise cufa() vs. switch is a wash, and in practice the difference from HEAD is minor either way as well.

Whether or not #6 is a DrupalWTF depends on which you consider worse: variable parameter count but no extra array(), or an extra array() but a fixed number of documented parameters on functions.

Personally I favor the latter, and Drupal has been trending that way. The new DB API, for instance, goes as far as requiring an associative array even for a single value. Being able to pass in an array also allows for, potentially, some funky dynamic logic in user space. since we don't have funky dynamic logic inside the API. Frankly I just dislike variable parameter count functions in general.

catch’s picture

So it looks like performance- wise cufa() vs. switch is a wash, and in practice the difference from HEAD is minor either way as well.

No, it doesn't look like that. I specifically said I ran microbenchmarks on a drupal_alter() with zero implementations - equivalent to hook_file_url_alter() in a clean HEAD currently. So those benchmarks cover everything except cufa() vs. $function.

If #572618: All theme functions should take a single argument to make preprocess sane and meaningful gets in, that'd swing things in favour of the single array argument here too.

catch’s picture

FileSize
525.56 KB

Here's the webgrind output I failed to post before - front page of vanilla HEAD with no content.

According to webgrind, drupal_alter() takes 2.27% of a page request internally (obviously this will vary widely depending on which page and which site). If we reduce that time by half, then we're looking at ~1% improvement with either of the proposed patches. It's not exciting, but cutting 50% out of one of the top 10 most expensive functions in HEAD isn't to be sniffed at either - there's a few patches around like that at the moment, and they all add up.

sun’s picture

One potential (major) issue I see is that (contrib) developers who are not so familiar with PHP references could forget the ampersands in their drupal_alter() invocations. Admittedly, those can be fixed easily, but still, could easily lead to many issues.

Brainstorming. How about a third approach?

Thus far, the most common use-case is drupal_alter($thing). The second most common use-case is drupal_alter($form, $form_state). We only have this weird switch statement, because we think that we need to support more arguments. In reality, we use a different pattern for "a variable amount of arguments" elsewhere in core already:

function drupal_alter($type, &$one, &$two, &$context) {
}

Whereby $two could already serve the purpose of $context when there is only one alterable argument. In fact, the function signature of drupal_alter() doesn't matter at all.

+++ modules/field/field.attach.inc	2 Oct 2009 18:22:54 -0000
@@ -1193,7 +1193,7 @@ function field_attach_view($obj_type, $o
   // Let other modules make changes after rendering the view.
-  drupal_alter('field_attach_view', $output, $obj_type, $object, $build_mode, $langcode);
+  drupal_alter('field_attach_view', array(&$output, $obj_type, $object, $build_mode, $langcode));

@@ -1228,7 +1228,7 @@ function field_attach_preprocess($obj_ty
   // Let other modules make changes to the $variables array.
-  drupal_alter('field_attach_preprocess', $variables, $obj_type, $object, $element);
+  drupal_alter('field_attach_preprocess', array(&$variables, $obj_type, $object, $element));

+++ modules/node/node.module	2 Oct 2009 18:22:54 -0000
@@ -2421,7 +2421,7 @@ function node_access_grants($op, $accoun
   // Allow modules to alter the assigned grants.
-  drupal_alter('node_grants', $grants, $account, $op);
+  drupal_alter('node_grants', array(&$grants, $account, $op));

+++ modules/system/system.module	2 Oct 2009 18:22:54 -0000
@@ -1893,7 +1893,7 @@ function _system_get_module_data() {
     // Invoke hook_system_info_alter() to give installed modules a chance to
     // modify the data in the .info files if necessary.
-    drupal_alter('system_info', $modules[$key]->info, $modules[$key], 'module');
+    drupal_alter('system_info', array(&$modules[$key]->info, $modules[$key], 'module'));

@@ -1986,7 +1986,7 @@ function _system_get_theme_data() {
       // Invoke hook_system_info_alter() to give installed modules a chance to
       // modify the data in the .info files if necessary.
-      drupal_alter('system_info', $themes[$key]->info, $themes[$key], 'theme');
+      drupal_alter('system_info', array(&$themes[$key]->info, $themes[$key], 'theme'));

These are the only places in core where we have more than 2 arguments.

So what if we "simply" rewrite those to pass a $context instead? I mean:

  $context = array(
    'obj_type' => $obj_type,
    'object' => $object,
    'build_mode' => $build_mode,
    'langcode' => $langcode,
  );
  drupal_alter('field_attach_view', $output, $context);

Note: $output not invented here. ;)

If you want to pass anything in $context by reference, you put it into $context by reference (as documented in the patch in #6). The maximum that drupal_alter() would support then would be:

  drupal_alter('form', $form, $form_state, $context);
catch’s picture

That seems like a good option. In the cufa() benchmarks done on another issue, the main slowdown from the switch pattern was the number of arguments passed and/or passing NULL arguments - so it might even be a touch faster too.

sun’s picture

It is even *very* debatable whether $form_state isn't a "context" already... If we say it is, then we'd have drupal_alter($type, &$data, &$context);

catch’s picture

To summarize, we have the following options:

// status quo.

// Status quo with a max of 7 arguments.

// One big array.
drupal_alter('foo', Array array(&$stuff);

// Two alterable arguments, then an array for context.
drupal_alter('foo', &$stuff, &$more_stuff_or_context, Array $context);

// One alterable argument, then an array for context
drupal_alter('foo', &$stuff, Array &$context);

Looking at those, I really like the last one.

sun’s picture

+++ includes/form.inc	1 Oct 2009 20:38:44 -0000
@@ -669,20 +669,11 @@ function drupal_prepare_form($form_id, &
+  // Invoke hook_form_alter() implementations.
+  drupal_alter('form', $form, $form_state, $form_id);

Whoopsie. ;)

sun’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
Failed: Failed to install HEAD. View

ok, this is what it would look like. We need to agree on the approach before changing all code to use $context.

Status: Needs review » Needs work

The last submitted patch failed testing.

Crell’s picture

Do we have any cases where we want to alter more than one thing in one shot? I can't think of any. Maybe $form_state...

drupal_alter($type, $thing_to_alter, $metadata) seems like a good pattern. Whether we want to include a 3rd param that is an "alter this or use it as context", I'm undecided. I guess I can go either way. The overall concept of a fixed parameter count and a keyed array to handle "Extended" information, however, I am +1 on.

Dave Reid’s picture

#320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks would want two things altered in hook_url_alter_outbound(): $path and $options. The idea of drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) is I think the best solution proposed so far. No cufa() or ugly switch's.

sun’s picture

And... yes, we need at least 3 arguments, because of hook_form_alter($form, $form_state, $form_id) - changing that would be a nightmare.

All 3 arguments to drupal_alter() are passed by reference, but you can pass a keyed array as $context and put non-alterable variables in there (as described in the Doxygen in the patch).

chx’s picture

I am in support of function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) if you find a better name for context1 and context2. More like context1 needs a better name , context2 should be just context or args. Actually then function drupal_alter($type, &$data, &$context = NULL, &$args = NULL) ?

Crell’s picture

I'm cool with chx's naming in #42. If we want to allow 3 primitives for simplicity, then we should not put a type hint on $args. Otherwise we should make it drupal_alter($type, &$data, &$context = NULL, Array &$args = NULL). I am comfortable either way.

sun’s picture

Type-hinting won't fly, again, because of hook_form_alter($form, $form_state, $form_id).

Seriously, I have troubles to think of better variable names. $context can be the second OR third parameter, whatever makes sense for your code.

And I already mentioned in IRC that those variable names are invisible. Neither the caller nor the hook implementations use them. So, to me this sounds like bikeshedding.

catch’s picture

function drupal_alter($type, &$data, &$context = NULL, &$args = NULL)

Looks great.

bjaspan’s picture

Just out of curiosity: What exception to code freeze does this fall under?

sun’s picture

sun’s picture

Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

sun’s picture

Status: Needs work » Needs review
FileSize
12.87 KB
Failed: Failed to install HEAD. View

Not sure why it failed to install HEAD.

This patch converts the only to drupal_alter() invocations in field.attach.inc to use a $context.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

#216098: drupal_goto() does not follow same parameters as url() is blocked by this issue. Please help - I don't know why testbot fails to install HEAD.

sun’s picture

Status: Needs work » Needs review
FileSize
14.84 KB
Passed: 13818 passes, 0 fails, 0 exceptions View

catch was able to replicate the failure.

This one should pass.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Nice job.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Lovely patch, gave it a final pass through and found nothing to complain about. Benchmarks from #25 still apply.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Sorry.

catch’s picture

Benefits of this patch:

drupal_alter() goes from a bit of a monster to 5 lines of easy to understand code.

We get a 50% internal performance improvement in one of Drupal's most expensive functions (top ten most expensive on a vanilla HEAD install).

This combined with module_implements() caching means adding an _alter() hook somewhere is more or less free.

While this patch only gives us at most a 1% saving in page execution time, it's in the critical path, so that's on every, single, Drupal request ever served. Also we don't know what drupal_alter() calls contrib modules will do, which could increase it to 3-5% in some situations.

There are no ab benchmarks on this issue because 1% of page execution time equates to about 0.75% in ab, and ab isn't designed to measure with that level of precision - since it's issuing http requests, not just the application.

kwinters’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks great, removes some ugly hacks, and is faster. Committed. Good job, sun, chx et al.

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs Documentation

Let's document this in the upgrade guide.

sun’s picture

Issue tags: -D7 API clean-up

.

Crell’s picture

Status: Needs work » Fixed
Issue tags: -Needs Documentation

Update docs added.

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -PHPWTF, -API clean-up

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