Check for existing subdirectory returns false positive under specific conditions

Thank you for reporting and the detailed issue summary

Can you provide a test case showing the issue too

Thanks.

Hello @smustgrave I have attempted to create a test for this, however it depends on the write folder being at the top level of the webserver and the standard docker image for creating tests doesn't allow modifying that location.

I've independently just discovered this, and can confirm it also occurs in Drupal 10 (10.2.1)

I also came across this on Pantheon - is_dir('public://x/x/files') is always TRUE.

For me, this caused problems with generating image style derivative images - FileSystem::prepareDirectory() would not create the directory (because it appears to exist), and image generation would fail with the following message:

TypeError: imagejpeg(): Argument #2 ($file) must be a file name or a stream resource, bool given in imagejpeg() (line 258 of core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php).

The patch works for me for Drupal 9.5.

Last week during a Drupal Camp’s contribution day, I tried to learn how to write tests to try to get this to move forward. But I definitely had some issues getting the testing suite running locally. Maybe I’ll try again at some point when I have more time.

Based on your setup there’s some good documentation examples out there. Example I use ddev and phpstorm so this helped me

https://www.previousnext.com.au/blog/running-and-debugging-phpunit-tests...

Also if on Drupal community slack there is a channel #testing where someone may be of some help.

@Jandor64, I'm not sure if this could be considered a bug, since the documentation states this:

This directory must be relative to the Drupal installation directory and be accessible over the web.

/**
 * Public file path:
 *
 * A local file system path where public files will be stored. This directory
 * must exist and be writable by Drupal. This directory must be relative to
 * the Drupal installation directory and be accessible over the web.
 */
# $settings['file_public_path'] = 'sites/default/files';

But, since in #6 was said this was happening also in Pantheon (in which I doubt they would be using a root directory), I supposed that it would be happening also for non root directories, but I haven't been able to reproduce it.

I've created a test directory under my public files directory, which was the default sites/default/files, and I' ve made these tests, which are all correct:

$ drush php:eval 'var_dump(is_dir("public://test"));'
bool(true)
$ drush php:eval 'var_dump(is_dir("public://x/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://x/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/"));'
bool(true)

@vidorado, Pantheon does have /files as a root directory. It only happens for root directories.

Thanks @claw, I supposed wrong then!

So, per #12, I would either

1. Close this as "Works as designed".
2. Update the documentation in settings.php and FileSystemForm, and make the test that covers the fix.

I wouldn't be confident to say that the documentation is wrong, although some people are using root directories successfully (apart from the bug). So I would need some help on deciding :)

I'm going to add another wrinkle. For #13 I just confirmed the existence of /files/, but I didn't remember ever having to set that path in config. I had another look, and realised that that is because I never needed to.

Pantheon does have the files directory at /files/. It also creates a sites/default/files symlink, linking to /files.

So the file system path according to Drupal config is sites/default/files, which in the file system is /code/web/site/default/files, which is a symlink to /files.

realpath(), of course, resolves the symlink, exposing the issue.

That's a very valuable discovery, @claw! 🙂

Now I see that this can be an issue, even if the constraint of the path being relative isn't violated.

Thanks for your input, I will try to make a test that covers the fix.

I've managed to test it 🙂

I believe this test should be in a separate file due to the override of the stat() and realpath() methods, which are closely tied to this new specific test method.

What I'm not entirely sure about is whether this should be done as a regression test, like RegressionTest or JsonApiRegressionTest, or if the current naming and file placement are already correct.

vidorado → changed the visibility of the branch 11.x to hidden.

Can we update the IS to mention this only affects root directories (or symlinks to them)?

You're right @kim.pepper, I reflected that with a new title but not also in the IS :)

I've tried to explain it better in the IS.

I ran the test-only pipeline and it fails, but not where I expected it to. Any thoughts?

1) Drupal\KernelTests\Core\File\StreamWrapperCustomRootDirectoryTest::testRealPathHandlesCustomRootDirectory
TypeError: stat(): Argument #1 ($filename) must be of type string, false given

Whoops, that was an unchecked edge case. we must not call \stat() directly with the result of \realpath() because it could return FALSE.

I've added a guard for it.

Now it doesn't fail! Haha. In my local environment the test fails as expected when I undo the fix.

Failed asserting that directory "public://foo/custom-dir" does not exist.
/var/www/html/core/tests/Drupal/KernelTests/Core/File/StreamWrapperCustomRootDirectoryTest.php:19

Time: 00:01.228, Memory: 6.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\File\StreamWrapperCustomRootDirectoryTest::testRealPathHandlesCustomRootDirectory
Failed asserting that directory "public://foo/custom-dir" does not exist.

/var/www/html/core/tests/Drupal/KernelTests/Core/File/StreamWrapperCustomRootDirectoryTest.php:19

I will give it a look.

I can't figure out why the test-only changes test is passing. I've checked the job ouput and everything appears to be correct.

In my local environment it fails as expected when I manually revert the changes made on LocalStream.

I will leave this as "Needs review" so a reviewer can check this on their local environment.

Left comments on MR.