Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

do NOT ignore "." dirs OR ignore "." dirs and all children #2581

Merged
merged 1 commit into from
Oct 23, 2019
Merged

do NOT ignore "." dirs OR ignore "." dirs and all children #2581

merged 1 commit into from
Oct 23, 2019

Conversation

keithbentrup
Copy link
Contributor

If you ignore any "files" beginning with "." including directories, then the all() method will exclude .somedir, but not .somedir/somefile. Subsequently, when trying to copy all files returned from all(), it will fail when the copy() method tries to copy a file into a directory that has not yet been created because .somedir was omitted from the returned array of all().

I found this bug when trying to install the admin plugin and ./tmp was a mount and thus rename() failed and self:copy() was invoked instead (see lines 364-5).

Alternatively, the check at line 238 could be omitted altogether and all "." files and dirs would be copied. (I'm not familiar enough with the project to know why they were ignored in the first place.)

Also, ignoring "." dirs and ALL their children is an option.

If you ignore any "files" beginning with "." including directories, then the all() method will exclude .somedir, but not .somedir/somefile. Subsequently, when trying to copy all files returned from all(), it will fail when the method tries to copy a file into a directory that has not yet been created because .somedir was omitted from the return array of all().
I found this bug when trying to install the admin plugin and ./tmp was a mount and thus rename() failed and self:copy() was invoked instead (line 365).
@rhukster
Copy link
Member

rhukster commented Aug 13, 2019

The intention is to ignore any files and folders (with their children). For example, if you had a .node_modules/ folder that contains lots of subfolders and files.

It's not clear to me how this is failing in your use case, it should ignore both files and folders already?

@keithbentrup
Copy link
Contributor Author

keithbentrup commented Aug 14, 2019

I understand it's a confusing problem. You'll see it though if you can hit line 365 with a debugger and then go into the self::copy() and then the all() method.

You would only hit that line though if your @rename failed on 356 because (for example) you were working with a mounted filesystem (see comment on line 364).

Anyway, once you're in all() at ln 238, this directory structure of the admin plugin will cause grav to fail with an error

.idea/
  misc.xml
  utils.iml
  ...

because strpos($file->getFilename(), '.') === 0 is true for .idea but not on the subsequent iteration with misc.xml. The loop will continue for .idea and not be added to the array. However, for the second file misc.xml, it will be added to the array, which will be returned BUT will ultimately cause a failure when copy() tries to create a file in a dir that does not exist because .idea was continued over and thus was not created by line 308.

I assumed the intention might have been to ignore presumably unnecessary files, but without knowing every plugin author everywhere would always follow a convention of "." files being unnecessary, then it's a big leap to just blanket ignore them and assume everything will still work.

Regardless, plugins (like the admin) are including dirs like ".idea" which is causing a failure when @rename fails, so it seems the choice is to either improve the file checks or drop the don't copy "." files/dirs logic altogether.

@rhukster
Copy link
Member

I'll take a look thanks.

@mahagr
Copy link
Member

mahagr commented Aug 19, 2019

I think the issue is with the recursive iterator use; all the folders are being read before the results get filtered.

@rhukster rhukster merged commit 8678f22 into getgrav:develop Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants