Drop function_exists for 'callback' functions

I think it's definitely a safe assumption to ignore function_exists(). However marginal the performance improvement, seems worthwhile. Philosophically, I agree that if a developer makes a mistake, it should be immediately obvious. A PHP error showing a improperly named function would certainly fit the bill - silent failures are bad for the developer and the end user.

I support the idea, but have not reviewed the patch.

Any idea on what the performance gain is? It is not clear how important of an improvement this is. Would be great to see some benchmarking.

In the theme.inc example, is there a valid reason for the function not to exist?

I don't think there's going to be much gain here if any, especially since many places here are rarely called. However I support the idea of removing unnecessary checks since it's a bad pattern that people will copy elsewhere - same as our constant file_exists() checks all over the place.

yeah, all except for theme.inc look safe.

Marking #994992: _menu_check_access() does not warn when the access callback does not exist as a duplicate.

I've attached the original patch, except the changes in theme.inc (which I think are necessary).

I think this is generally a good idea, not for performance, but so that other issues (like the one pillarsdotnet linked to above) aren't swept under the rug. This, for example, would catch the case of your callback not being called because of a slight typographical error.

Yes, this patch is a good idea - even if it doesn't wind up helping performance.

I ran into an example of this working on a Views patch (#1104860: Breadcrumbs are broken for clone/analyze/export/etc.). I had a situation where the title callback function for some menu items didn't exist (because it was in a file that wasn't loaded). If I had gotten an error message I would have figured out what was wrong in about 10 seconds... Instead, Drupal silently ignored it and just wound up not giving the menu items a title at all (so that they resulted in empty entries being printed in the page's breadcrumb), which is nonsensical and was a whole lot harder to track down.

I'm still in support of this patch so I'd love to see someone revisit this patch -- and maybe do some benchmarking too.

yessir!

This change is going to bite us in the a**, just like the registry has led to unrecoverable (and actually unresolvable) states already.

Of course, the function_exists() checks are entirely superfluous on production sites on which nothing is going to change. However, in all other scenarios, totally not limited to but especially staging, module updates, etc, these checks are useful and actually required in order to have Drupal not die.

Hence, I'd recommend, instead of removing the conditions, turn them into fast OR conditions, so production sites can get the performance they deserve, and everything else works like now:

  if (variable_get('module_list_static', 0) || function_exists($module . '_' . $foo)) {
    ...
  }

Alternatively, if variable_get() is too slow and inflexible, use a global:

  if (!empty($GLOBALS['drupal_module_list_static']) || function_exists($module . '_' . $foo)) {
    ...
  }

I agree that silent failure is a very bad thing for all involved. I don't expect any notable performance difference either way.

sun, why is it going to bite us? The registry can, I admit, lead to states where it thinks a class exists but it doesn't, or vice versa, and that's hard to recover from. We have no function registry, though, so I don't see where this would blow up on us. Have you an example?

If there is a danger of unrecoverable explosion here, then we shouldn't abandon this patch. Instead, the next step would be to replace:

if (function_exists($func)) {
  $func();
}

with:

if (function_exists($func)) {
  $func();
}
else {
  trigger_error(...); // Some E_USER_WARNING level thing.
}

That would avoid explosion but still give us a nice obvious error to aid debugging. I would wait to see if we can find a deadlock condition, though, before we go that route.

A rudimentary performance benchmark:

function make_seed()
{
  list($usec, $sec) = explode(' ', microtime());
  return (float) $sec + ((float) $usec * 100000);
}
mt_srand(make_seed());

//-- Pre-generate a lot of random function names
//-- to avoid 
//-- 1. function existance check caching by using same function name all the time and
//-- 2. losing time during benchmarking on function name generation.
$random_names = array();
for ($i=0; $i<1000; $i++) {
	$randomval = mt_rand();
	$random_names[] = md5(microtime(TRUE) + $randomval);
}

$start = microtime (TRUE);
for ($i=0; $i<1000; $i++) {
	function_exists($random_names[$i]);
}
$elapsed = microtime(TRUE) - $start;
print("Elapsed time: $elapsed seconds\n");

which returns: "Elapsed time: 0.0025150775909424 seconds" i.e. 1,000 function_exists() calls only add up to 0.00251 of a second on a dual-core 2.13GHz Intel with 4GB RAM and ~ 0.00148 seconds when run on a 8-core 32GB RAM server.

So performance difference is indeed negligible.

That said, I do join my vote to the notion of: sweeping a non-existant callback under a rug being a bad pattern, and really like the solution by Crell in #12

I can't benchmark this in a way to show a meaningful difference. I created xdebug trace files on the Examiner dev server. On a single node, I see 1887 vs 1959 function_exists calls. On a list page, I count 792 vs 830. That's just impossible to benchmark (for the latter, there are close to 15K function calls, so we got rid of 0.2% function calls and not all function calls are equal speed and these 0.2% are quite fast) . However ... while wanting to test this patch there, I could not log in because our mongodb session implementation silently fails if the database disk is full and it took some time to figure out why login does not work any more. Ironic, isn't. It shows clearly: silent fails are the death. So I think this is good to go, yes.

Ops, cross post, didnt see the others. Let's wait for sun.

To clarify: the direct consequences of this patch are non-obvious. The testbot won't ever be able to catch or even test this kind of patch.

I'm definitely not interested in the module.inc or theme.inc changes, at all. I'm focusing on custom/dynamic and especially cached stuff; i.e., dynamic callbacks, #pre_render, #value_callbacks, anything. Now. Forget about the unimportant peripheral stuff. Forget about the cached stuff. Think about the stuff that relies on the cached stuff, or think about stuff that depends on the cached stuff. What I'm after is 3D or even 4D, both just simply natural horizons of Drupal.

Given that performance is not the goal for this issue, we have to focus on silent failure. Good goal. However, silent failure is not a problem until failure (death) becomes a problem. #996236: drupal_flush_all_caches() does not clear entity info cache clarifies this. If we think one-dimensional, we ignore the possible problems. Drupal is more than 1D, more than 2D, and sometimes even more than 3D. That's why aforementioned issue most likely cannot be resolved for D7, since our approach to "flushing" attempts to do two very different things at the same time: clearing + (re)building.

In order to be sure. In order to be sure that every single function, that we intend to call, actually exists, and in order to ensure that no whatever functionality depends on any function that may or may not depend on yet another function, which may or may not exist, we need to make sure that all data that we are trying to build relies on information that is not outdated and cannot rely on any information or function that may not exist. At any point in time. Regardless of whether we're doing updates, flushing caches, or executing regular runtime behavior.

On a personal remark, TBH, I never had any issues with missing or improperly defined functions or callbacks, in my entire Drupal development history. What attracted me to this issue was the performance improvement idea, but given the actual numbers, performance is no longer an argument here. Only the silent failure remains, but frankly, in my entire dev history, I immediately recognized that my particular function wasn't called, or, I may not even have noticed, but who on earth cares if I didn't expect more functions to be called, if there's no performance penalty involved either way?

sun is a bit vague here and I won't be much better off but let me try: you change code. Now if we fatal on a function non existing, then either because you removed something that was cached. Or when you try to rebuild only then we fatal out. That certainly can be a problem. I would strongly support #12.

call_user_func() and friends throw a warning 'must be a valid callback', not fatal errors, I don't see a reason not to use that here (except for not very useful call graphs, but that'd be the same problem if we introduced a wrapper around function_exists()/trigger_error() too).

