Problem with order of precedence in file_scan_directory

Seeing that a beta version just came out, and thinking about the implications of this bug, I really want to make sure it has been addressed before Drupal 6 goes prime-time. If you need any further clarification, please let me know.

Thank you for looking into this. I tried your patch on my pre-beta copy of Drupal 6, and it didn't fix the problem. I then updated to Drupal6-beta1 and the patch still didn't work (the patch even broke Drupal when I reinstalled the database because the $file variable in the loop is an object now).

If this helps any, I'm running Drupal under Windows and Apache in PHP 5.2. When a directory is traversed using opendir(); and readdir();, they are being returned in alphabetical order for me.

I went back and tried to get this to work, and it did after making the following modifications to the file_scan_directory(); function in includes/file.inc:877 :

function file_scan_directory($dir, $mask, $nomask = array('.', '..', 'CVS'), $callback = 0, $recurse = TRUE, $key = 'filename', $min_depth = 0, $depth = 0) {
  $key = (in_array($key, array('filename', 'basename', 'name')) ? $key : 'filename');
  $files = array();

  if (is_dir($dir) && $handle = opendir($dir)) {
    while ($file = readdir($handle)) {
      //print $dir . '/' . $file . '<br />'; // Outputs the directory/file being looked at; gives output like I demonstrated in the initial bug report
      if (!in_array($file, $nomask) && $file[0] != '.') {
        if (is_dir("$dir/$file") && $recurse) {
          // Prevent array_merge(); from overwriting what we may already have
          $tmp = file_scan_directory("$dir/$file", $mask, $nomask, $callback, $recurse, $key, $min_depth, $depth + 1);
          foreach ($tmp as $curr) {
            // In going through each new file, only overwrite if it's closer to the base directory
            if (isset($files[$curr->$key]) && substr_count($curr->filename, '/') >= substr_count($files[$curr->$key]->filename, '/')) {
          	  continue;
		    }
		    $files[$curr->$key] = $curr;
		  }
        }
        elseif ($depth >= $min_depth && ereg($mask, $file)) {
          $filename = "$dir/$file";
          $basename = basename($file);
          $name = substr($basename, 0, strrpos($basename, '.'));
          // Same as before: only overwrite if it's closer to the base directory
          if (!empty($files[$$key]) && substr_count($filename, '/') >= substr_count($files[$$key]->filename, '/')) {
          	  continue;
		  }
          $files[$$key] = new stdClass();
          $files[$$key]->filename = $filename;
          $files[$$key]->basename = $basename;
          $files[$$key]->name = $name;
          if ($callback) {
            $callback($filename);
          }
        }
      }
    }

    closedir($handle);
  }

  return $files;
}

I've changed it to only overwrite the $files index if it is closer to the base directory. If two files are the same depth away from the base directory, the first one encountered will take priority. I'm not sure if this is the best way to do it, but it does fix the bug.

Hope that helps explain my problem a little better.

I just revisited the problem I was having, and realized that the real issue is that files from the recursive call to the file_scan_directory(); function were overwriting anything set in the calling file_scan_directory();. A simple change to line includes/file.inc:885 fixes the problem without the extra logic:

          $files = array_merge(file_scan_directory("$dir/$file", $mask, $nomask, $callback, $recurse, $key, $min_depth, $depth + 1), $files);

All I did was move the $files variable to be the second argument rather than the first. This basically does the same thing as my previous code posting without the added logic, giving priority to files found closer to the base directory or found first. The only problem with this is that if a file isn't found in the base directory, the first time it is found in a recursive call would take priority over something that might be closer (e.g. ./a/b/c/example.tpl.php would be chosen over ./z/example.tpl.php). I'm uncertain as to whether that would be a problem or not. The alternative would be to do the depth checking instead of the array_merge();.

Again, I'm still uncertain as to what other functionality might be impacted by this change, but it does fix the sub-theme problem I originally outlined.

Is anyone else able to confirm this as a bug?

Did the sub-theme directory's name come alphabetically after the letter P (name of the template file)? My problem is that:

my_theme/my_sub_theme/page.tpl.php
my_theme/page.tpl.php

would work fine because the base page.tpl.php would be the last found file. However:

my_theme/page.tpl.php
my_theme/sub_theme/page.tpl.php

would break, using the sub_theme's template file since it would be the last occurrence of page.tpl.php. Could you verify this and follow-up? I really appreciate this sanity check!

Also, just out of curiosity, what platform are you testing this on?

As an aside, make sure you clear your cache when you make the changes since the template files used are now being cached. I know I've forgotten to do this a couple times myself.

Try renaming your sub-theme directory to something like "zed" and see what you get. The key is that the sub-theme's directory name must come alphabetically AFTER the name of the template file. Both "gapland" and "arland" would be hit before page.tpl.php because they start with letters before the letter P, and thus there are no problems.

I'll see if I can't also reproduce this on a different server setup myself, but if you could try the above out, that would be great! If you can reproduce the problem, see if the solution in #4 fixes it. Thanks!

Oh, and if it helps any, I'm doing this under sites/all/themes. I'll test to see if just /themes makes any difference.

I'll see what I can do about creating a patch from the code in #4. As for patch #2, it ends up ordering file information AFTER the directories have already been traversed, which is a little late (plus it was was against a version of 6 before the beta).

I must say, I'm just glad someone else was able to reproduce this bug.

Attached is a patch that applies the fix mentioned in #4. This will favor a file at the base directory of the scan over one found in a subdirectory. For every matching file not in the base scan directory, the first file found will be used instead (remember that traversing a directory is done alphabetically).

I was able to confirm that this patch fixed my problem. Just make sure you flush the cache after the patch has been applied and it should work.

So if the Bot has passed the patch, does that mean it has been patched into head or that it's ready to commit?

In the off-chance that it's the latter, I'll update this issue to reflect such.

Well, I've tested this patch and it both works for me and fixes the bug. If anyone else can put eyes on this, that'd be appreciated!

I've added comments to explain what is happening with the file-matching code in this function. Nothing else has changed.

Changed the wording of the first comment a little and just copied the other comment suggestion you made.

Updating the status. Since there's no functional changes from when this was RTBC before the comments, this patch should do the trick.

Is there anything else that needs to be done for this to be committed? This is a really simple bug fix that will save themers grief down the road. The patch is in #22.