Currently, module_implements and module_invoke_all both check if a specific function exists with drupal_function_exists/function_exist. Altleast when the hooks are requested the first time. On the other side, drupal_alter() doesn't do such a check, which means that it might call a function that is not yet loaded or not available anymore.

Attached is a simple patch that removes the check from module_implements and adds a similar check to drupal_alter, so that it is checked once (and only once) in every case. I also changed the check in both cases to "if (function_exists($function) || drupal_function_exists($function)) {", so that drupal_function_exists is only called when necessary.

CommentFileSizeAuthor
#34 drupal-warn_on_missing_registry_file_instead_of_failing-412808-34-D7.patch785 bytesPieterDC
#32 drupal-registry_fix-412808-32.patch785 bytesscottalan
#29 _fix_registry.patch805 bytessun
#26 handling_missing_files_in_registry-412808-26.patch2.76 KBrupertj
#23 handling_missing_files_in_registry-412808-23.patch908 bytesrupertj
#12 registry_autoremove.patch3.52 KBAnonymous (not verified)
#7 412808.registry_autoremove.patch3.51 KBBerdir
#2 drupal.drupal_function_alter_behavior_2.patch2.08 KBBerdir
drupal.drupal_function_alter_behavior_1.patch1.91 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Needs work

True, there is an unecessary drupal_function_exists() in module_invoke_all(). module_implements(), by contract, loads "the files containing the implementations as necessary", that's why drupal_alter() doesn't have to call drupal_function_exists() itself.

Could you please reroll with just the code in module_invoke_all() modified?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
drupal.drupal_function_alter_behavior_2.patch2.08 KB

Updates, discussed with Damz in IRC.
- removed function_exists check in module_invoke_all() and drupal_alter
- re-added check to module_implements
- joined system table in module_implements() to only load enabled modules.
- throw an exception when a function can't be found by drupal_function_exists (wording and probably type of exception needs to be improved!)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

the registry only parses enabled modules, so we don't need to join on the system table.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review

@justinrandell: the idea is to allow site administrators to manually change the state of a module in the {system} table, without breaking the world.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

@damien: changing the state of a module in the system table should "break the world", or at least, have far reaching consequences.

can you explain the use case some more? certainly from the point of view of the registry, a change in the list of enabled modules should trigger a rebuild.

also, i'm not clear on how that relates to this patch.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Following use case:

A new module is installed on a site, which is broken. On the next request, it dies with a Fatal Error, because it contains a syntax error in hook_init. To be able to access the site again, the module needs to be disabled in the system table, because that's the only way to access the the site again. However, that does not update the registry.

Because that patch removes the (in normal cases) unecessary drupal_function_exists() call in module_invoke_all(), module_invoke_all() would try to invoke a non-existing function - again, fatal error.

Possible other ways:

- dynamically rebuild registry if a function does not exist - Problematic, might be a loop, takes time
- only check in module_invoke_all() and drupal_alter() - slight overhead if a hook is executed multiple times
- remove missing entries in $implementations[$hook], leave stale data in registry/cache - Might be problematic too

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
412808.registry_autoremove.patch3.51 KB

There is a similiar problem with _registry_check_code() which unconditionally includes a file. My above usecase actually dies with a fatal error inside _registry_check_code() which doesn't test if a specific file exists before it tries to include it.

Also, this patch does have a big problem:
It is not possible to throw exceptions in these functions, because it is for example not allowed to throw exceptions inside autoload functions (_registry_check_code() is called inside such a function). See http://ch2.php.net/__autoload. For some reason, it isn't even possible to do it in drupal_function_exists, it simply dies with the following fatal error:
Fatal error: Exception thrown without a stack frame in Unknown on line 0

I think the way to solve this is to remove missing functions and files from the registry, if they aren't found in the system. We could also only remove them from the cache but that would mean that they remain in the database for (maybe) no reason. DamZ didn't really liked the idea (ok, not at all ;)) but maybe I can convince him now that Exceptions don't work.