This also points to the need for a 'fix my broken site' button like #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap.

Right, many of the functions in the patch are already using something like call_user_func()... and many of the others aren't ever cached (so wouldn't suffer from this problem anyway).

So the basic approach in the patch seems fine, but we'd just need to look more carefully at some of the individual cases.

A specific use-case for function_exists() for a callback: #1281732-4: Fatal error when taxonomy_options_list() tries to call an undefined callback function.

I'm pretty sure this will blow up, but we still have sufficient time to test and eventually roll it back; so let's do it.

Removing the Performance tag, as benchmarks already showed that there is no real gain.

Adding API clean-up tag instead, as the main idea is to not silently ignore the error when a registered/expected callback cannot be called.

A fatal error (or at least an exception) would be the proper response here. If a validation function does not exist, for example because a file is not loaded, you might end up submit a form that should not have been submitted.

tests pass, and DX is much better so RTBC

Works for me! Committed to 8.x. Thanks.

So is the idea that we're deferring the "trigger a user warning" vs. "throw a fatal error" discussion to another issue?

By not addressing it, the patch wound up naturally doing a little bit of both, but completely randomly... I guess either option is better than the previous behavior of failing silently, but it seems a little odd to have this be inconsistent with no particular rhyme or reason.

Should the patch in #22 should be backported to 7.x, or should an alternative approach be used?

Re-rolled for D7.

Hmm, I am not sure about backporting something like this. While it is better DX, it is reminiscent of #1067750: Let Field API fail in a tale-telling way on invalid $entity which made a lot of folks sites' explode and which we still get complaints and issues about eight months later.

No problem either way; was mainly looking for an administrative decision on whether a backport was the way to go, so I know how to proceed in #994992: _menu_check_access() does not warn when the access callback does not exist.

In a D7 site I am optimizing, 12% of the exclusive wall time is spent calling function_exists() from _theme_process_registry(). In total, there are almost 400k calls...

So I applied the backported D7 patch from #29 and did the same measurements. Unfortunately, the number of function_exists() calls is virtually unchanged. I wonder, is there anything wrong with the backport or is this not a solution for the problem I face?

The function_exists() calls from _theme_process_registry() are perfectly normal. This is caused by the theme registry being rebuilt is unrelated to this issue.

This patch will not be backported.