Page 1 of 2
Bulletproof __autoloader
Posted: Mon Aug 14, 2006 2:57 am
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");
}
}
Posted: Mon Aug 14, 2006 5:24 am
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

Posted: Mon Aug 14, 2006 5:26 am
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");
}
?>
Posted: Mon Aug 14, 2006 5:34 am
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?
Posted: Mon Aug 14, 2006 5:39 am
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:
Shouldn't it be...
Posted: Mon Aug 14, 2006 5:41 am
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

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
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.
Posted: Mon Aug 14, 2006 5:45 am
by volka
What's the point of a new exception object if it is neither used nor thrown not even returned?
Posted: Mon Aug 14, 2006 5:45 am
by Verminox
Jenk wrote:Also, using the line
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.
Posted: Mon Aug 14, 2006 5:51 am
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?
Posted: Mon Aug 14, 2006 5:59 am
by Oren
Jenk wrote:The multiple parenthesis is semantics

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.
Posted: Mon Aug 14, 2006 6:08 am
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?
Re: Bulletproof __autoloader
Posted: Mon Aug 14, 2006 6:27 am
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

Posted: Mon Aug 14, 2006 6:35 am
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.
Posted: Mon Aug 14, 2006 6:44 am
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.

Posted: Mon Aug 14, 2006 9:21 am
by s.dot
astions wrote:Are included files "evaled" or "executed" by the PHP engine?
Absolutely.