After a discussion on IRC we decided that sites blowing up when you move a file is not a very handy thing for Drupal to do. The attached patch lets the registry semi quietly fail when it can't include a file by using include_once instead of require once.

This patch looks to do more but it actually just shifts the function to include a more tightly grouped exit point instead of the two seperated requires and returns before so the only actual code change is the require_once/include_once swap.

I'm open to taking that out if its threatening the lives of kittens though.

Comments

Crell’s picture

Status: Needs review » Needs work

The kittens set up a picket line outside my apartment. One thing at a time, please. :-)

neclimdul’s picture

Status: Needs work » Needs review
StatusFileSize
new778 bytes

:( alternate version.

chx’s picture

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

Focus on a recover script instead.

neclimdul’s picture

StatusFileSize
new2.12 KB

So, documenting a little more discussion from IRC, part of the concern from chx is that this is babying possible bad code that could lead to a problem like #539220: Missing validation handlers are silently ignored by FAPI. The problem we run into is require fails utterly and include fails not enough. Seems like I've heard that somewhere before...

Anyways, one possibility is we fail a little more dramatically but controlled when the file doesn't exist by using file_exists to test it. We're adding a call to the filesystem here but it should be cached by the OS and not performance impacting or at least not significantly.

The "throw Exception();" implementation in the attached patch is obviously not significantly more helpful here but it outlines a path forward.

Also, I went back to the restructuring of the file discovery as in the original path so this logic is contained in a single place. I think that's important to this patch so not actually threatening kittens. I think it better to use the if/else structure rather than falling out with a return and semi-emulating a else in this case anyways.

I'm not going to personally push this out of won't fix since the concerns are valid but as should be evident I think this is a useful DX fix.

Crell’s picture

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

Hello bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

Kjartan’s picture

Wouldn't it be possible to have do something like:

$ret = include_once DRUPAL_ROOT . '/' . $file;
if ($ret === FALSE) {
// error, do something to fail gracefully. rebuild cache?
}

I'm assuming none of the autoloaded files return FALSE on purpose / by accident.

pieterdc’s picture

@chx, #2097189: Add a rebuild script is probably what you meant by a "recover script". And is probably also the "rebuild cache?" part @Kjartan talks about.

@neclimdul, #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap does describe a similar use case, but did not help in my case.
I'm going with #412808: Handling of missing files and functions inside the registry which holds exactly the same patch as the one from your 2nd comment; replacing require_once with include_once in _registry_check_code().

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.