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

Joomla\Filesystem\File::delete method chmod issue #23504

Open
sven420 opened this issue Jan 10, 2019 · 57 comments
Open

Joomla\Filesystem\File::delete method chmod issue #23504

sven420 opened this issue Jan 10, 2019 · 57 comments

Comments

@sven420
Copy link

sven420 commented Jan 10, 2019

When Path::canChmod($file) returns false method throws an exception as if deletion is not possible but some servers have such setup that chmod is not but file deletion is possible.

Because of this I got errors when trying to update extensions but after commenting out this and line below that is trying to chmod file to 0777 update was success.

In my opinion unlink($file) should be executed even if chmod is not possible and this is a bug that can cause issues for some users.

@mbabker
Copy link
Contributor

mbabker commented Jan 10, 2019

In my opinion unlink($file) should be executed even if chmod is not possible and this is a bug that can cause issues for some users.

-1, the entire point of attempting to chmod the resource is to make sure it is writable as a delete action is a write action. Blindly executing chmod() without ensuring the resource is in a writable state or executing unlink() on a read only resource is also prone to errors, removing sanity checks is not a valid bug fix here.

The fact you are having issues with the chmod() function points to either PHP being too restricted (i.e. the function is in the disabled functions list), filesystem permissions oddities, or the umask being an invalid value causing chmod(0777) to put the file into a read-only state.

@sven420
Copy link
Author

sven420 commented Jan 10, 2019

I see your point but we are not always in the situation where we have control over PHP or server configuration and Joomla should work in those cases also.

I don't think chmod is good sanity check here because executing unlink in try catch block would produce exception in case file isn't writeable.

What if chmod passes but unlink fails for some reason, if chmod check is definite proof that file is writable another try catch block for unlink is not necessary.

I still think that unlink try catch should be executed even if chmod part fails, there is no reason not to try it in method named delete.

@mbabker
Copy link
Contributor

mbabker commented Jan 10, 2019

I still think that unlink try catch should be executed even if chmod part fails, there is no reason not to try it in method named delete.

If anything is going to change about the code, this should be the workflow. It should continue to be full of sanity checks, but potentially gets around whatever issue you might be running into. At the end of the day, the API MUST ensure it can actually work with the file given otherwise everyone's better off just hardcoding the use of unlink() into their code.

  • Check file actually exists, if not then return true (because it should never be considered an error to attempt to delete a non-existing filesystem resource, you wanted it gone and it's not there)
  • Check file is writable, if not then execute chmod($file, 0777); and fail if chmod() cannot be performed
  • Attempt to delete file via unlink(), if this fails and chmod() was executed attempt to restore to the previous state

@sven420
Copy link
Author

sven420 commented Jan 10, 2019

I agree this is the proper way to do this.

@rotech1
Copy link

rotech1 commented Jan 15, 2019

Recently have similar issues with an extension update ( and un-install, too! ) see https://forum.joomla.org/viewtopic.php?f=728&t=968897


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

@mbabker
Copy link
Contributor

mbabker commented Jan 15, 2019

For the record nothing significant has changed in Joomla\CMS\Filesystem\File (previously JFile) nor Joomla\Filesystem\File in quite some time. There were exactly zero changes in those classes between 3.9.0 and 3.9.1 (or for that fact any other parts of the code using methods from those classes) so if something has changed between those two releases it has nothing to do with the filesystem API.

Without a consistent environment to debug the root cause of the issue, at best all we're getting in this thread is "this code is blocking me from doing something" and that doesn't establish why it's preventing you from doing something.

@rotech1
Copy link

rotech1 commented Jan 15, 2019

It really looks like 3.9.2 has some kind of filesystem bug - see also https://www.kunena.org/forum/k5-1-support/156312-j3-9-2-breaks-attachment-uploads


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

@mbabker
Copy link
Contributor

mbabker commented Jan 15, 2019

You can't have a new bug in an API that had zero changes in 3 releases.

@rotech1
Copy link

rotech1 commented Jan 15, 2019

Well, some very file-system-like problems suddenly appear after the J3.9.2 update - what else could be causing these behaviors?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

@mbabker
Copy link
Contributor

mbabker commented Jan 15, 2019

Without an environment to recreate those issues I couldn't tell you. I can tell you though that the filesystem APIs have not been changed in quite some time (independently auditable; https://github.com/joomla/joomla-cms/commits/3.8.13/libraries/joomla/filesystem shows all changes to the libraries/joomla/filesystem folder from 3.8.13 and earlier and https://github.com/joomla/joomla-cms/commits/3.9.2/libraries/src/Filesystem for changes after the classes were namespaced during 3.9's development). So whatever issue you are seeing that points to an error coming from the filesystem API is a symptom of something else.

