2
\$\begingroup\$

I'm writing a simple front controller, index.php is the main file.
In my index.php, first thing loaded is bootstrap.php via require_once().

I'm deliberating over two ways to load bootstrap.php:

A)

require_once 'bootstrap.php';

B)

if ( is_readable('bootstrap.php') )
  {
    require_once 'bootstrap.php';
  }
else
  {
    exit( 'Sorry, we have a temporary issue. We are aware and it should hopefully be resolved soon. Please try again in 10 mins' );
  }

I see most well know frameworks (Codeigniter etc) just use A, which is neat and simple. If the file is not found, A provides:

  1. A means for the script to halt via the require_once() returning a fatal error
  2. Nothing else graceful or useful

However I'm tempted to use B as if the file is not found, B will:

  1. Provides a basic message to the screen, no white screen
  2. Avoids a fatal error which A will produce
  3. B seems more frameworky and robust, controlling what happens via the application rather than just relying upon PHP which falls back on a fatal error

This (A or B) is only used a couple of times, once to load bootstrap.php and then twice within bootstrap.php to load SPL autoloader and error management (etc). Thus dealing with require() and any errors/failures throughout the rest of the system are handled more gracefully (with SPL and error management) and so neither A or B are needed.

However, I'm trying to make the entire system tight and well coded, rather than "it's only used a few times, meh that'll do".


I'm personally drawn towards using B, but I don't want opinions on which you would use for personal reasons, there will be some logical reasons or rationale I have not considered which I'd be grateful for you to throw at me.

Such as B has too much overhead (likely negligible but just an example), major frameworks use A because XYZ, etc.

Questions:

  1. Is A flimsy and a bit unprofessional by just allowing code within a framework to use PHP fall back to a fatal error? When a framework should manage things in the best possible way.

  2. Is A acceptable just allowing a fatal error?

  3. Is B over the top, too much code FWIW, and not really necessary?

  4. Is B much better because it removes a stinky fatal error, and because it controls how the application degrades rather than relying on a PHP fatal error to halt?

\$\endgroup\$
  • \$\begingroup\$ If you are making the control if the file exists, why don't you use include_once instead? It (possibly) is a bit faster and the only reason to use require will be invalid since you did the check before on your own. (As a side-note, some people might prefer to use die instead of exit.) \$\endgroup\$ – Ismael Miguel Nov 3 '14 at 15:30
  • \$\begingroup\$ @IsmaelMiguel Good idea thanks. I'll leave this Q as it is, making A and B have additional differences between them might cloud what I'm asking. \$\endgroup\$ – James Nov 3 '14 at 15:37
  • \$\begingroup\$ I don't think it will confuse anyone: basically you are re-implementing require_once but instead of letting php throw an error, you handle it on your way and show a more readable message. I'm not saying "You are wrong, change now!", but I'm just saying that leaving like that won't be beneficial but I leave that judgment to you. \$\endgroup\$ – Ismael Miguel Nov 3 '14 at 15:42
  • \$\begingroup\$ Hmm, actually having thought about it, I prefer having the second layer of ensuring a non-existent file is not loaded and script is halted. While it's unlikely to get past the if, there are other things at play with PHP which might make this occur. I have made performance a key point in building the framework, however not when it impedes on security and stability/robustness setup. And the difference between require_once() and include_once() is arguably insignificant (especially as I'm only using it a few times). But thanks for the thought! \$\endgroup\$ – James Nov 3 '14 at 15:50
  • 1
    \$\begingroup\$ @IsmaelMiguel I appreciate your suggestions, but suppressing errors is 99% of the time hacky. I'd be introducing code in my framework where I know two warnings are potential, and we're supposed to fix these not introduce them ;). Include and require once take microseconds to run. I don't know what the overhead of error suppression is, but it must surely be worse than just including/requiring? So the suggestion is slower, messy, not as readable, etc. Thanks tho \$\endgroup\$ – James Nov 3 '14 at 16:23
3
\$\begingroup\$

The short answers to your questions:

A1: Is A flimsy and a bit unprofessional?
No. It's not at all "flimsy" and not unprofessional. The difference between an include and a require is just that: If you require something, your code needs that contents to be there, or it can't work. If require doesn't work, that represents a fatal error, which is just what require(_once)emits.

