Bulletproof __autoloader

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Bulletproof __autoloader

Post by Benjamin »

In my never ending quest to perfect the code that I write, I wrote this autoloader that checks if the file exists and that it is readable. Should I check that it's executable as well? Anything else I can make better?

Code: Select all

#error logging class is already initialized at this point

# init fatal_exception
class fatal_exception extends Exception {
    public function __construct($e)
    {
        parent::__construct();
        $log = e_log::getInstance(); // just an error logger
        $log->newEntry($e);
        header("Location: " . HTTP_DOMAIN . 'unavailable.html');
        exit();
    }
}



# init autoloader
function __autoload($class) {
    $f = WEBSITE_ROOT_PATH . 'includes/classes/' . $class . '.php';
    if ((file_exists($f)) && (is_readable($f)))
    {
        require_once $f;
    } else {
        new fatal_exception("FATAL: Could not init class $class");
    }
}
User avatar
Oren
DevNet Resident
Posts: 1640
Joined: Fri Apr 07, 2006 5:13 am
Location: Israel

Post by Oren »

Didn't you mean?

Code: Select all

throw new fatal_exception("FATAL: Could not init class $class");
And this:

Code: Select all

if ((file_exists($f)) && (is_readable($f)))
Could be written like this:

Code: Select all

if (file_exists($f) && is_readable($f))
It's much easier to read it like the above :wink:
User avatar
Verminox
Forum Contributor
Posts: 101
Joined: Sun May 07, 2006 5:19 am

Post by Verminox »

If the file exists and it is readable, then it should logically, get included. And the point of using 'require' instead of 'include' is that the script stops execution if it cannot be included, so I am sensing some kind of contradiction here.

For example, if the file cannot be included due to some other reason (if that is possible), then the execution will stop due to the fact that 'require' failed, and yet it will not throw an execption.

Also, you are not specifying the reason why the file could not be included (such as missing, or cannot read) therefore you can make it shorter like so:

Code: Select all

<?php
# init autoloader
function __autoload($class) {
    $f = WEBSITE_ROOT_PATH . 'includes/classes/' . $class . '.php';
    $a = @include_once $f;
    if(!$a) new fatal_exception("FATAL: Could not init class $class");
} 
?>
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Oren wrote:Didn't you mean?

Code: Select all

throw new fatal_exception("FATAL: Could not init class $class");
I'm not using it within a try/catch block so I don't believe I need to throw the exception.
Oren wrote:

Code: Select all

if ((file_exists($f)) && (is_readable($f)))
Could be written like this:

Code: Select all

if (file_exists($f) && is_readable($f))
Yeah that is shorter, but I've gotten into the habit of using multiple parenthesis.
Verminox wrote: If the file exists and it is readable, then it should logically, get included. And the point of using 'require' instead of 'include' is that the script stops execution if it cannot be included, so I am sensing some kind of contradiction here.
I didn't change the require_once from the sample code in the php manual for the autoloader. I can't imagine why the autoloader would attempt to include a class that is already defined, but I left it the way it was to be safe. Any clues on that? Is there a chance the autoloader would attempt to reload a class?
User avatar
Verminox
Forum Contributor
Posts: 101
Joined: Sun May 07, 2006 5:19 am

Post by Verminox »

I wasn't talking about the '_once' part, I was talking about 'require' vs. 'include'. If you use 'require_once' then on failure your script will exit execution without throwing an execption. Therefore it is more logical to use '@include_once' so that even if that fails, there will be no error message and the script will continue and you can throw an exception if it failed.


Also, in fatal_exception::__construct() you have the following line:

Code: Select all

parent::__construct();
Shouldn't it be...

Code: Select all

parent::__construct($e);
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

You'll get an uncaught exception error, or possibly parse error (unexpected T_NEW on line blah) as you are not throwing or assigning the new exception. Haven't tested.

The multiple parenthesis is semantics :P PEAR coding standards use each condition within parenthesis (e.g. 'if ((condition1) && (condition2))' or atleast was the last time I read them) and is also my choice of coding standard. Personal Preference :)

Also, using the line

Code: Select all

$a = @include_once $f;
may cause problems. There is a 'feature' that if you error suppress an include, it will suppress any and all errors generated by the code within the included file.
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Post by volka »

What's the point of a new exception object if it is neither used nor thrown not even returned?
Last edited by volka on Mon Aug 14, 2006 5:49 am, edited 1 time in total.
User avatar
Verminox
Forum Contributor
Posts: 101
Joined: Sun May 07, 2006 5:19 am

