I have found a situation where menu.inc keeps trying to find the file path for the parent module see http://cgit.drupalcode.org/drupal/tree/includes/menu.inc?h=7.x#n3691
if (empty($item['file path']) && isset($item['module']) && isset($parent['module']) && $item['module'] != $parent['module']) {
$item['file path'] = drupal_get_path('module', $parent['module']);
}
I debugged this and found that drupal_get_path() will return Null every tme $parent['module'] is empty.
drupal_get_path() calls drupal_get_filename() passing the empty $name as parameter.
drupal_get_filename() does not check if the value of $name is empty which means it will do all the logic, including this:
// Fallback to searching the filesystem if the database could not find the
// file or the file returned by the database is not found.
if (!isset($files[$type][$name])) {
// We have a consistent directory naming: modules, themes...
$dir = $type . 's';
if ($type == 'theme_engine') {
$dir = 'themes/engines';
$extension = 'engine';
}
elseif ($type == 'theme') {
$extension = 'info';
}
else {
$extension = $type;
}
if (!isset($dirs[$dir][$extension])) {
$dirs[$dir][$extension] = TRUE;
if (!function_exists('drupal_system_listing')) {
require_once DRUPAL_ROOT . '/includes/common.inc';
}
// Scan the appropriate directories for all files with the requested
// extension, not just the file we are currently looking for. This
// prevents unnecessary scans from being repeated when this function is
// called more than once in the same page request.
$matches = drupal_system_listing("/^" . DRUPAL_PHP_FUNCTION_PATTERN . "\.$extension$/", $dir, 'name', 0);
foreach ($matches as $matched_name => $file) {
$files[$type][$matched_name] = $file->uri;
}
}
}
see http://cgit.drupalcode.org/drupal/tree/includes/bootstrap.inc?h=7.x#n876
I am proposing we add a check on the very top of drupal_get_filename() and return NULL when $name is empty.
This will fix the problem cause by menu.inc and also any problem caused by other functions that call drupal_get_filename() without $name.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 2383823-check_name_empty-33.patch | 512 bytes | tariqinam |
| #27 | 2383823-check_name_empty-26.patch | 573 bytes | joseph.olstad |
| #8 | 2383823-check_name_empty_d8_8.patch | 580 bytes | marcelovani |
Comments
Comment #1
marcelovaniComment #2
socialnicheguru commentedOMG. This was driving me crazy.
Is there anyway to print the name of the .info file to the log? so if name is not defined, just print something like ' thismodule.info does not specify a name'?
Comment #3
marcelovaniIt is not coming for a info file, it is happening because of some bad logic here http://cgit.drupalcode.org/drupal/tree/includes/menu.inc?h=7.x#n3691
Comment #4
extremal commentedThis patch worked for me. Thanks
Comment #5
mikeytown2 commentedWorks for me as well; RTBC.
Any chance we can fix the bad logic as well? Fixing it here as well makes sense due to contrib using this function via drupal_get_path().
Comment #6
marcelovaniI don't think we need to change drupal_get_path(). What really needs fixing is menu.inc, it is so bad that that piece of code on the top of this issue happens at least 3 times when I clear the caches. Too much in my opinion...
Comment #7
tstoecklerThis should be fixed in 8.0.x first.
Comment #8
marcelovaniPatch for D8 here, please review
Comment #9
sylus commentedLooks good for me in both 7.x and 8.x.x ^_^
Comment #10
alexpottThis feels the wrong fix - nothing should be calling drupal_get_filename with an empty $name
Comment #11
marcelovaniDefinitely menu.inc should not pass empty values to drupal_get_filename(). We should totally fix that, but this issue is about protecting against a performance issue if any module passes an empty value.
If that kind of behaviour happens in core, I guess it could happen in any contrib/custom module.
BTW, the problem is in D7. I do not know if there is a similar situation in D8. I have done the patch for D8 because they explicitly requested in #7.
Comment #12
mikeytown2 commented@alexpott
I agree but with drupal_get_filename() getting called by drupal_get_path() which then get's called by 119 different functions in core; I can totally see contrib calling it with a NULL, in fact core does it. This happens in core in a convoluted way: ExtensionInstallStorage::getAllFolders() is one such call that calls drupal_get_filename with $name = NULL. Offending code:
drupal_get_profile() returns NULL; InstallStorage::getComponentNames() calls InstallStorage::getComponentFolder() which then calls
drupal_get_path('profile', NULL). Patch for this fix is included in #1081266-137: Avoid re-scanning module directory when a filename or a module is missingComment #13
jeroen.b commentedWe removed the fix from the patch in #1081266 again and fixed some wrong calls in core (like the one in #12).
This should be fixed at the source of the function that calls drupal_get_filename, not in drupal_get_filename itself.
So perhaps it's better to focus on a fix in menu.inc here?
Comment #14
jeroen.b commentedWhen #1081266 is committed it will probably lead up to some errors. So adding as related.
Comment #15
sylus commentedAs per what @mikeytown said this can not be fixed at the source because we can't handle contrib calling this with a null. Can we please not ignore comments in #11 and #12. I really feel this should be marked as RTBC and am not as of yet seeing a cogent argument against not having this patch included.
Comment #16
jeroen.b commentedYes we can, open a issue at the project that calls it with null.
When https://www.drupal.org/node/1081266 is in core, calling drupal_get_filename with null will actually result in a php error/watchdog message so that this mistake won't be made again and is easier to recognize in current projects.
Comment #17
sylus commentedAh I didn't get from the earlier comments that #1081266 will flag this as an error. That alleviates my concern that developers will have to hunt down the offending implementation in contrib.
As long as we aren't opening additional patches to core to fix calling drupal_get_filename with null, and the issue above warns developers about this issue for contrib. This seems reasonable to me.
Comment #18
jeroen.b commentedAll issues in core that are calling drupal_get_filename with null are already fixed in #1081266 as it won't get past the tests with the
trigger_errorin the patch. See https://www.drupal.org/node/1081266#comment-9859317.That patch also does a static cache and a db cache for missing records so any calls with null will also be cached.
Comment #19
sylus commentedThanks for the clarification jeoren.b much obliged!
Comment #21
David_Rothstein commentedComment #22
jeroen.b commentedI'm wondering if we should port this patch. I'm also wondering if we should rollback this patch for D8.
Ignoring these messages is not the way to go. As in #2761829: Getting drupal_trigger_error_with_delayed_logging errors on Drupal 7.x dev (7.50) they point to an actual problem and they should be fixed, not ignored.
This could also be really annoying in development. Let's take the following situation:
You altered a menu item in hook_menu_alter() and changed the page callback of a menu item. You did not change the 'module' property and now when you visit the path, nothing is shown. It would be really annoying if Drupal wouldn't say anything about the module property being empty.
Comment #23
joseph.olstadpatch 1 no longer applies on 7.50 (7x-dev) we had been using this patch for over a year now on releases up to and including 7.44.
Comment #24
joseph.olstadHere is a reroll of patch 1 for 7.x-dev (release 7.50)
Comment #25
joseph.olstadfor backport, test patch 24 against 7.x , not 8.x
Comment #27
joseph.olstadsame patch as 24, but test against 7.x
Comment #29
David_Rothstein commentedNote that for Drupal 7 we are now hiding the warning message in this particular case (via #2762393: Skip error triggering for missing files if the files are empty or "default") - but did not prevent drupal_get_filename() from searching for the empty $name.
We should remove that code once we have a real fix here.. but the idea is that we don't want to flood lots and lots of sites with warnings for this when it's a known bug with an issue in progress for it here.
Comment #30
fabianx commentedComment #31
tariqinam commentedreroll of patch 1 for 7.x(release 7.52)
Comment #32
tariqinam commentedComment #33
tariqinam commentedComment #34
fabianx commentedSetting to Needs review, #33 looks good to me.
Comment #35
fabianx commentedI think we can even call this RTBC.
Comment #37
fabianx commentedRe-test, due to CI Error
Comment #38
swaps commented#33 Looks good .
Cheers
SwapS
Comment #39
stefan.r commentedComment #40
David_Rothstein commentedI think we should consider addressing #29 here. If I recall correctly it had a @todo pointing to this issue.
Comment #41
David_Rothstein commentedAlso has this issue been resolved for Drupal 8 yet?
Comment #42
stefan.r commentedComment #43
poker10 commentedThe D10 code does not seems to return early if
$extension_nameis empty. So probably it is still an issue in D10, but it looks like the caching here is better than in D7 (seeExtensionList::getPathname()in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...).But regarding the D7 patch, we definitely need to address #29 and the mentioned @todo / code block.