Re: 1.5.0 Zend_Loader, auto load and non-existent classes

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
Anyone?
Does this deserve reopening the issue and marking it as unresolved?

Jack Sleight wrote:

> Hi,
> This issue was resolved in 1.5.0:
> http://framework.zend.com/issues/browse/ZF-2463, which resulted in the
> error suppression on the include_once on line 83 being removed.
> However, this causes PHP warnings when a file doesn't exist. This is a
> problem when using autoload, because autoload functions are not
> required to successfully include the file, they should simply attempt
> to include it if they can. No errors should occur if the file cannot
> be found.
>
> Line 83 should really be changed to:
>
> if (self::isReadable($file)) {
>    include_once $file;
> }
>
> The current method effectively makes it impossible to simply check if
> a class exists, using autoload where required.

--
Jack
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

weierophinney
Administrator
-- Jack Sleight <[hidden email]> wrote
(on Wednesday, 19 March 2008, 10:44 AM +0000):
> Anyone?
> Does this deserve reopening the issue and marking it as unresolved?

Well, Zend_Loader::autoload() (which is the callback registered  by
Zend_Loader::registerAutoload()) actually wraps Zend_Loader::loadClass()
in a try/catch block, and returns a boolean false if the class was not
found.

PHP Warnings simply indicate that the file was not loaded; they are not
a fatal issue, but simply an indicator that something is potentially
wrong. In your production environments, I would expect you would have
display_errors off as it is, and these would never be seen. In your
development environment, having these show is probably a good thing, as
it shows potential breakage in your code.

I wouldn't re-open the issue, but if you feel strongly about it, I'd
recommend opening a separate issue asking for the following: that
Zend_Loader::registerAutoload() register an error handler that catches
only the warnings generated by include_once and suppresses them. And
then we can restart the discussion.

> Jack Sleight wrote:
>> Hi,
>> This issue was resolved in 1.5.0:
>> http://framework.zend.com/issues/browse/ZF-2463, which resulted in the
>> error suppression on the include_once on line 83 being removed. However,
>> this causes PHP warnings when a file doesn't exist. This is a problem when
>> using autoload, because autoload functions are not required to
>> successfully include the file, they should simply attempt to include it if
>> they can. No errors should occur if the file cannot be found.
>>
>> Line 83 should really be changed to:
>>
>> if (self::isReadable($file)) {
>>    include_once $file;
>> }
>>
>> The current method effectively makes it impossible to simply check if a
>> class exists, using autoload where required.
>
> --
> Jack
>

--
Matthew Weier O'Phinney
PHP Developer            | [hidden email]
Zend - The PHP Company   | http://www.zend.com/
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
Hi Matthew,
having these show is probably a good thing, as
it shows potential breakage in your code
That isn't necessarily the case. The reason this is an issue for me is that I'm using class_exists() to check if certain classes have been defined. class_exists() will use the registered autoload functions to attempt to load the class, and if they have no luck will obviously return false. In this instance, having PHP warnings show up when the class is not found is of no help at all, because that's nothing to do with an error in the code, it's not that I'm actually trying to use the class.

This is a problem for the development environment, and although display_errors is set to false on the production server, it will still pollute the error logs with warnings that shouldn't be there.

Although it doesn't explicitly say so, this line from the PHP docs implies that the auto load function should simply try to include the class, not require that is is successful in doing so.
By calling this function the scripting engine is given a last chance to load the class before PHP fails with an error.
If you were to try and use a class that none of the autoload functions could include, PHP would throw its own class not defined error.

There is also an issue when using multiple autoloads. What if Zend_Loader::autoload() isn't the last autoload function in the stack? And the particular class you're trying to use should be loaded by the last autoload? Zend_Loader::loadClass() is still going to throw a file not found error, despite the fact that the class may still get loaded.

Matthew Weier O'Phinney wrote:
-- Jack Sleight [hidden email] wrote
(on Wednesday, 19 March 2008, 10:44 AM +0000):
  

Well, Zend_Loader::autoload() (which is the callback registered  by
Zend_Loader::registerAutoload()) actually wraps Zend_Loader::loadClass()
in a try/catch block, and returns a boolean false if the class was not
found.

PHP Warnings simply indicate that the file was not loaded; they are not
a fatal issue, but simply an indicator that something is potentially
wrong. In your production environments, I would expect you would have
display_errors off as it is, and these would never be seen. In your
development environment, having these show is probably a good thing, as
it shows potential breakage in your code.

I wouldn't re-open the issue, but if you feel strongly about it, I'd
recommend opening a separate issue asking for the following: that
Zend_Loader::registerAutoload() register an error handler that catches
only the warnings generated by include_once and suppresses them. And
then we can restart the discussion.

  

  

--
Jack
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
In reply to this post by weierophinney
Also,

Matthew Weier O'Phinney wrote:
> I wouldn't re-open the issue, but if you feel strongly about it, I'd
> recommend opening a separate issue asking for the following: that
> Zend_Loader::registerAutoload() register an error handler that catches
> only the warnings generated by include_once and suppresses them. And
> then we can restart the discussion.
>  
I really don't think that's required, the simplest fix is to just change
line 83 from:

include_once $file;

To:

if (self::isReadable($file)) {
   include_once $file;
}
--
Jack
Reply | Threaded
Open this post in threaded view
|

AW: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Tobias Gies
In reply to this post by Jack Sleight

That isn't necessarily the case. The reason this is an issue for me is that I'm using class_exists() to check if certain classes have been defined. class_exists() will use the registered autoload functions to attempt to load the class, and if they have no luck will obviously return false. In this instance, having PHP warnings show up when the class is not found is of no help at all, because that's nothing to do with an error in the code, it's not that I'm actually trying to use the class.

 

Hey Jack,

 

You’ll probably want to use class_exists(‘…’, false) to tell class_exists to NOT use the autoloader.

 

Best regards,

Tobias

Reply | Threaded
Open this post in threaded view
|

Re: AW: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
Tobias Gies wrote:

 Hey Jack,

 

You’ll probably want to use class_exists(‘…’, false) to tell class_exists to NOT use the autoloader.

 

Best regards,

Tobias

But I do want it to use the autoloader. Whether you set that to true or false, using class_exists() should never result in PHP errors related to files not being found.
--
Jack
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

weierophinney
Administrator
In reply to this post by Jack Sleight
-- Jack Sleight <[hidden email]> wrote
(on Wednesday, 19 March 2008, 01:55 PM +0000):

> Matthew Weier O'Phinney wrote:
> > I wouldn't re-open the issue, but if you feel strongly about it, I'd
> > recommend opening a separate issue asking for the following: that
> > Zend_Loader::registerAutoload() register an error handler that catches
> > only the warnings generated by include_once and suppresses them. And
> > then we can restart the discussion.
> >  
> I really don't think that's required, the simplest fix is to just change
> line 83 from:
>
> include_once $file;
>
> To:
>
> if (self::isReadable($file)) {
>   include_once $file;
> }

We explicitly are *not* doing that to keep this lean; isReadable() adds
some significant overhead, particularly when the method is called
repeatedly as it may with autoloading.

--
Matthew Weier O'Phinney
PHP Developer            | [hidden email]
Zend - The PHP Company   | http://www.zend.com/
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
OK then. Well I guess another method (such as yours) should be used.

Either way, no autoload method should assume that is is the only autoload method, and therefore no autoload method should cause an error when it can't include the requested class, it should simply fail silently. It isn't the job of any individual autoload to ensure a class is defined. Do you at least agree with me on that?

I'd not really thought of this as something I specifically "feel strongly" about, I'd assumed it was an obvious flaw with Zend_Loader that most people would agree with, I guess I was wrong. I'll create a new issue and hopefully get some more opinions on the matter.

Matthew Weier O'Phinney wrote:
-- Jack Sleight [hidden email] wrote
(on Wednesday, 19 March 2008, 01:55 PM +0000):
  

We explicitly are *not* doing that to keep this lean; isReadable() adds
some significant overhead, particularly when the method is called
repeatedly as it may with autoloading.

  

--
Jack
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Darby Felton
In reply to this post by weierophinney
Matthew Weier O'Phinney wrote:

> -- Jack Sleight <[hidden email]> wrote
> (on Wednesday, 19 March 2008, 10:44 AM +0000):
>> Anyone?
>> Does this deserve reopening the issue and marking it as unresolved?
>
> Well, Zend_Loader::autoload() (which is the callback registered  by
> Zend_Loader::registerAutoload()) actually wraps Zend_Loader::loadClass()
> in a try/catch block, and returns a boolean false if the class was not
> found.
>
> PHP Warnings simply indicate that the file was not loaded; they are not
> a fatal issue, but simply an indicator that something is potentially
> wrong. In your production environments, I would expect you would have
> display_errors off as it is, and these would never be seen. In your
> development environment, having these show is probably a good thing, as
> it shows potential breakage in your code.
>
> I wouldn't re-open the issue, but if you feel strongly about it, I'd
> recommend opening a separate issue asking for the following: that
> Zend_Loader::registerAutoload() register an error handler that catches
> only the warnings generated by include_once and suppresses them. And
> then we can restart the discussion.

Indeed, my original solution to the problem of complete error
suppression of include[_once] directives (e.g., invisible parse errors)
was to accumulate PHP errors as a property of an Exception object that
would be thrown. The problem, of course, is that doing all this fancy
stuff in a part of the code that must be fast may exert undue
performance costs on people not needing such features in production. It
also was not strictly backward compatible with previous behavior.
Perhaps this type of behavior could be optional, but performance is
definitely something to consider with this part of the framework.

Best regards,
Darby