Again, the first problem is identifying an environment that can actually create the problem. Because if we are going strictly from my own hosting, Joomla's hosting, or my personal development space, I could say this is not a core bug and close it right now as a local environment issue and leave it to you to debug. If there is some sort of core issue though, it's better to troubleshoot and identify it instead of blindly making changes to files that have gone untouched in a substantial amount of time.

@mahagr
Copy link
Contributor

mahagr commented Jan 21, 2019

@mbabker FYI

We are also getting reports where updating/removing extensions fail in Joomla 3.9 but not in Joomla 3.8. I am trying to get more information about the environments as I have no idea what could cause it to happen.

PHP documentation says:

Note: When safe mode is enabled, PHP checks whether the files or directories you are about to operate on have the same UID (owner) as the script that is being executed. In addition, you cannot set the SUID, SGID and sticky bits.

Maybe safe mode is what causes all the issues? Because of the above paragraph in the docs, I don't believe that chmod() is a reliable way to figure out if the file can be deleted or not.

@mbabker
Copy link
Contributor

mbabker commented Jan 21, 2019

If someone wants to start blindly changing code by all means go for it. All this thread has is a bunch of "this is broken and needs fixing" comments but there is still no real issue identified, just a bunch of hypotheticals. That's still not a good way to troubleshoot and fix potential code issues in methods which have not changed but at this point I'm done arguing for proper debugging and troubleshooting of issues.

@mahagr
Copy link
Contributor

mahagr commented Jan 21, 2019

@mbabker I've never proposed blindly to change the code, I guess that we're all just trying to narrow down what could cause the issue to happen -- and why you won't get the same issue with older versions of Joomla.

I just received a response from one user and it looks like he had libraries folder with UID root, which is obviously a permission issue in his end. I have no idea how older Joomla versions do not fail in this case, maybe older Joomla releases just silently fail to update the files?

For a long time, I have suspected that Joomla has a bug in the installer, which sometimes causes files not to be updated even if installation succeeds without an error. Deleting the files manually and installing the same version again always helped before and I believe the same is true now.

Maybe someone finally found the issue and fixed it?

I will let you know if something new comes out on this.

@brianteeman
Copy link
Contributor

Safe mode was removed in php 5.4.0 so if that is the problem there is an obvious and easy answer

@rytechsites
Copy link

We manage over 100 joomla sites, we are on:
Php 7.0.7

We have found for SOME tools (not all)

  • joomlashack's tools (osmap)
  • docman

If we are on release 3.9.2, and we try to upgrade either of these, we end up getting file errors on the 'uninstall of files', it is as if the files don't uninstall properly.

If we manually install joomla 3.8.13, and attempt to run these upgrades everything works perfectly well.

Please note, on my 'privately hosted sites' on siteground I have NOT had this issue at all.

Our servers are 'local' at our university, and they are managed here 100%. Initially we tried to change file permissions, etc, but that didn't work.

Our best workaround was to go back to 3.8.13...

Is there anything else that I could give to help you see the recreation of this issue?

I'm guessing it is a combination of our file system, php and joomla 3.9.2...something...

thanks,
Laura

@billtomczak
Copy link

@rytechsites I can confirm that we've identified this as the source of problems we've been seeing with our stable of Joomlashack extensions. Updates to library extensions are handled differently in Joomla. First they are uninstalled, then installed. If the uninstall fails - as it does under certain specific conditions - the entire install/upgrade procedure fails leaving the filesystem and/or database corrupted. Untangling the mess is sometimes pretty painful.

So far we've found that it seems to become an issue when an installed site is moved from one server/host to another. But not always, not reliably.

We're in the process of redoing our whole build/install/deployment chain and it will include workarounds for this problem. In the meantime, all we've been able to do is respond to users when it rears its ugly little head.

I'm not entirely convinced it's a problem as such in Joomla code. Definitely some weird quirk of file permission reporting on some servers under some (unspecified/unknown) circumstances.

@rytechsites
Copy link

Thank you very much!!! I agree, I think it is a strange combination of 'things', because unfortunately it doesn't happen 'all of the time'.

If you need us to run any kind of testing in our environment, because THERE it does happen 100% of the time with specific extensions, please let me know.

thanks,
Laura

@rytechsites
Copy link

I just realized something... when we 'install our upgrades', we copy our files from our 'production environment' over to our 'development' environment...install all upgrades, then 'move everything back to production'...

which means we do move the files from one host to another...

-- Laura

@brianteeman
Copy link
Contributor

We're in the process of redoing our whole build/install/deployment chain and it will include workarounds for this problem. In the meantime, all we've been able to do is respond to users when it rears its ugly little head.

That's why problems never get fixed. People like yourself with the skills to fix it never do. You just keep it to yourselves.

And of course you don't even need to work around the problem because @mbabker already fixed it #23303

@mahagr
Copy link
Contributor

mahagr commented Jan 24, 2019

And of course you don't even need to work around the problem because @mbabker already fixed it #23303

Well, not quite true. The fix for the library installer is already present in J3.9.2 and it doesn't have any effect on this chmod issue. That said, I'm really glad to see the library installer fix!

But in this case, it is better to fail with a nasty error message than to silently ignore files which cannot be updated. It is far better to have the installer to fail than to end up having a wrong set of files.

@rytechsites If you copy files from one server to another, make sure that the file owner/group are correct for all files after doing that. The installer won't be able to copy/replace the files if the files are owned by one user while the installer (web server) runs as some other user. :) The only reason why chmod() would fail, is that the user doesn't have the permissions needed to do the operation.

@rytechsites
Copy link

@mahagr thank you for your notes. We will verify, but as far as I know our 'process' of moving the files from one server to another works as it should.

This was a complicated issue, and I appreciate all involved that are taking the time an anyway they can to assist.

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/23504

@ghost
Copy link

ghost commented Mar 4, 2019

closed as Issue seems resolved. If not please reopen.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

@jobst
Copy link

jobst commented Mar 28, 2019

Joomla Version: 3.9.4
PHP: 7.1.24
Gantry5 installed: 5.4.24
Gantry5 upgrading to: 5.4.28

I have the problem, too, but I know that one of your assumptions is incorrect and I will get to that.

If anything is going to change about the code, this should be the workflow. It should continue to be full of sanity checks, but potentially gets around whatever issue you might be running into. At the end of the day, the API MUST ensure it can actually work with the file given otherwise everyone's better off just hardcoding the use of unlink() into their code.

Agreed.