Attached is a patch which removes the data from the cache and the registry table if something goes wrong. I'm uploading this patch only for discussion, I haven't even run some tests on it. If we want include that stuff, we have to add tests for that anyway.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: drupal_function_exists gets sometimes called either twice or not at all » Handling of missing files and functions inside the registry

updating title..

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Critical

Subscribing - it's perfectly easy to install a hosed contrib module and everyone fixes that by updating the system table. Since we're potentially leaving site admins needing to do registry_rebuild() truncate cache_registry() or whatever other steps would be necessary to get this fixed manually, updating to critical. If the issue itself is won't fix, then we need a critical documentation issue to update the troubleshooting guide on how to get out of a hosed site.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

(Note that I don't yet know how the Job Queue works.. )

Now that we have a Job Queue system, could we maybe use that to safely rebuild the registry when something like this happens?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue tags: +Registry

tagging with Registry

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
registry_autoremove.patch3.52 KB

updated patch for HEAD, corrected some code style issues, no other code changes. review coming.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

This is also a pain if you're applying or removing patches which contain new hooks. Seems like you could run into cases where a module switches from hook_foo() to hook_bar(), you cvs update, and then get undefined function for the first hook.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Your point is actually a good example why we need to rebuild the registry and removing stale data is not enough. Because if only the old hook is removed, it would silently not work. But to do that properly, we need something like #251792: Implement a locking framework for long operations, imho.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

perhaps we need a blunt-instrument approach like this:

    case DRUPAL_BOOTSTRAP_DATABASE:
      // Initialize the database system. Note that the connection
      // won't be initialized until it is actually requested.
      require_once DRUPAL_ROOT . '/includes/database/database.inc';
      // Register autoload functions so that we can access classes and interfaces.
      spl_autoload_register('drupal_autoload_class');
      spl_autoload_register('drupal_autoload_interface');
      
      // If we have a dead site, admins can set this variable to force
      // a registry rebuild.
      if (variable_get('registry rebuild required', FALSE)) {
        require_once DRUPAL_ROOT . '/includes/registry.inc';
        _registry_rebuild();
        variable_set('registry rebuild required', TRUE);
      }
      break;
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

This issue seems to be duplicate of #332733: Implement a way to require that some functions are available. Please mark as such, if that turns out to be true.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Category: task » bug

Bumping to bug report for the hook renaming issue which I think is going to bite us very hard as soon as people start upgrading modules in D7.

casey’s picture

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

This issue is not relevant any more; #497118: Remove the registry (for functions).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Closed (won't fix) » Needs work

We still have the registry for classes.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

See also #605374: Dont add nonexisting files to the registry.

sun.core’s picture

sun.core CreditAttribution: sun.core commented
Priority: Critical » Normal

...but unless someone complains, this is not critical...

rupertj’s picture

rupertj CreditAttribution: rupertj commented
Assigned: Berdir » Unassigned
Status: Needs work » Needs review
FileSize
handling_missing_files_in_registry-412808-23.patch908 bytes

I frequently get this error when switching branches on the product I'm working on from a branch that has a module that stores a class in the registry to a branch that doesn't have that module. (Which is really annoying when working on a new feature and then going back to master for a bugfix...)

The attached patch alters _registry_check_code() to check if files actually exist before require_once'ing them, which stops the fatal error after removing the module.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

If we do file_exists() + require_once then we're not really requiring. Why not include_once which will throw a warning, but not fatal, and saves the extra file check?

rupertj’s picture

rupertj CreditAttribution: rupertj commented

That approach did cross my mind (after I'd rolled the patch, of course ;) ) I just assumed there was some reason for the require here. I'll make another patch with include_once instead.

rupertj’s picture

rupertj CreditAttribution: rupertj commented
FileSize
handling_missing_files_in_registry-412808-26.patch2.76 KB

Patch as described in #25. Also includes another require_once futher up the file, which I missed first time around.

hapydoyzer’s picture

hapydoyzer CreditAttribution: hapydoyzer commented

#26: handling_missing_files_in_registry-412808-26.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Moving to the 8.x queue and tagging for possible backport.

I like the changes of this page, there is going to be an error if a file is missing but it's not a unhelpful fatal error like it's right now. Will try the patch the next time I move a .test file around (that is the most common time I happen to end up with an inconsistent registry).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
_fix_registry.patch805 bytes

Attached patch is that simple fix.

At least for D8, I'd like to see this committed ASAP. It could be backported to D7.

However, this only prevents fatal errors/WSODs. I've the impression that #1372122: STOP the registry integrity constraint violation nightmare or some other patch introduced a massive regression, since files that no longer exist in the file system are not removed / cleaned up from the class registry AT ALL.

In turn, the registered but missing files are causing fatal errors (e.g., when Simpletest queries all existing .test files from the registry). The registered but missing files are kept forever, even after invoking drupal_flush_all_caches() and any other action that should normally cause a full registry rebuild and thus clean-up operation to happen.

With this patch, you can at least bootstrap Drupal and run tests (eyes wide shut on the tons of file inclusion warnings ;)).

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work

No registry in 8.x anymore :)

