Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0-BETA3
    • Component/s: None
    • Labels:
      None

      Description

      The problem is that we need to load all annnotation classes before we read annotations from the target class.
      So we need to require_once them at target class source, or just before reading.
      It's not cool.

      The reason of this, is a fix of bug #77
      http://www.doctrine-project.org/jira/browse/DDC-77
      and class_exists(..., false).

      Solutions:
      1. Filter DocBlockTags, and think that all other @foo string is annotations (don't check with class_exists)
      2. Use class_exists(..., true) but supress warnings with @ and try/catch operators.

      May be there are some another, better solutions.

        Activity

        Hide
        Roman S. Borschel added a comment -

        The class_exists check can maybe be removed altogether since is_subclass_of is no longer used.

        Show
        Roman S. Borschel added a comment - The class_exists check can maybe be removed altogether since is_subclass_of is no longer used.
        Hide
        Tim Nagel added a comment -

        Would removing the second parameter to class_exists solve the problem sufficiently?

        I would thing you would have to leave the class_exists check in, otherwise it will try to parse annotations that do not resolve to class names?

        Show
        Tim Nagel added a comment - Would removing the second parameter to class_exists solve the problem sufficiently? I would thing you would have to leave the class_exists check in, otherwise it will try to parse annotations that do not resolve to class names?
        Show
        Tim Nagel added a comment - Changed: http://github.com/merk/common/commit/152fdecf11b7999262df0410b5584951de5191b3
        Hide
        Roman S. Borschel added a comment -

        The fundamental problem with class_exists($blub) (which is the same as class_exists($blub, true) is that it basically requires a class loader / autoloader that fails silently when a class file does not exist, which means it must use costly file_exists checks before loading any class. The Doctrine\Common\ClassLoader does (on purpose) not check for file existance. If the class loader is responsible for a particular class and it is requested to load it, failing to do so is (and should be) a fatal error.

        We consider the fact that two responsibilities are mixed here, a) (auto)loading a class file and b) checking for the existance of a class (file), to be a design flaw in class_exists / autoloading. It means that you are required to check for file existance in a class loader and fail silently if the file does not exist which is completely unnecessary in 99% of the cases. Compare how many times class_exists(..., true) is usually used in a single request and how many classes, in total, are usually loaded per request. So just for the case that some code might use an occasional class_exists(..., true) check you have to check each single file for existance before loading it (and you must fail silently, it might be a class_exists check!).

        Even more, in a case where you really only want to check for class (file) existance, but you dont actually want to load it, you can't! class_exists(..., true) when the class file exists results in loading the class file, even if its completely unnecessary.

        We are planning to come up with a better approach that separates the 2 concerns.

        This is related to DCOM-7.

        Show
        Roman S. Borschel added a comment - The fundamental problem with class_exists($blub) (which is the same as class_exists($blub, true) is that it basically requires a class loader / autoloader that fails silently when a class file does not exist, which means it must use costly file_exists checks before loading any class. The Doctrine\Common\ClassLoader does (on purpose) not check for file existance. If the class loader is responsible for a particular class and it is requested to load it, failing to do so is (and should be) a fatal error. We consider the fact that two responsibilities are mixed here, a) (auto)loading a class file and b) checking for the existance of a class (file), to be a design flaw in class_exists / autoloading. It means that you are required to check for file existance in a class loader and fail silently if the file does not exist which is completely unnecessary in 99% of the cases. Compare how many times class_exists(..., true) is usually used in a single request and how many classes, in total, are usually loaded per request. So just for the case that some code might use an occasional class_exists(..., true) check you have to check each single file for existance before loading it (and you must fail silently, it might be a class_exists check!). Even more, in a case where you really only want to check for class (file) existance, but you dont actually want to load it, you can't! class_exists(..., true) when the class file exists results in loading the class file, even if its completely unnecessary. We are planning to come up with a better approach that separates the 2 concerns. This is related to DCOM-7 .
        Hide
        Roman S. Borschel added a comment -

        The recommended approach for annotations by the way, (recommended by me), is to load them manually via a require call. The ORM does exactly this. If you rely on class_exists(..., true) and silently failing autoloaders, this becomes costly considering how often class_exists would be invoked while parsing annotations.

        Show
        Roman S. Borschel added a comment - The recommended approach for annotations by the way, (recommended by me), is to load them manually via a require call. The ORM does exactly this. If you rely on class_exists(..., true) and silently failing autoloaders, this becomes costly considering how often class_exists would be invoked while parsing annotations.

          People

          • Assignee:
            Roman S. Borschel
            Reporter:
            Kirill chEbba Chebunin
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: