Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix #178: Check if the file (directory) exists before deleting in Cache_file.php #239

Closed
wants to merge 4 commits into from

8 participants

John Bellone Eric Barnes Phil Sturgeon Sean Lavine Tyler Brownell Chris Berthe vlakoff Andrey Andreev
John Bellone

Question is should we return FALSE or TRUE is the directory does not exist? My suggestion is FALSE, as the operation was not performed.

John Bellone

My damn tabs are messed up in Emacs. There should be a tabify on one of the pre-hooks.

Eric Barnes

This needs to match our coding standards before it can be merged.
http://codeigniter.com/user_guide/general/styleguide.html

Phil Sturgeon

Maybe default should be NULL? If nothing happens then a "meh" response seems reasonable?

John Bellone

Well, unlink() returns TRUE for success and FALSE for failure, wouldn't it be better to stick to that expected value? I'll change it if you want :).

Sean Lavine

-1. Just add @ on unlink if you want to get rid of the warning.

Tyler Brownell

Isn't it better to avoid suppressing errors/warnings and instead attempt to prevent them in the first place?

May I suggest using the is_readable() function in place of the file_exists() function, as it also checks to see if you have permission to touch the file in question?

Chris Berthe

Agreed with everything @refringe said. I too was going to suggest is_readable() as it checks both cases.

Sean Lavine

Isn't it better to avoid suppressing errors/warnings and instead attempt to prevent them in the first place?

Usually, but I don't see why you would want to in this case. There is no conditional you need to perform. You are simply returning a boolean indicating whether the file was deleted or not. Why add an additional check unless you are going to return a separate value indicating that the file doesn't exist?

Tyler Brownell

I completely understand what you mean @freewil; in either case it would still return the same value. However, I've always believed it to be best practice to prevent warnings when you can, and only use the suppression method in a pinch.

Every time I write @ in PHP I feel dirty on the inside.

Sean Lavine

@refringe lol, it's only dirty if you do it to cover up your lack of error handling. In this case there is no error to handle, the intended operation is done, we don't care about the warning.

Tyler Brownell

True—You've got me there. Made me think though, should we throw an error if the file doesn't exist or we don't have permissions?

 log_message('error', 'Unable to delete file "' . $this->_cache_path . $id . '" - File not found.');
vlakoff

Has been fixed in a3a8b61 (2011-09-16)

Changelog entry added in 255a5c1 (maybe could be improved: not an error but a warning)

Andrey Andreev narfbg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 21, 2011
  1. John Bellone

    Adding file_exists

    johnbellone authored
  2. John Bellone

    Fixing style

    johnbellone authored
  3. John Bellone
Commits on Aug 22, 2011
  1. John Bellone
This page is out of date. Refresh to see the latest.
5 system/libraries/Cache/drivers/Cache_file.php
View
@@ -107,6 +107,11 @@ public function save($id, $data, $ttl = 60)
*/
public function delete($id)
{
+ if ( ! file_exists($this->_cache_path.$id))
+ {
+ return FALSE;
+ }
+
return unlink($this->_cache_path.$id);
}
1  user_guide/changelog.html
View
@@ -108,6 +108,7 @@
<li class="reactor">Fixed a bug where not setting 'null' when adding fields in db_forge for mysql and mysqli drivers would default to NULL instead of NOT NULL as the docs suggest.</li>
<li class="reactor">Fixed a bug where using <kbd>$this->db->select_max()</kdb>, <kbd>$this->db->select_min()</kdb>, etc could throw notices. Thanks to w43l for the patch.</li>
<li class="reactor">Replace checks for STDIN with php_sapi_name() == 'cli' which on the whole is more reliable. This should get parameters in crontab working.</li>
+ <li class="reactor">Fixed issue #178: Checking if file exists before performing unlink() operation to get rid of warning message.</li>
</ul>
<h2>Version 2.0.2</h2>
Something went wrong with that request. Please try again.