A2: Is A acceptable just allowing a fatal error?
For reasons explained above: yes, yes it is. Of course fatal errors due to missing require files are not acceptable in production, but for code to make it to the production stage, the requires should be absolutely bug-free.

A3: Is B over the top, too much code FWIW, and not really necessary?
Yes and No. B is over the top, it is too much code, but at the same time it doesn't check enough. your is_readable will check for a readable file in the same directory as the script that calls is_readable. include and require on the other hand will look for files in the current directory and all of the paths in the include_path. Check out which directories this includes by using

var_dump(
    explode(
        PATH_SEPARATOR,
        get_include_path()
    )
);

Not that the include path isn't a constant: you can add directories to it at runtime:

set_include_path('/home/me/php/classes/'.PATH_SEPARATOR.get_include_path());

So to check if the require calls will fail, you'd have to check all of the directories in the include path, for readable files, and only then will an include or a require fail. So your code is more verbose, but doesn't check enough.

A4: Is B much better because it removes a stinky fatal error, and because it controls how the application degrades rather than relying on a PHP fatal error to halt?
Absolutely NOT: a "stinky" fatal error or a die or exit call... I don't really see much difference (other than the one you seem to consider cleaner generating a less clear error message). It doesn't really control how the application degrades at all: it merely halts it. There's no predefined sequence by which the runtime is unloaded, connections are being closed and resources are freed. Internally speaking, the differences between your approach and the fatal error are nil.

In addition to all of this, I'd like to rant a bit about some of your statements:

"Such as B has too much overhead (likely negligible but just an example)..."

Well overhead is definitely one reason not to use B. As I've explained: to check if the file is readable or not, you'll have to iterate over all directories that are in the include_path. Each time calling is_readable for each file that you're trying to include. Given that most frameworks const of hundreds or even thousands of files, the overhead this causes is in no way negligible. It'll be quite substantial actually. The one thing that you'll want to avoid if you have to maintain a high-traffic application is pointless disk IO, because Disk IO has a direct impact on your performance. And in this case, it can be avoided easily by just spending a bit more time debugging, and making sure the requires all work.

"B seems more frameworky and robust, controlling what happens via the application rather than just relying upon PHP which falls back on a fatal error"

