Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Dir::make() emits E_WARNING, if a file with the same name exists #3442

Closed
fabianmichael opened this issue Jun 17, 2021 · 5 comments
Closed
Assignees
Milestone

Comments

@fabianmichael
Copy link
Contributor

Describe the bug
The Kirby\Toolkit\Dir::make() has some sanity checks for preventing errors, e.g. checking the directory already exists and throws an exception, if it cannot be created. But if a file with the same name exists, where the directory should be created, these checks don’t work with the current implementation.

To Reproduce
Steps to reproduce the behavior:

  1. Enable debugging mode
  2. Create an example directory with a file in it: media/example/testfile
  3. Call Dir::make('media/example/testfile')
  4. The method will emit an E_WARNING level error (see https://www.php.net/manual/en/function.mkdir.php#refsect1-function.mkdir-errors)

Expected behavior
Other checks are throwing an Exception, so I guess as directory creation also fails in this case, it should also throw an Exception instead of emitting an error.

Kirby Version
3.5.6

Console output

Warning: mkdir(): File exists

Additional context
PHP 8.0.7 on some Linux server

@fabianmichael fabianmichael changed the title Dir::make() throws warning, if a file with the same name exists. Dir::make() throws warning, if a file with the same name exists Jun 17, 2021
@fabianmichael fabianmichael changed the title Dir::make() throws warning, if a file with the same name exists Dir::make() emits E_WARNING, if a file with the same name exists Jun 17, 2021
@distantnative
Copy link
Member

Adding a check like this should be sufficient, right?

 if (is_file($dir) === true) {
    throw new Exception(sprintf('A file with the name "%s" already exists', $dir));
 }

@distantnative distantnative self-assigned this Jun 19, 2021
@distantnative distantnative added this to the 3.5.7 milestone Jun 19, 2021
afbora added a commit that referenced this issue Jun 21, 2021
@lukasbestle lukasbestle assigned afbora and unassigned distantnative Jun 21, 2021
@bastianallgeier
Copy link
Member

@fabianmichael
Copy link
Contributor Author

@distantnative @bastianallgeier I think the check is fine, I just hope that this has no notable performance implications. But as the method is not used too often on regular page loads, it should be fina this way.

@lukasbestle
Copy link
Member

@fabianmichael I think it will use PHP's stat cache, so the same path only needs to be queried once.

@bastianallgeier
Copy link
Member

I'm also really not worried about performance issues in this case. The check will improve error handling a ton though. I think it's great.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

No branches or pull requests

5 participants