Doctrine Common
  1. Doctrine Common
  2. DCOM-20

ClassLoader should check before require()...

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 2.0.0-BETA4
    • Fix Version/s: None
    • Component/s: Class Loading
    • Labels:
      None
    • Environment:
      Ubuntu 10.04

      Description

      I noticed that prior to the require function call in loadClass(), no attempt is made to see if the file can be loaded, or to trap the result of the require attempt.

      This means that when I chain together class loaders on the autoload stack, the first one that specifies a namespace of null effectively terminates the process, preventing any others from being run.

        Activity

        Hide
        Alexander Trauzzi added a comment -

        One solution I propose (and have used in my own class loaders in the past) is to check and see if the file exists and/or is accessible prior to the require() or include().
        It might be done something like this:


        public function loadClass($className)
        {
        if ($this->namespace !== null && strpos($className, $this->namespace.$this->namespaceSeparator) !== 0)

        { return false; }

        if ($this->canLoadClass())

        { require ($this->includePath !== null ? $this->includePath . DIRECTORY_SEPARATOR : '') . str_replace($this->namespaceSeparator, DIRECTORY_SEPARATOR, $className) . $this->fileExtension; return true; }

        return false;

        }

        Another might be to catch any errors thrown by the require statement if possible.

        This effectively guarantees that in the event of a failure, the natural PHP mechanism of the class autoloader can follow up or terminate, resulting in a Class Not Found error, rather than a failed include - which might be potentially misleading??

        Show
        Alexander Trauzzi added a comment - One solution I propose (and have used in my own class loaders in the past) is to check and see if the file exists and/or is accessible prior to the require() or include(). It might be done something like this: — public function loadClass($className) { if ($this->namespace !== null && strpos($className, $this->namespace.$this->namespaceSeparator) !== 0) { return false; } if ($this->canLoadClass()) { require ($this->includePath !== null ? $this->includePath . DIRECTORY_SEPARATOR : '') . str_replace($this->namespaceSeparator, DIRECTORY_SEPARATOR, $className) . $this->fileExtension; return true; } return false; } — Another might be to catch any errors thrown by the require statement if possible. This effectively guarantees that in the event of a failure, the natural PHP mechanism of the class autoloader can follow up or terminate, resulting in a Class Not Found error, rather than a failed include - which might be potentially misleading??
        Hide
        Alexander Trauzzi added a comment -

        I should note that it isn't up to ClassLoader to trigger a failure in class loading, otherwise class instantiation requires every call to the "new" keyword to be prefixed by a check to verify the class is available.

        ClassLoader should for the most part fail silently with the one exception of notifying the autoload stack with the boolean return value. Any other errors force developers to get into the plumbing and breaks the contract between PHP's class loading behaviour and the autoload stack.

        Show
        Alexander Trauzzi added a comment - I should note that it isn't up to ClassLoader to trigger a failure in class loading, otherwise class instantiation requires every call to the "new" keyword to be prefixed by a check to verify the class is available. ClassLoader should for the most part fail silently with the one exception of notifying the autoload stack with the boolean return value. Any other errors force developers to get into the plumbing and breaks the contract between PHP's class loading behaviour and the autoload stack.
        Hide
        Roman S. Borschel added a comment -

        Hi.

        This is by design. We consider the default "contract" for autoloaders inferior as checks for file existence are unnecessary and expensive in 99.9% of the classes loaded.

        The ClassLoader of the Common project separates the responsibilities for a) checking whether a class can be loaded and b) actually loading the class.

        Furthermore, it is by design that each class loader is responsible for classes in a certain namespace. Only 1 classloader can thus be responsible for a single class. If a class loader is asked to load a class, and he is responsible for loading that class, any failure to do so is a fatal error. A missing class that is requested to be loaded is a fatal error. Silently ignoring such cases is a very bad approach from our point of view and it requires slow(er) implementations due to checks for file existence.

        If you specify NULL as the namespace, that means this class loader should be responsible for all classes. Thus it is obvious that a class loader that is configured as such can only be used a) as the only one or b) at the end of the autoload stack (as the last one).

        This approach to class loading and this implementation is designed for best efficiency.

        If you do not like this approach to class loading you are free to use any other implementations, Doctrine classes don't care which autoloader loads them.

        ps. return values don't matter at all in autoload functions for PHP, they only matter if you invoke them directly. The SPL autoload stack continues to invoke the next loader until the class is defined/loaded, then it stops, irrespective of what a loader returns. It only matters whether the class definition is now available to PHP.

        Show
        Roman S. Borschel added a comment - Hi. This is by design. We consider the default "contract" for autoloaders inferior as checks for file existence are unnecessary and expensive in 99.9% of the classes loaded. The ClassLoader of the Common project separates the responsibilities for a) checking whether a class can be loaded and b) actually loading the class. Furthermore, it is by design that each class loader is responsible for classes in a certain namespace. Only 1 classloader can thus be responsible for a single class. If a class loader is asked to load a class, and he is responsible for loading that class, any failure to do so is a fatal error. A missing class that is requested to be loaded is a fatal error. Silently ignoring such cases is a very bad approach from our point of view and it requires slow(er) implementations due to checks for file existence. If you specify NULL as the namespace, that means this class loader should be responsible for all classes. Thus it is obvious that a class loader that is configured as such can only be used a) as the only one or b) at the end of the autoload stack (as the last one). This approach to class loading and this implementation is designed for best efficiency. If you do not like this approach to class loading you are free to use any other implementations, Doctrine classes don't care which autoloader loads them. ps. return values don't matter at all in autoload functions for PHP, they only matter if you invoke them directly. The SPL autoload stack continues to invoke the next loader until the class is defined/loaded, then it stops, irrespective of what a loader returns. It only matters whether the class definition is now available to PHP.

          People

          • Assignee:
            Roman S. Borschel
            Reporter:
            Alexander Trauzzi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: