When calling with an absolute path, such as '/Users/joachim/bin/drupal_hooks/8', this function returns FALSE.
This is because the call to explode produces an array like this:
Array
(
[0] =>
[1] => Users
[2] => joachim
[3] => bin
[4] => drupal_hooks
)
When the directory path is then built up from the pieces, the first one will be an empty string.
For that matter, this looks wrong:
foreach ($components as $component) {
$recursive_path .= $component;
because the exploding on DIRECTORY_SEPARATOR means that none of the pieces contain DIRECTORY_SEPARATOR. Therefore, the joining will just smush the pieces together without slashes.
Comment | File | Size | Author |
---|---|---|---|
#56 | 2043771-56.patch | 1.46 KB | 20th |
#26 | d7backport-drupal-mkdir-absolute-2043771-26.patch | 3.55 KB | n3uronick |
#15 | drupal-mkdir-absolute-test-only.patch | 1.03 KB | jbrown |
#15 | drupal-mkdir-absolute.patch | 2.09 KB | jbrown |
#15 | interdiff.txt | 1.67 KB | jbrown |
Comments
Comment #1
joachim CreditAttribution: joachim commentedFix is very simple, but probably needs tests.
Comment #3
slashrsm CreditAttribution: slashrsm commented#1: 2043771.drupal.file-drupal_mkdir-recursive.patch queued for re-testing.
Comment #5
jbrown CreditAttribution: jbrown commentedThanks for finding this @joachim!
DIRECTORY_SEPARATOR is appended at the end of the loop:
How about this approach?
Comment #6
joachim CreditAttribution: joachim commentedOh of course -- my patch will fail for relative paths :(
Your approach makes more sense. Might need a bit more commenting to make it easier to understand what it's doing.
Shouldn't this be comparing DIRECTORY_SEPARATOR rather than assuming it's a / ?
I'm confused by this... is this because with an absolute path, the first item of the array is a ''?
Comment #7
jbrown CreditAttribution: jbrown commentedI actually want to get rid of DIRECTORY_SEPARATOR in this function: https://drupal.org/node/1068266#comment-7118718 but I put it back into the patch for consistency.
Yeah - if an absolute path is exploded then first string in the array empty and we need to ignore it.
Comment #8
slashrsm CreditAttribution: slashrsm commentedLooks good to me. Can we call this RTBC?
Comment #9
joachim CreditAttribution: joachim commentedLet's see if I can write a test for this...
Here's two patches, one with just a test, and one which is #7 plus the test.
Comment #10
slashrsm CreditAttribution: slashrsm commentedThis only creates one level directory. Shouldn't we also test recursive stuff (multi-level directories)?
Comment #11
jbrown CreditAttribution: jbrown commentedAdded an extra level to the directory created in the test.
Also, test descriptions are not suposed to be translated. The patch linked to in #7 fixes this.
Comment #12
slashrsm CreditAttribution: slashrsm commentedLooks good to me. Let's wait for testbot.
Comment #13
slashrsm CreditAttribution: slashrsm commentedIt looks good, it is green and it is fairly simple patch. I'll set this to RTBC. Anybody feel free to change that if any other objections appear.
Comment #14
alexpottNice find...
I think this comment should be a little more verbose and explain why $components[0] is == '' for absolute paths.
I think we should also test that the directory actually exists... not just that no errors where reported.
Comment #15
jbrown CreditAttribution: jbrown commentedComment #16
jbrown CreditAttribution: jbrown commentedComment #17
slashrsm CreditAttribution: slashrsm commentedCould we use assertFileExists() here?
https://api.drupal.org/api/drupal/core%21vendor%21phpunit%21phpunit%21PH...
Comment #18
jbrown CreditAttribution: jbrown commentedassertDirectoryPermissions() checks that it exists and also checks the perms. Surely that is better?
Comment #19
slashrsm CreditAttribution: slashrsm commentedHm... you're right. Patch looks fine to me then.
Comment #20
jbrown CreditAttribution: jbrown commentedRTBC?
Comment #21
slashrsm CreditAttribution: slashrsm commentedRTBC.
Comment #22
alexpottCommitted 420f38c and pushed to 8.x. Thanks!
Comment #24
guedressel CreditAttribution: guedressel commentedDrupal 7 is also affected.
Backport of the fix would be nice.
Comment #25
joachim CreditAttribution: joachim commentedComment #26
n3uronick CreditAttribution: n3uronick commentedI created a backport for this based on d8 function.
Comment #27
slashrsm CreditAttribution: slashrsm commentedComment #28
slashrsm CreditAttribution: slashrsm commentedTrailing whitespace.
Any downside if using drupal_chmod()?
Comment #31
20th CreditAttribution: 20th commentedI am pretty sure that D7 is not affected. Uploading a test.
Comment #34
20th CreditAttribution: 20th commentedFixing test.
Comment #35
20th CreditAttribution: 20th commentedAnd works as expected.
The behavior is obvious if you look at the actual code, and the check in the test is very simple, no need to even bother committing it to core. Changing to fixed.
Comment #36
joachim CreditAttribution: joachim commentedI think you mean RTBC -- 'fixed' means the patch has been committed.
Comment #37
20th CreditAttribution: 20th commentedNo, I mean exactly that, fixed.
This issue was originally for D8. The patch for D8 was committed and marked as "needs backport" for D7. But D7 is not affected, it can create directories recursively using absolute paths just fine. So there is nothing that needs to be done neither for D8, nor D7; hence, fixed.
Comment #38
alexpott@20th well if there's no test coverage then adding some always seems like a good idea. That said...
This shouldn't be using the temporary directory for testing. It should be using the site's files directory because using the temp directory means that it won't be cleaned up if this test somehow fails.
Comment #39
20th CreditAttribution: 20th commented@alexpott: got it. How about like this then.
Comment #41
20th CreditAttribution: 20th commentedStrange, that passes locally.
Comment #44
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI see you have a tesbot CI error which is the same as mine in #2837008: Move scheduler.test into /tests and move three test module into /tests/modules. I started getting this problem yesterday 17 Dec 23:00 GMT.
I've raised #2837015: CI problem with permission when testing 7.x patches but there has been no response yet.
Comment #45
20th CreditAttribution: 20th commentedThanks for looking in this problem, @jonathan1055!
Actually, I've had some failures because of "Permission denied" error even before Dec 16, but that might be completely unrelated. For example, check failure logs in #39. That patch passes all tests locally without problems.
Comment #46
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe test failures in #39 are not related. Those are due to your assertions which fail due to lack of permission for the user running the tests. The 'CI error' failures we are now getting are before the tests even start to run, it is a failure of permission when trying to apply the patch (I think). You get the grey 'CI error' against the patch, instead of the pink 'pass / fail' indicator.
Hopefully someone on the testbot team will see that issue and respond. Are you on IRC or do you have any other way to alert the team? It is a fairly important fault affecting all 7.x project patches (and even core patches)
Comment #47
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented#2837015: CI problem with permission when testing 7.x patches is fixed I so have requeued the patch.
Comment #49
20th CreditAttribution: 20th commentedThanks a lot, @jonathan1055!
Comment #50
20th CreditAttribution: 20th commentedComment #52
20th CreditAttribution: 20th commentedComment #54
20th CreditAttribution: 20th commentedComment #56
20th CreditAttribution: 20th commentedComment #58
MixologicOkay, I've fixed a couple things on the testbots permissions wise (they were kinda messy) and now realpath should not be returning FALSE.
OTOH, if you use realpath on the testbots, your going to get unexpected behavior due to the fact that they run out of a *symlink* directory called /checkout - realpath is going to get rid of that symlink and the directories are not going to match.
We run them via a symlink to simulate running drupal out of a subdirectory, However, this is less than ideal and we should probably build them in a subdirectory to begin with.
I've opened an issue here: #2838194: Run out of real subdirectory, not symlink
Comment #59
20th CreditAttribution: 20th commentedLooks like the behavior of realpath() is still the same and the test is still failing, so we are not quitre there yet. :)
@Mixologic, thank you very much for your help! But this issue is so minor that it is probably not worth investing any more time into it. I will keep an eye on #2838194: Run out of real subdirectory, not symlink and report there if realpath() starts behaving normally, or I find another way to test this.
For context:
To test recursive directory creation, I try to get the absolute path of
public://
directory usingrealpath()
but it returns FALSE instead. My theory is, this happens because the testbot user does not have enough permissions on some of the directories. This is a documented behavior of therealpath()
function: