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

remove unneeded try {} catch {} on @fopen at Cache FileHandler::writeFile() #1525

Merged

Conversation

samsonasik
Copy link
Member

By providing @ in front of fopen() will make it always return false on failure fopen, so the catch was never reached, try {} catch {} is not needed.

Checklist:

  • Securely signed commits

@natanfelles
Copy link
Contributor

I had issues because fopen is suppressed and the catch just returned false.

Testing the Cache Files, some tests return false but is not possible know why if the cache directory has not write permission. Nothing is logged too.

Maybe a note on user_guide about this is important.

@natanfelles
Copy link
Contributor

Also, the directories inside writable/ comes with chmod 755... Coming with 777 in git clone could solve that.

@samsonasik
Copy link
Member Author

how about check with is_really_writable() function ?

@natanfelles
Copy link
Contributor

I see two options at the moment:

  1. A note in the user guide.
  2. Check with is_really_writable() only once in the FileHandler constructor and if return false throw an exception. This would be the quickest way to detect this problem.

@jim-parry
Copy link
Contributor

I like both the last suggestions from natanfelles.

@samsonasik samsonasik force-pushed the rem-unneeded-try-catch-fopen branch from c1da31c to 1fe3de0 Compare November 29, 2018 06:36
@samsonasik
Copy link
Member Author

I've added note in userguide for use 777 to make path writable and use is_really_writable() check early in __construct.

@samsonasik
Copy link
Member Author

I've added unit test for check against is_really_writable($path) in __construct.

@jim-parry jim-parry added this to the 4.0.0 milestone Nov 29, 2018
@lonnieezell lonnieezell merged commit ef8619d into codeigniter4:develop Nov 30, 2018
@samsonasik samsonasik deleted the rem-unneeded-try-catch-fopen branch November 30, 2018 06:47
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.

4 participants