There are several places in core where we use drupal_function_exists() not as way to verify that a function exists, but only as a way to lazy load those functions.

In several places, the return of that function is not even checked, such as in drupal_retrieve_form():

      drupal_function_exists($callback);

I suggest we implement a drupal_require(array('myfunction', 'myfunction2')) that will both lazy load the functions *and* die in horrible pain (by throwing an exception) if those functions are not available.

CommentFileSizeAuthor
#6 332733-drupal-require.patch2.27 KBdamien tournoud

Comments

Anonymous’s picture

nice idea, i'll try to get a patch up soon.

bjaspan’s picture

I disagree with this approach. Drupal needs to become *more* API driven, which means the number of API functions that might require calling drupal_require() before using them will increase; eventually, every other line in Drupal code will be drupal_require!

Take a look at how I'm working on this for #361683: Field API initial patch (which defines *lots* of API functions that are not always loaded). I use a script (generate-autoload.pl) that reads the .inc files and creates autoload wrappers for each function that requires it. Specifically, download the latest patch on that issue and look at:

field.autoload.inc (automatically generated)
field.attach.inc, field.info.inc (has @autoload directives in it)
generate-autoload.pl (the script)

I actually think the @autoload directive is the wrong approach. Instead, we should name functions that want autoload wrappers function al_funcname. Then the script can run over all .inc files and know which functions to generate autoloaders for (the ones whose names start with al_). It can parse the @defgroup/@ingroup directives in comments to put the autoloaders into the correct group for API documentation purposes.

The only problem with this approach, and with drupal_require()/drupal_function_exists(), is that the code registry does not work during installation.

damien tournoud’s picture

Oh my. That's way ugly. Two levels of indirection? Come on.

bjaspan’s picture

It's one level of indirection and two extra function calls, one more call than drupal_require() but less ugly for the developer.

Here's the drupal_require approach:

  drupal_require('foo_func');
  foo_func(...);

One call to drual_require(), one call to foo_func(). Here's the autoloader approach:

  foo_func(...);

Which code would you rather read? In some other file that most developers never see, we have:

function foo_func(...) {
  drupal_require('al_foo_func');
  al_foo_func(...);
}
bjaspan’s picture

I changed my generated autoloaders not to use the code registry, making them faster and allowing them to work during installation. The new functions look like:

function field_attach_form($obj_type, $object, &$form, $form_state) {
  require_once DRUPAL_ROOT . '/modules/field/field.attach.inc';
  return _field_attach_form($obj_type, $object, $form, $form_state);
}

I haven't made the change yet to use a function name prefix like al_field_attach_form to identify which functions should be autoloaded; that will come next and will make the system easier to use for developers.

damien tournoud’s picture

Status: Active » Needs review
StatusFileSize
new2.27 KB

I still believe this is a good idea. Could also benefit to the field API in some places (but see also #445044: Try to remove the field autoload "feature").

berdir’s picture

Note that throwing exception does not always work. For example inside _autoload and exception_handler function functions. This results in *very, very* hard to debug messages like "Exception thrown without stacktrace".

Anonymous’s picture

Issue tags: +Registry

tagging with Registry

sun’s picture

#412808: Handling of missing files and functions inside the registry seems to be a duplicate of this issue.

During my work on #345118: Performance: Split .module files by moving hooks, I needed to write a custom rebuild.php to scratch the registry after moving more and more functions around. Something like that will probably not happen to the average Drupal user, but if you consider developers working on their modules and shifting functions around, it becomes quite a pain.

So I thought a bit about this (and found this nice issue):

I think we need to split drupal_function_exists() into two functions.

The first, perhaps simply f() instead of drupal_function_exists(), for loading _expected_ functions, i.e. those that are required. When f() can't load a function, the registry is most likely broken and we need to rebuild. For example: Module X consists of several include files. It knows that there should be a X_foo() function in an include file of itself or of a core module. If that cannot be loaded, we are in deep trouble.

The other one would try to load, but not rebuild if nothing is found. However, that is, what I think module_invoke*() really is for. Looking up arbitrary, optional functions in modules and silently fail if there are no implementations.

Because, currently, we try to do and handle both situations in one function. That's why we can't differ between critical failures and expected failures.

The second could re-use f() with an optional $optional = TRUE argument, so we have the same logic, but just don't bail out.

catch’s picture

Status: Needs review » Needs work

Splitting drupal_function_exists() into two sounds good - although we should be careful of adding lots of function calls there. Something like f() works for me too. It's a naming issue more than what drupal_function_exists() actually does that's the problem.

mattyoung’s picture

#9,

Please don't call it f() if you are going to introduce such function. Give it a good real name like drupal_load_function().

I get very confused with the way drupal_function_exists() is used: #523918: form.inc line 442, what's the point of this: drupal_function_exists($callback);

and the name "registry", it is so generic I couldn't tell what it does. Like I said in the issue, it should be called something like code_registry like theme_registry.

catch’s picture

Just a note that using drupal_function_exists() as a drop-in replacement for function_exists() in core introduces quite a lot of overhead - I've been filing patches for

if (function_exists($function) || drupal_function_exists($function)) {
}

in a few places (module_implements(), module_invoke_all() etc.) which cuts this down a bit. A rename of drupal_function_exists() to something like drupal_autoload_function() would make that pattern look a bit less silly though.

sun’s picture

Status: Needs work » Closed (won't fix)

Registry is gone.