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.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | missing_file_death.patch | 2.12 KB | neclimdul |
| #2 | missing_file_death.patch | 778 bytes | neclimdul |
| missing_file_death.patch | 1.76 KB | neclimdul |
Comments
Comment #1
Crell commentedThe kittens set up a picket line outside my apartment. One thing at a time, please. :-)
Comment #2
neclimdul:( alternate version.
Comment #3
chx commentedFocus on a recover script instead.
Comment #4
neclimdul#534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap
Comment #5
neclimdulSo, 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.
Comment #6
Crell commentedHello bot.
Comment #8
Kjartan commentedWouldn't it be possible to have do something like:
I'm assuming none of the autoloaded files return FALSE on purpose / by accident.
Comment #9
pieterdc@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().