ZF-6791: big logical mistake, resource path gets replaced even if they are already existent

Description


    public function addResourceType($type, $path, $namespace = null)
    {
        $type = strtolower($type);
        if (!isset($this->_resourceTypes[$type])) {
            if (null === $namespace) {
                require_once 'Zend/Loader/Exception.php';
                throw new Zend_Loader_Exception('Initial definition of a resource type must include a namespace');
            }
            $namespaceTopLevel = $this->getNamespace();
            $namespace = ucfirst(trim($namespace, '_'));
            $this->_resourceTypes[$type] = array(
                'namespace' => empty($namespaceTopLevel) ? $namespace : $namespaceTopLevel . '_' . $namespace,
            );            
        }

            if (!is_string($path)) {
                require_once 'Zend/Loader/Exception.php';
                throw new Zend_Loader_Exception('Invalid path specification provided; must be string');
            }
            $this->_resourceTypes[$type]['path'] = $this->getBasePath() . '/' . $path;            

        $component = $this->_resourceTypes[$type]['namespace'];
        $this->_components[$component] = $this->_resourceTypes[$type]['path'];
        return $this;
    }

should be changed to


    public function addResourceType($type, $path, $namespace = null)
    {
        $type = strtolower($type);
        if (!isset($this->_resourceTypes[$type])) {
            if (null === $namespace) {
                require_once 'Zend/Loader/Exception.php';
                throw new Zend_Loader_Exception('Initial definition of a resource type must include a namespace');
            }

            if (!is_string($path)) {
                require_once 'Zend/Loader/Exception.php';
                throw new Zend_Loader_Exception('Invalid path specification provided; must be string');
            }
            
            $namespaceTopLevel = $this->getNamespace();
            $namespace = ucfirst(trim($namespace, '_'));
            $this->_resourceTypes[$type] = array(
                'namespace' => empty($namespaceTopLevel) ? $namespace : $namespaceTopLevel . '_' . $namespace,
            );
            
            $this->_resourceTypes[$type]['path'] = $this->getBasePath() . '/' . $path;            
        }

        $component = $this->_resourceTypes[$type]['namespace'];
        $this->_components[$component] = $this->_resourceTypes[$type]['path'];
        return $this;
    }

because this function is supposed to add a resource if it isn't existent.

the problem with it is, it checks the param. path later as it should and even more critical it overwrites the resource PATH even if the resource is already is existent!

best regards

enalog Alex. Hannel

Comments

The requested behaviour seems to be in direct opposition to expected (and current) behaviour as documented in the test case testAutoloaderShouldAllowAddingResettingResourcePaths(Zend_Loader_Autoloader_ResourceTest).

The test case asserts that "re-adding" a resource type with a different path should overwrite the existing path set in the autoloader, the requested behaviour would prevent that use-case.

I need the following before I can move forward on this: * Reproduce case that displays the issue * Expected result (I'm assuming "lack of an error condition") * Actual result (the error condition that occurs)

As I'm not sure what error occurs, nor what conditions trigger it, I can't write a unit test for it at this point. Please update with the above information, and I'll start work on it.

Brenton -- You evidently posted sometime after I'd loaded the page originally, so I missed your bit about the test case in question.

Can you detail why you want to change the behavior? What benefit does this have?

Matthew,

I was saying that I don't believe this is an issue.

Using the code suggested in the initial report causes a failure in the current test suite (testAutoloaderShouldAllowAddingResettingResourcePaths). Which indicates the behaviour that the initial report is trying to "fix" is expected, and thus this is not a bug.

Aha! Excellent -- I'll close as "not an issue" then. Thanks for the clarifications!