History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: ZF-2985
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Trivial Trivial
Assignee: Thomas Weidner
Reporter: Gunter Spöcker
Votes: 6
Watchers: 10
Operations

If you were logged in you would be able to see more operations.
Google issue summary
Zend Framework

Bug in Zend_Loader::isReadable if file does not exist

Created: 27/Mar/08 09:24 PM   Updated: 03/Jun/09 04:04 AM
Component/s: Zend_Loader
Affects Version/s: 1.5.0
Fix Version/s: 1.8.0

Time Tracking:
Original Estimate: 8 hours
Original Estimate - 8 hours
Remaining Estimate: 8 hours
Remaining Estimate - 8 hours
Time Spent: Not Specified
Remaining Estimate - 8 hours

Issue Links:
Dependency
 

 Public Fields   Internal Project Management Fields   
Tags:
Participants: Basil Guevarra, Darby Felton, Dolf Schimmel, Emil Ivanov, EV, Gunter Spöcker, Jeffrey Sambells, julien PAULI, Martin Winkel, Matthew Weier O'Phinney, Satoru Yoshida and Thomas Weidner
Fix Version Priority: Must Have


 Description  « Hide
If you're running the PHP application in strict mode and the file you wish to check for readability does not exist, an warning is given by PHP.
In my case this occurred if you validate a xyz@domain.net eMail address since in the folder "Hostname" is no file for .net domains.

A trivial fix would be:

if (!file_exists($filename) || !$fh = @fopen($filename, 'r', true)) {

instead of:

if (!$fh = @fopen($filename, 'r', true)) {


 All   Comments   Work Log   Change History   FishEye   Crucible      Sort Order: Ascending order - Click to sort in descending order
Satoru Yoshida - 14/Apr/08 08:18 AM
Can you tell me whether or not we also should fix Zend_Cache::_isReadable ?
It seems Zend_Cache has same problem.

Gunter Spöcker - 14/Apr/08 08:49 AM
I would say so, since it is basically the same code.

Satoru Yoshida - 14/Apr/08 07:34 PM
I think ZF-2701 has same trouble like as this.

Satoru Yoshida - 23/Apr/08 10:37 AM
Resolved in SVN r2925

Darby Felton - 23/Apr/08 12:47 PM
Reopening, since SVN r9295 should be reverted because fopen() is directed to utilize the include_path, whereas file_exists() does not use the include_path.

Jeffrey Sambells - 23/Apr/08 01:45 PM
Ya, r9295 breaks the PluginLoader: exception 'Zend_Loader_PluginLoader_Exception' with message 'Plugin by name Word_CamelCaseToDash was not found in the registry.'

julien PAULI - 24/Apr/08 03:09 AM
Please, revert and see ZF-3170 as well

I have some more Zend_Loader tests to commit as well


Satoru Yoshida - 24/Apr/08 10:05 AM
Thank you for mailing me and reverting, Darby.

Satoru Yoshida - 02/May/08 10:00 AM
testing new code now

Satoru Yoshida - 07/May/08 08:20 AM
I think the function will be as following.
Today, I tested with Zend_Loader_PluginLoader.
And I will try to test with other components.
I will be happy If you tell me any ideas
public static function isReadable($filename)
{
    if (is_readable($filename)) {
        return true;
    }

    $paths = explode(PATH_SEPARATOR, get_include_path());

    foreach ($paths as $path) {
        if (is_readable($path . DIRECTORY_SEPARATOR . $filename)) {
            return true;
        }
    }

    return false;
}

Emil Ivanov - 14/May/08 04:59 AM
I believe that SplFileObject supports opening a file in the include path and it throws an exception (that can be caught if the file is not found.

Satoru Yoshida - 14/May/08 07:09 AM
Fixed in SVN r9451. In SVN r6780, fopen method was used instead of is_readable method. But it causes some problems. So, I fix by using is_readable.

To:Emil Ivanov
Thanks your comment. But Unfortunately, I can not use filename in openFile method of SplFileObject.


Basil Guevarra - 14/May/08 01:53 PM
Pardon me if I'm posting in the wrong place, but after updating to SVN r9451 I noticed breakage related to changes in Loader.php's isReadable() method.

Symptoms: My rendered views no longer worked as if the files template files were not being read. (Note, I use a Smarty/ViewRenderer solution)

I made a patch in my local copy by placing your suggested is_readable($filename) check above the directory path traversing.

I see you had posted on May 7th the inclusion of the is_readable($filename), I believe it should be added for consideration.

<code>
public static function isReadable($filename)
{
// If the directory methods above don't work, try the filename directly
if (is_readable($filename)) { return true; }

$dirs = explode(PATH_SEPARATOR, get_include_path());
$dirs = array_merge($dirs, array('.'));

foreach ($dirs as $dir) {
if (is_readable($dir . DIRECTORY_SEPARATOR . $filename)) { return true; }
}

return false;
}
</code>

Thanks,


Satoru Yoshida - 14/May/08 03:16 PM
Hello, Basil. You means is_readable( "./$filename") does not work?
If so, I will add is_readable($filename) and remove array_merge($dirs, array('.')).

Emil Ivanov - 14/May/08 11:44 PM
Satoru Yoshida: "Thanks your comment. But Unfortunately, I can not use filename in openFile method of SplFileObject."

Do not use openFile, use $f = new SplFileObject($filename, $mode, $useIncludePath);


Basil Guevarra - 15/May/08 06:12 AM
Correct Satoru, the is_readable(./$filename) does not work. Adding the is_readable($filename) and removing array_merge($dirs, array('.')) would be good. Thanks!

Satoru Yoshida - 15/May/08 10:24 PM
Thank you for advice and responces, Emil Ivanov and Basil Guevarra .

Buy the way I hear ZF Dev Team members works on this matter.
I will wait their to expect good results.


Martin Winkel - 11/Jun/08 06:07 AM
I saw the same problem too, just like the other problem in a previous topic.

I think isReadable should also search in the default root, not using an includepath, so I did as follows:

/**
     * Returns TRUE if the $filename is readable, or FALSE otherwise.
     * This function uses the PHP include_path, where PHP's is_readable()
     * does not.
     *
     * @param string   $filename
     * @return boolean
     */
    public static function isReadable($filename)
    {
    	$arrPaths = explode(PATH_SEPARATOR, get_include_path() );
    	
    	$blnReturn = false;
    	
    	if(  file_exists( $filename ) &&  $fh = @fopen( $filename, 'r') ){
    		@fclose($fh);
    		$blnReturn = true;
    	}else{
	    	
    		if( substr($filename, 0, 2) === './' ) $filename = substr($filename, 2);
	    	
    		foreach($arrPaths as $path){
	    		if( substr($path, -1, 1) !== '/' && substr($path, -1, 1) !== '\\'  ) $path .= '/';
	    		
	    		if ( file_exists( $path . $filename ) &&  $fh = @fopen($path . $filename, 'r')) {
		            @fclose($fh);
	    			$blnReturn = true;
		        }
	    		
	    	}
    	}
    	
    	

        return $blnReturn;
    }

Matthew Weier O'Phinney - 02/Oct/08 08:03 PM
Two notes: First, any solution that involves looping over paths in userland code will be rejected. These solutions have a tremendous performance impact, which is why the fopen() solution was chosen (the third argument to this function allows it search on the include_path – but doing so in C is magnitudes faster than doing so in userland PHP).

Second, the fopen() call also has the error suppression operator; you will only see these errors in your error log. This is acceptable when it comes to E_STRICT standards – the spirit of this rule refers to the messages that will be displayed when display_errors is on. This is one reason why error suppression is discouraged – it should be used only for very, very good reasons. This is one such case, to our thinking.


Satoru Yoshida - 06/Oct/08 06:03 AM
Hi, Matthew.

I thank you that you explain why fopen() should be selected than other ideas.

I think this issue should be closed as 'Won' t fix' , do you think?


Satoru Yoshida - 03/Nov/08 05:06 AM
Hi, All.

I wondered why fopen() with at mark happens.
Because I did not find any warning in my tests with ver.1.6.1, 1.6.2, 1.5.3 and etc.

I understand what causes the warning, by talking about Simon on ZF-2900.
I recommend to look at that issue to solve this issue.


EV - 15/May/09 03:25 PM
"First, any solution that involves looping over paths in userland code will be rejected. These solutions have a tremendous performance impact, which is why the fopen() solution was chosen"

According to my tests, your fopen-based method is always 5-30% slower than my "looping over paths in userland code" method.

Here's my test code:
first add the new method to Zend/Loader.php:
public static function _isReadable($filename)
{
if (is_readable($filename)) return true;
foreach (explode(PATH_SEPARATOR, get_include_path()) as $path)

{ if (is_readable($path . (in_array(substr($path, -1), array('\\', '/')) ? $filename : DIRECTORY_SEPARATOR . $filename))) return true; }

return false;
}

Now run test:
<?php
set_include_path(implode(PATH_SEPARATOR, array('/path1', '/path2', '/path3', '/path4', '/path5', '/path6')) . PATH_SEPARATOR . get_include_path());

require_once '/ZF/library/Zend/Loader.php';

$start = microtime(true);
for ($i = 0, $count = 1000; $i < $count; $i++)

{ Zend_Loader::_isReadable("nonexistingFile$i"); Zend_Loader::_isReadable('Var_Dump.php'); }

$stop = microtime(true);
echo 'Using new method: ' . ($stop - $start) . ' seconds<br />';

$start = microtime(true);
for ($i = 0, $count = 1000; $i < $count; $i++)

{ Zend_Loader::isReadable("nonexistingFile$i"); Zend_Loader::isReadable('Var_Dump.php'); }

$stop = microtime(true);
echo 'Using old fopen-method: ' . ($stop - $start) . ' seconds<br />';

?>

Var_Dump.php is part of PEAR, and was just added to test for existing files in include_path (assuming PEAR path is part of include_path).

You could try commenting out the call to set_include_path(), or calls to both methods using nonexisting file or existing file, just to check that the resulting times are always similar when given similar input. The two methods also provide the same actual results, of course.

Could you please change this now, or do you need more proof?


EV - 15/May/09 03:52 PM
Sorry, there was a bug in my method - empty input would return true, as is_readable would only check against the directories in include_path.
Adding the following line to the beginning of the method fixes the problem.
if ($filename === null || $filename === false || $filename === '' || !is_scalar($filename)) return false;

This change will not effect performance (noticeably anyway).


Dolf Schimmel - 27/May/09 10:45 AM
Reopening as requested in a comment in ZF-2900 , I'm curious...

EV - 28/May/09 01:02 AM
Actually, I asked that this issue was un-duplicated, ie. remove the duplicate tag for this issue. I also mentioned that I would post a new bug instead, implying that re-opening this one would not be necessary.

You may close this again unless there were other reasons than my comment in ZF-2900.


Satoru Yoshida - 28/May/09 02:35 AM
If you think it is not duplicated issue, I would be out of ability to close this.
change into unassign.

Thomas Weidner - 03/Jun/09 04:04 AM
Removed link to ZF-2900.

Closed issue again as the mentioned code is not reproducable and already fixed within the actual release.