Post by Verminox »

Jenk wrote:Also, using the line

Code: Select all

$a = @include_once $f;
may cause problems. There is a 'feature' that if you error suppress an include, it will suppress any and all errors generated by the code within the included file.
Indeed, I overlooked that. My bad.

However, if your included file just contains one class definition and you know for sure that it does not have any parse errors then I don't see any reason to worry.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Jenk wrote:You'll get an uncaught exception error, or possibly parse error.
I'll have to test it and find out. Based on what I read (Advanced PHP Programming) it should be ok. I shouldn't have to wrap the entire application in a try/catch block..

EDIT: I may have to use set_exception_handler(). But, since I'm not throwing it, I have a feeling it will work. But if I were to throw it, then it would not be caught. I'll post back after i play around with it.

EDIT 2: Just ran a quick test, it works fine with the following code..

Code: Select all

<?php
class fatal_exception extends Exception {
    public function __construct($e)
    {
        parent::__construct($e);
        echo $e;
    }
}

$x = new fatal_exception("A FATAL EXCEPTION HAS OCCURRED");
?>
volka wrote:What's the point of a new exception objection if it is neither used nor thrown not even returned?
I'm throwing an exception, which is a fatal_error, which causes a header redirect and an event to be added to an event log. Is that not right?
Last edited by Benjamin on Mon Aug 14, 2006 6:12 am, edited 3 times in total.
User avatar
Oren
DevNet Resident
Posts: 1640
Joined: Fri Apr 07, 2006 5:13 am
Location: Israel

Post by Oren »

Jenk wrote:The multiple parenthesis is semantics :P PEAR coding standards use each condition within parenthesis (e.g. 'if ((condition1) && (condition2))' or atleast was the last time I read them) and is also my choice of coding standard. Personal Preference :)
I also use them, but only where it is needed. In this particular case, I don't see why it is needed.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Verminox wrote:However, if your included file just contains one class definition and you know for sure that it does not have any parse errors then I don't see any reason to worry.
I almost used your code until I saw that. There may be times when I want errors displayed, especially during local debugging. I have a feeling that the PHP manual is showing the proper way to use the __autoloader. Until I understand it better, I will use require_once. Besides, it's not going to fail anyway if the file exists and is readable.

My only concern there is, what if the file doesn't have execute permissions? Which brings up another question.. Are included files "evaled" or "executed" by the PHP engine?
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Re: Bulletproof __autoloader

Post by volka »

Ah, I didn't look at the constructor code of that exception. For a simple reason: I didn't expect it to do something like logging the message somewhere. Is this really necessary for constructing the object? I doubt it. In fact it's so unessential that new fatal_exception("FATAL: Could not init class $class"); only for logging a certain message looks weird to me. If this class wasn't yours would you have expected something like this? Or let me ask it in another way: Would you have thought of or searched in this way if you had been looking for a logging mechanism?
It also generates an (in my view) unnecessary strong (but weak featured) coupling with this e_log class: specified classname, only default logger available, two functions in this class required (for only one purpose) - and again: all this still only for constructing the object.
And last but not least, not everything,anything has to be logged. It's like the personal firewall constantly crying wolf. It's annoying and distracting. Why would I want an unconditional message logging if all that happend was the construction of a simple object? It also somehow contradicts the usefullness of exceptions.
(sorry, have to go right now, only short form for now)
a()->b()->c()->d(). Now an exception occurs in d(). But b() and c() do not care, all they have to do is ...not to do anything, only a() cares about that the execution somehow was aborted. In the old days without exceptions d() hat to return an error code, c() had to check it and also to return it. Same with b ...extra code ...extra code ...extra code without real meaning. Now you throw an exception in d() and since c() and b() do nothing it (more or less) directly ends in the catch block of a(), no extra code in b() and c() for "i really do not care". That's the beauty of exceptions. And that's why I wouldn't want exception handling (like logging) in the constructor of my exceptions ;)
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Thanks for the insight volka. I'll read that a few times and digest it. Basically I am taking a layered approach to the Exceptions. The fatal error is the error of last resort in the application. There will be other Exception classes as well. Some recoverable, some not. The fatal_exceptions are exceptions that should never happen, which is why they are all logged.
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Code: Select all

$x = new fatal_exception(/*... */);
is fine, it was because in your first post you didn't throw, nor assign the new object that it would parse error. :)
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

astions wrote:Are included files "evaled" or "executed" by the PHP engine?
Absolutely.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
Post Reply