mpotter’s picture

mpotter CreditAttribution: mpotter commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Just because there isn't a registry in D8 doesn't mean this patch for D7 should have languished.

We have run into this issue recently when moving the Migrate module to a different directory. Migrate calls "class_exists" in a migrate_module_implements_alter() which runs really early in Bootstrap. Since the registry still points to the inc file that has moved, the require_once causes a fatal error and prevents anything being done via drush, including "drush registry-rebuild", or even the manual "registry_rebuild.php" script.

Using this patch fixes the issue and allows drush registry_rebuild to work properly. Really seems like a no-brainer to get this into D7.

scottalan’s picture

scottalan CreditAttribution: scottalan at Phase2 commented
FileSize
drupal-registry_fix-412808-32.patch785 bytes

Here's a patch for D7 in case anybody else needs it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: drupal-registry_fix-412808-32.patch, failed testing.

PieterDC’s picture

PieterDC
Primary language Dutch
Location Ghent
CreditAttribution: PieterDC at Randstad Digital for Government of Flanders commented
Status: Needs work » Needs review
Related issues: +#1081266: Avoid re-scanning module directory when a filename or a module is missing
FileSize
drupal-warn_on_missing_registry_file_instead_of_failing-412808-34-D7.patch785 bytes

I agree with @mpotter. We had almost the same use case, albeit with the Entity Cache module instead of Migrate. #2604054: Update from 7.x-1.2 to 7.x-1.5 => PHP Fatal error: require_once(): Failed opening required

There's a similar issue #1081266: Avoid re-scanning module directory when a filename or a module is missing almost going into 7.x while I type but it affects drupal_get_filename() instead of _registry_check_code() (indirectly drupal_autoload_*()) and tries to immediately search for the new location of the missing file.

Rerolling the latest patch against latest Drupal 7.x and setting to 'Needs review' to let the test bots have another go at it.

PieterDC’s picture

PieterDC
Primary language Dutch
Location Ghent
CreditAttribution: PieterDC at Randstad Digital for Government of Flanders commented
Status: Needs review » Reviewed & tested by the community

Putting back to 'Reviewed & tested by the community'.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Issue tags: +Pending Drupal 7 commit

RTBC + 1, approved by catch, sun and berdir and marked by mpotter is enough for me to convince me.

Marked for commit!

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Very nice fix, thanks!

  • Fabianx committed 84539bd on 7.x 
    Issue #412808 by Berdir, rupertj, beejeebus, sun, PieterDC, scottalan,...
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit +7.50 release notes

Committed and pushed to 7.x! Thanks @all for fixing this 7 years old issue!

Status: Fixed » Closed (fixed)

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