Now it is true that frameworks handle nearly all errors/exceptions that internal components throw at some point or another. An uncaught exception is generally handled by an exception handler that is registered by the framework itself, which then churns out a nice render of this exception, complete with stack-trace and all that. So I can understand why you think that, instead of letting a fatal error propagate, preventing it and displaying a more "tidy" error message is what frameworks could/should do. But there are 2 things you're not taking into account:

  1. Frameworks consist of hundreds or thousands of files. Classes that are autoloaded are, at some point, required. If a framework were to loop over the include paths, using is_readable over and over again, even for components of the framework itself, that would become a significant bottleneck. Especially considering the fact that stable frameworks rarely (I haven't seen it happen) encounter missing includes/requires. A framework worth using contains no broken requires, and that's an end of it. That's why they'll never check the files they require.
  2. When frameworks handle things like uncaught exceptions, they often do a couple of things more than just presenting a human-readable error message. They allow for user code to hook into that event, to log any problems that are encountered. They could use the exception to prevent a fatal crash (DB connection goes away, reconnect and retry). But that's more of an "in-theory" use-case. What you do is actually handling a real FATAL, possibly not even code-related error. That's not what frameworks do. They don't catch syntax errors, they don't handle missing files or wrong permission settings. And they certainly don't brute-force attack the DB when the connection params (username, password) are incorrect.

So if, after all this, you still feel that apprach B is more frameworky, then why not go one step further, and also check syntax errors prior to the require? No? Because it would be silly? Yes, it would be. So just require or include the files: a fatal error is an error that needs to be fixed ASAP. The best way to communicate the urgency to devs is by displaying them the raw fatal error. It stresses the fact that there's a serious issue that needs to be fixed. Don't pamper them too much, because you'll end up with code that just looks silly:

/**
 * This is just an example of how far you can take things
 * If you ever see someone using code like this, shout at that person
 * NEVER USE THIS AWFUL FUNCTION
 */
function kittyGlovesRequire($file)
{
    $paths = explode(PATH_SEPARATOR, get_include_path());
    $exists = false;
    foreach ($paths as $path)
    {
        if (file_exists(realpath($path.'/'.$file))
        {
            $exists = true;
            if (is_readable(realpath($path.'/'.$file))
                $exists = realpath($path.'/'.$file);
            break;
        }
    }
    if ($exists === false)
        exit($file.' Could not be found, please check the name or create the file');
    if ($exists === true)
        exit($file.' exists, but is not readable. Check permissions, file corruption or...');
    if (!php_check_syntax(realpath($path.'/'.$file, $msg))
        exit($file.' exists, and is readable, but contains syntax errors: '.$msg);
    require realpath($path.'/'.$file);
}

Not only is it more verbose, it's also slower, and makes debugging just an awful lot harder than a good 'ol:

require 'path/to/file.php';
\$\endgroup\$
  • \$\begingroup\$ When I originally read your answer, everything I "understood" at the time made sense. I've learned a lot more since I posted, and having just read through your answer again it all very much makes sense. I just wanted to say thanks again for taking the time to explain so much, it sank in "somewhere" in the old grey matter. You certainly helped push me on a better path for making more logical decisions (etc). Thanks again! \$\endgroup\$ – James Jun 26 '15 at 13:17
5
\$\begingroup\$

I don't recommend B:

  • A file being readable does not guarantee that it can be successfully included. The specified path could be a directory (weird and unlikely, but possible), or it might point to a file with a syntax error.
  • is_readable() does not check the include_path the way require_once would.

Supposing that you could write a function to ensure that it is a regular file, and that it does not contain syntax errors, and checked all directories in the search path, you would have written a significant amount of code that reimplements PHP's built-in functionality. The built-in functionality isn't broken to begin with. If you say that a file is required, and PHP is unable to include it, that's a fatal error. What is the point of prettifying a fatal error and suppressing the error code?

If the file to be loaded is not entirely essential, or if you want your script to commit suicide on its own terms, a better strategy might be to check the return value of include_once.

\$\endgroup\$
2
\$\begingroup\$

It's unclear to me why you would want to display a message to the screen that explains why the script is not working. There are reasons why servers usually just display a "Server Error" page in that case. Your users don't want to know how your server is broken. They just want to know that it is broken and ideally when it will be fixed. Unless you want them to do something, there's no reason to tell them what's actually wrong.

Another issue is that your error message may communicate something to black hats. If there was a reason to tell white hats, then it might be worth it.

All that said, if you really want to do this, you can do better than is_readable:

include_once('bootstrap.php') or die('A core system file cannot be found. [exit]');

This detects any error in including, not just the specific problem of not being able to read the file.

A final issue is that requires are processed at parse time normally. By adding the is_readable check, you force it to wait until run time to process the require (because the code might not run). require has two advantages over include_once. First, it processes at compile time, saving execution time and avoiding processing part of the script before failing. Second, it automatically creates a fatal error if there is a failure. If you aren't using those, then why use require?

Answers to your questions

  1. A require should never fail; therefore, you should never have to handle this situation. If it does happen, this is not a recoverable situation. There's no point in managing the failure if you can't mitigate it.

  2. Yes.

  3. Yes.

  4. No, because you still halt. If you could continue, then maybe. But even then, switching from require to include_once would be the way to go. You can handle a broader range of failure conditions that way.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the opinion. I want to display a message for end user courtesy. If you go to a website & see a white screen, do you hang around for 10 mins, refreshing? Or leave immediately and go elsewhere? So displaying "Sorry, we are having some technical difficulties. We are aware, please try again in 10 mins" might make you come back. It might not, but it's at least something rather than nothing. The file being included is part of the core and shouldn't ever be missing, but things can happen and I just want the system to gracefully handle all potentials, rather than white screen. \$\endgroup\$ – James Nov 5 '14 at 11:06
  • \$\begingroup\$ Additionally, or die() wont work with an include_once(). \$\endgroup\$ – James Nov 5 '14 at 12:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.