>
>> Jack Sleight wrote:
>>> Hi,
>>> This issue was resolved in 1.5.0:
>>> http://framework.zend.com/issues/browse/ZF-2463, which resulted in the
>>> error suppression on the include_once on line 83 being removed. However,
>>> this causes PHP warnings when a file doesn't exist. This is a problem when
>>> using autoload, because autoload functions are not required to
>>> successfully include the file, they should simply attempt to include it if
>>> they can. No errors should occur if the file cannot be found.
>>>
>>> Line 83 should really be changed to:
>>>
>>> if (self::isReadable($file)) {
>>>    include_once $file;
>>> }
>>>
>>> The current method effectively makes it impossible to simply check if a
>>> class exists, using autoload where required.
>> --
>> Jack
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Darby Felton
In reply to this post by Jack Sleight
Jack Sleight wrote:
> OK then. Well I guess another method (such as yours) should be used.
>
> Either way, no autoload method should assume that is is the only
> autoload method, and therefore no autoload method should cause an error
> when it can't include the requested class, it should simply fail
> silently. It isn't the job of any individual autoload to ensure a class
> is defined. Do you at least agree with me on that?

I tend to agree that the autoloader function should not produce errors,
and think that it would be worth creating a JIRA issue for this
particular problem. I also think it can be solved easily enough.

Best regards,
Darby

>
> I'd not really thought of this as something /I/ specifically "feel
> strongly" about, I'd assumed it was an obvious flaw with Zend_Loader
> that most people would agree with, I guess I was wrong. I'll create a
> new issue and hopefully get some more opinions on the matter.
>
> Matthew Weier O'Phinney wrote:
>> -- Jack Sleight <[hidden email]> wrote
>> (on Wednesday, 19 March 2008, 01:55 PM +0000):
>>  
>>
>> We explicitly are *not* doing that to keep this lean; isReadable() adds
>> some significant overhead, particularly when the method is called
>> repeatedly as it may with autoloading.
>>
>>  
>
> --
> Jack
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
I've created an issue for this:
http://framework.zend.com/issues/browse/ZF-2923

Darby Felton wrote:
> Jack Sleight wrote:
>
> I tend to agree that the autoloader function should not produce
> errors, and think that it would be worth creating a JIRA issue for
> this particular problem. I also think it can be solved easily enough.
>
> Best regards,
> Darby
>

--
Jack
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Stanislav Malyshev
In reply to this post by Darby Felton
> I tend to agree that the autoloader function should not produce errors,

Does it produce error or warning?
--
Stanislav Malyshev, Zend Software Architect
[hidden email]   http://www.zend.com/
(408)253-8829   MSN: [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Darby Felton
Stanislav Malyshev wrote:
>> I tend to agree that the autoloader function should not produce errors,
>
> Does it produce error or warning?

Depends on the situation. For example, if you autoload with Zend_Loader
a class for which the file does not exist, include_once emits a warning.
If the file exists, and it contains a parse error, it is not suppressed
and the fatal error is handled in the normal PHP way. It's also possible
that user code contains something that generates other warnings or
errors, and these are neither suppressed nor caught using a custom error
handler.

Best regards,
Darby
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Stanislav Malyshev
> Depends on the situation. For example, if you autoload with Zend_Loader
> a class for which the file does not exist, include_once emits a warning.
> If the file exists, and it contains a parse error, it is not suppressed
> and the fatal error is handled in the normal PHP way. It's also possible

I think we can omit (maybe based on configuration) the warning when file
not found, but the error is OK.

> that user code contains something that generates other warnings or
> errors, and these are neither suppressed nor caught using a custom error
> handler.

This is OK too.
--
Stanislav Malyshev, Zend Software Architect
[hidden email]   http://www.zend.com/
(408)253-8829   MSN: [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Darby Felton
Stanislav Malyshev wrote:

>> Depends on the situation. For example, if you autoload with
>> Zend_Loader a class for which the file does not exist, include_once
>> emits a warning. If the file exists, and it contains a parse error, it
>> is not suppressed and the fatal error is handled in the normal PHP
>> way. It's also possible
>
> I think we can omit (maybe based on configuration) the warning when file
> not found, but the error is OK.
>
>> that user code contains something that generates other warnings or
>> errors, and these are neither suppressed nor caught using a custom
>> error handler.
>
> This is OK too.

I'll then plan to branch Zend_Loader to the incubator with some code to
implement selective suppression of the "file not found" warnings in
autoload().

I'm curious whether the performance cost to production applications will
be deemed worth the benefit, hence my intent to branch to the incubator
for benchmarking comparison with the trunk version. Hopefully the
difference will be negligible for most applications.

Any other opinions?

Best regards,
Darby
Reply | Threaded
Open this post in threaded view
|

Re: Re: 1.5.0 Zend_Loader, auto load and non-existent classes

Jack Sleight
I agree the only errors that should be suppressed are the file not found
errors. Parse errors would break the application, so need to be thrown
as usual.

Darby Felton wrote:

> Stanislav Malyshev wrote:
>
> I'll then plan to branch Zend_Loader to the incubator with some code
> to implement selective suppression of the "file not found" warnings in
> autoload().
>
> I'm curious whether the performance cost to production applications
> will be deemed worth the benefit, hence my intent to branch to the
> incubator for benchmarking comparison with the trunk version.
> Hopefully the difference will be negligible for most applications.
>
> Any other opinions?
>
> Best regards,
> Darby

--
Jack