* Check file actually exists, if not then return true (because it should never be considered an error to attempt to delete a non-existing filesystem resource, you wanted it gone and it's not there)

Agreed

* Check file is writable, if not then execute `chmod($file, 0777);` and fail if `chmod()` cannot be performed

I am not sure how you do the test whether the file is write able, but in MY case I am 100% sure that the file is write able by the user running the server. However, CHMOD is the problem, let me explain.

Assume "Apache" is the user running the webserver, for the sake of this example I named it the same as the type of server I use (Apache). Editor is the user allowed to edit and make changes and not, it's NOT root. Also "OTHER" is NOT allowed to see the files, this is the correct way to do this for many reasons (goes beyond the scope of what we want to do here).

 -rw-rw---- Editor Apache 3601 Aug 10 2018 EventListener.php

then the Apache Server ALLOWED to do delete, edit and move the file BUT the user of the server is NOT ALLOWED to do a FULL chmod as this goes fully against the LINUX/UNIX security philosophy.
In the example below I have su'ed into a shell AS the user running the server (Apache).
I have then echo'ed (well its an append) something to the file, then displayed whether it was appened.
At the end I did a CHMOD, this WILL and MUST fail on ANY UNIX/LINUX - and it should IMHO.

[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>ls -al
total 56
drwxrwx---. 3 Editor Apache 4096 Mar 28 11:54 .
drwxrwx---. 6 Editor Apache 4096 Aug 10  2018 ..
drwxrwx---. 4 Editor Apache 4096 Aug 10  2018 Controller
-rw-rw----. 1 Editor Apache 7847 Mar 28 11:54 EventListener.php
-rw-rw----. 1 Editor Apache 3601 Aug 10  2018 Page.php
-rw-rw----. 1 Editor Apache 4839 Aug 10  2018 Particles.php
-rw-rw----. 1 Editor Apache 5170 Aug 10  2018 Router.php
-rw-rw----. 1 Editor Apache 3487 Aug 10  2018 Styles.php
-rw-rw----. 1 Editor Apache 4479 Aug 10  2018 ThemeList.php
-rw-rw----. 1 Editor Apache 3952 Aug 10  2018 Theme.php
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>echo "// a comment " > EventListener.php
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>tail -n 4 EventListener.php
        CacheHelper::cleanMenu();
    }
}
// a comment
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>chmod 0777 EventListener.php
chmod: changing permissions of `EventListener.php': Operation not permitted
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>

So you can see Apache is able to write to the file but cannot CHMOD.
Apache should not be able to change the file to 0777, if it could it would be ludicrous.

Change your check to "touch" the file or "move" the file to a temp file then reverse it but do NOT use CHMOD rather tell the user the operation was not successful.

Also I am not sure how you test whether the file is write able, clearly in my case it's 100% write able.
You do not need full 0666 permissions, this would allow OTHERS to see what's in the files and in many cases files contain information that should not be seen by the public.

I know many people say you should have 0664 on files, that is not needed. I am not the only system administrator that will tell you exactly this. The user running the server needs to be able to write and this is 100% sufficient:

 -rw-rw---- Editor Apache 3601 Aug 10 2018 EventListener.php

@jobst
Copy link

jobst commented Mar 28, 2019

closed as Issue seems resolved. If not please reopen.

If I knew how I would ;-)

@orlitzky
Copy link

orlitzky commented Apr 6, 2019

This also completely breaks every Joomla installation that uses filesystem ACLs to grant read/write access. Using "can I chmod" as a test is fucking stupid, and has no basis in anything at all for being correlated with "can I delete this file."

Here's the patch to fix it:

- if (!Path::canChmod($file))
+ if (!Path::canChmod($file) && false)

@HLeithner HLeithner reopened this Apr 6, 2019
@jobst
Copy link

jobst commented Apr 15, 2019

@HLeithner never said I do not understand you are supporting other systems, that is totally fine and all part of being a CMS ... it's just chmod has problems on Linux and that's all I am pointing out!

@mahagr
Copy link
Contributor

mahagr commented Apr 15, 2019

I've been using Linux as my main operating system since 1996 and for 7 years my main job was programming in Linux kernel space in a security company (Firewall/IPS), so I do know POSIX pretty well. I also used to be a security expert and I do know the dangers of using world-writable permissions. I'm not saying that using 0777 is not bad, I'm saying that the code isn't doing that (with the exception of deleting files).

But on the contrary, I can say that using 0755 inside PHP code is BAD, because of it will prevent setups like yours from working because of Apache group isn't allowed to modify or delete the files. Using 0777 (with umask!) works and is safe in every single properly configured setup, 0755 does not because of it will not allow Apache to modify the files. And this is true primarily in Linux.

EDIT: Again, the only place where you should not use 0777 in PHP is chmod() command, which does not use umask. In there you should write chmod(0777 ~ umask()) for folders and without x for files.

@mahagr
Copy link
Contributor

mahagr commented Apr 15, 2019

Just something I noticed from this:

drwxr-x---. 3 /var/www/website1/htdocs/ Editor1 Apache

Unless you are running your web server as Editor1, Joomla is not allowed to modify any of the files in your site. This means that you need to do Joomla updates manually by copying the files by hand and running manually the upgrade scripts.

Another note on the upgrades starting to suddenly fail: there was a bug in Joomla where read-only files were silently skipped by the installer -- installation process passed, but some files were left behind without an update. While not optimal, now the installer at least fails if some of the files cannot be changed, which is a great improvement in my eyes and saves a lot of time for everyone when things do not work after the upgrade. Not that it cannot be improved, I do agree that pre-check before installation may be a good idea.

@ReLater
Copy link
Contributor

ReLater commented Apr 15, 2019

@ mbabker has provided some code in #23504 (comment) .

Now my question to the experts here is: Why don't you provide "better" code with detailed and reproducable testing instructions and code examples that apply to Joomla core?

Anybody can test it then.

@orlitzky
Copy link

Before making the assertion that "changing permissions should not cause delete to fail", you need to understand filesystem permissions. On a Linux filesystem, a delete action requires write permissions. So a file in a readonly state (400 or 444) cannot be deleted

No:

$ mkdir permissions-test
$ touch permissions-test/example.txt
$ sudo chown root:root permissions-test/example.txt
$ sudo chmod 400 permissions-test/example.txt
$ rm -f permissions-test/example.txt

@jobst
Copy link

jobst commented Apr 15, 2019

Just something I noticed from this:

drwxr-x---. 3 /var/www/website1/htdocs/ Editor1 Apache

My bad, I used this as an example.
I made this up (my tree is somewhere else) and forgot to change the group write.
Sorry.

@miojamo
Copy link

miojamo commented Jan 6, 2020

This also completely breaks every Joomla installation that uses filesystem ACLs to grant read/write access. Using "can I chmod" as a test is fucking stupid, and has no basis in anything at all for being correlated with "can I delete this file."

Here's the patch to fix it:

- if (!Path::canChmod($file))
+ if (!Path::canChmod($file) && false)

I have this issue under Windows Ubuntu 18.04 WSL, mounted directory with all permissions set to 777 by default by WSL. This patch worked great! There is no other solution so far. Thanks.

@kris-sum
Copy link

I have this issue under Windows Ubuntu 18.04 WSL, mounted directory with all permissions set to 777 by default by WSL. This patch worked great! There is no other solution so far. Thanks.

^ THIS!
It seems that depending on how your WSL filesystem is mounted, the chmod will fail. Been scratching my head over this for a couple of hours!

@miojamo
Copy link

miojamo commented Jan 31, 2020

@kris-sum Do you have any other solution for the problem? I've tried to reinstall Ubuntu, but issue persist
Also asked here with no answer:
https://askubuntu.com/questions/1203423/windows-10-and-ubuntu-18-04-wsl-folders-permissions-problem

@billtomczak
Copy link

Just an FYI on this issue in case it might be helpful to others still struggling with this issue

We have been using Cloudways servers for our staging and demo sites and can now reliably reproduce this problem. The way they've organized their system, if you use the 'Reset Permissions' option for a site on a server, ownership is given to the master/root user of the server. The only fix is to call their support team and have them set ownership back to the site user. They are aware of the issue and claim they will fix it someday.

Whenever, for whatever reason, the right (wrong?) file(s) are owned by the master user, we'll start having this problem. A quick call to Cloudways support to fix all the file/directory ownership for a site solves the problem.

@orlitzky
Copy link

Just patch out the problematic line. It's wrong and never should have been added. This isn't a problem with your systems, it's a problem with joomla.

@OctavianC
Copy link
Contributor

Just ran into this exact issue on a customer's server. $file is 0777 and can be deleted, but canChmod() fails for some reason and this prevents the installation from continuing. Simply commenting that line out resolves the issue.

@mbabker
Copy link
Contributor

mbabker commented Apr 30, 2020

I find it funny I wrote proof of concept code a year ago that could have theoretically addressed this issue, and yet here we are, a year later, issue still open, and people still instructing to core hack to bypass the issue.

Can someone else please provide some code? Maybe someone will act on it if it didn't come from me. That seems to have worked a few times in regards to getting pull requests actioned.

@orlitzky
Copy link

Your proof-of-concept doesn't work because there's a bug in your understanding of UNIX permissions: "On a Linux filesystem, a delete action requires write permissions." That's clearly false, as #23504 (comment) shows. Checking for write permissions (or the ability to chmod, or anything else other than the ability to actually delete the thing) is therefore pointless and counterproductive in those cases.

I've already proposed working code: delete that line. That is the fix. You don't need to use heuristics to guess if you can delete a file. You try it, and if you can't, it fails and you handle the error.

@mbabker
Copy link
Contributor

mbabker commented May 1, 2020

You're actually right this time, read-write is not necessary on the file in question. It is necessary on the directory which owns the file. Therefore, while removing the check at the file level would be a valid fix, Joomla as an end user facing software should still attempt to check and potentially reset filesystem permissions on the owning directory. Or, do a permissions check before performing the delete operation and fail with a more appropriate error message in the case that the prerequisite permissions are not in place as necessary.

Or, just do what you suggested, blindly remove some code from the platform without actually identifying the root cause of the issue. That always seems to go over well.

You are welcome to make your change proposal as a pull request. Actually, anyone in this thread is. Or, y'all can keep bitching while waiting for an unnamed someone to do something, because I sure as shit ain't doing it.

@mbabker
Copy link
Contributor

mbabker commented May 1, 2020

BTW, the @chmod($file, 0777); line has existed (with the note regarding Windows) since c02dfed from July 2007. It has operated without any form of error check, and has been explicitly error suppressed since July 2007. I don't know how you deliver code or the types of QA measures that you use, but to me error suppression is a red flag that something is failing to do adequate checks or raise appropriate errors in the right conditions. So yeah, a Path::canChmod() check before blindly calling chmod() makes sense. Unless you just say "screw error handling, it's for suckers".

@orlitzky
Copy link

orlitzky commented May 1, 2020

You're actually right this time, read-write is not necessary on the file in question. It is necessary on the directory which owns the file.

Also incorrect, since write-only permissions suffice on any parent directory of that directory. Or write permissions can be granted with POSIX filesystem ACLs, which change the meaning of the w bit. Or explicit "delete" permissions can be given with the NFSv4 ACLs on OSX. Or god knows what can happen on Windows these days.

Therefore, while removing the check at the file level would be a valid fix, Joomla as an end user facing software should still attempt to check and potentially reset filesystem permissions on the owning directory.

I have no idea what you think we should be resetting the permissions to, but this is doomed to fail since you can't even tell me what sort of access controls are in use on the parent directory.

Or, do a permissions check before performing the delete operation and fail with a more appropriate error message in the case that the prerequisite permissions are not in place as necessary.

Even if you could write a working permissions check (you can't), the error message is always going to say roughly the same thing: delete failed for some reason. PHP doesn't provide a better API than that. The difference is that, with an incorrect permissions check, you also throw that error when there is no problem (hence, this bug report).

Or, just do what you suggested, blindly remove some code from the platform without actually identifying the root cause of the issue. That always seems to go over well.

It's hardly blind. The root cause has been identified: someone thought they knew how to tell if a file could be deleted, and they were wrong. Now we're all getting errors when our files are deletable but the buggy check fails. The solution is to remove the buggy check, and to raise/log an error if deleting the file actually fails. What we have now is complicated and doesn't work. Deleting the check is simple and does work. It shouldn't be this hard to sell.

@mbabker
Copy link
Contributor

mbabker commented May 1, 2020

Then the chmod() action needs to go too. Which means validating whether the issue it was working around for Windows based systems needs to be validated. If it needs to stay, then a split path for Windows versus Linux should be implemented if attempting chmod() is really that much of a problem.

Seriously, using error suppression to perform an action, then not even validate that the action succeeds or fails, is about as bad as having a buggy check before attempting said action. Actually, I'd call it borderline worse, because the developer implementing it knew damn well there could be error conditions coming out of the call but was too lazy to do anything with them.

@orlitzky
Copy link

orlitzky commented May 1, 2020

Then the chmod() action needs to go too. Which means validating whether the issue it was working around for Windows based systems needs to be validated. If it needs to stay, then a split path for Windows versus Linux should be implemented if attempting chmod() is really that much of a problem.

No argument there. Personally, I would say that if someone sets the "read-only" flag on a file in Windows, we should do the obvious thing and respect it rather than toggle it off to subsequently be bypassed. Using the read-only flag for access control is the wrong solution to begin with in that situation, as evidenced by the fact that it can be bypassed at all. In other words, the best solution to the problem was probably to ask the user "why did you do that?" way back before the chmod line was added to this function.

Seriously, using error suppression to perform an action, then not even validate that the action succeeds or fails, is about as bad as having a buggy check before attempting said action. Actually, I'd call it borderline worse, because the developer implementing it knew damn well there could be error conditions coming out of the call but was too lazy to do anything with them.

The error isn't suppressed if you skip the canChmod check, it just raises a less descriptive but more accurate error if/when the unlink fails:

if (!@ unlink($file))
{
        throw new FilesystemException(__METHOD__ . ': Failed deleting ' . $filename);
}

Whether or not that's a good idea is debatable, given how crude the API is. If the unlink fails because the file was already deleted, is that an error? But let's not stray too far from the original topic.

@HLeithner HLeithner self-assigned this Sep 25, 2020
@brianteeman
Copy link
Contributor

@HLeithner I see that you assigned this to yourself. It looks to me that there have been no changes relating to this and it is still a valid issue for J4.

Can someone please update the label to J4 Issue and if @HLeithner says its not a valid issue in j4 then they can close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests