Upgrade drupal_alter()

- $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.

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

The last submitted patch failed testing.

HEAD was broken

The last submitted patch failed testing.

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. :-/

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.

@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 ;)

Thank you!

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

The last submitted patch failed testing.

- Using syntax chx proposed.

- Added tests.

The last submitted patch failed testing.

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

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.

Guys, we quickly need a decision here.

We need more people in here.

subscribe

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. :-)

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?

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

#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.

That idea doesn't apply to objects though.

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.

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.

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.

Forgot to attach script..

.

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 {

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.

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.

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.

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:

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.

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:

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:

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.

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);

To summarize, we have the following options:

Whoopsie. ;)

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

The last submitted patch failed testing.

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.

#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.

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).

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) ?

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.

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.

Looks great.

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

@bjaspan: "API clean-up". @see http://drupal.org/project/issues/search/drupal?status[]=Open&version[]=7.x&issue_tags=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.

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.

The last submitted patch failed testing.

#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.

catch was able to replicate the failure.

This one should pass.

Nice job.

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

Sorry.

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.

Subscribing, this is sort of holding up #216098: drupal_goto() does not follow same parameters as url().

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

Let's document this in the upgrade guide.

.

Update docs added.

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