Issues

ZF-4810: Zend_Controller_Request_Http::setBasePath() does not set valid basePath on Windows machine.

Description

Zend_Controller_Request_Http::setBasePath() does not set valid basePath on Windows machine if URL has form: http://anyhost/index.php and argument for this method is null (basePath === null).

Example: require_once 'Zend/Controller/Request/Http.php'; $request = new Zend_Controller_Request_Http; die(print_r($request->getBasePath(), 1)); This example returns '\' string.

For fix this bug, need replace $basePath = dirname($baseUrl); to $basePath = str_replace('\', '/', $baseUrl); in Zend_Controller_Request_Http::setBasePath() method (two backslashes not see here).

Complete method code should be: public function setBasePath($basePath = null) { if ($basePath === null) { $filename = basename($_SERVER['SCRIPT_FILENAME']);

        $baseUrl = $this->getBaseUrl();
        if (empty($baseUrl)) {
            $this->_basePath = '';
            return $this;
        }

        if (basename($baseUrl) === $filename) {
            $basePath = str_replace('\\', '/', dirname($baseUrl));
        } else {
            $basePath = $baseUrl;
        }
    }

    $this->_basePath = rtrim($basePath, '/');
    return $this;
}

Comments

I have confirmed that this fix works correctly.

/**
 * Set the base path for the URL
 *
 * @param string|null $basePath
 * @return Zend_Controller_Request_Http
 */
public function setBasePath($basePath = null)
{
    if ($basePath === null) {
        $filename = basename($_SERVER['SCRIPT_FILENAME']);

        $baseUrl = $this->getBaseUrl();
        if (empty($baseUrl)) {
            $this->_basePath = '';
            return $this;
        }

        if (basename($baseUrl) === $filename) {
            $basePath = dirname($baseUrl);
            /*fix for windows*/
            $basePath = str_replace('\\','/',$basePath);
        } else {
            $basePath = $baseUrl;
        }
    }
    $this->_basePath = rtrim($basePath, '/');
    return $this;
}

Applied the fix as proposed in my last comment. Committed to standard/trunk today.

That commit has to be reverted. It has an echo and the str_replace is more like a hack and should check for OS == WIN.

I just did a rollback.

Just replacing all backslashes in paths is no solution, as test\test is a valid directory name (at least on linux i can approve that).

Sorry about the echo. Here is a new try, which will work if $_SERVER['HTTP_USER_AGENT'] is set.


    public function setBasePath($basePath = null)
    {
        if ($basePath === null) {
            $filename = basename($_SERVER['SCRIPT_FILENAME']);

            $baseUrl = $this->getBaseUrl();
            if (empty($baseUrl)) {
                $this->_basePath = '';
                return $this;
            }

            if (basename($baseUrl) === $filename) {
                $basePath = dirname($baseUrl);
                /* fix for windows, if detectable in HTTP_USER_AGENT */
                if(isset($_SERVER['HTTP_USER_AGENT']) and stristr($_SERVER['HTTP_USER_AGENT'], 'win') ){
                    $basePath = str_replace('\\','/',$basePath);
                }
            } else {
                $basePath = $baseUrl;
            }
        }
        $this->_basePath = rtrim($basePath, '/');
        return $this;
    }

This looks to me as an improvement at least, if not a complete fix, because of the depency on the server var to be set.

Forget about my last comment. It checks the clients' os and probably isn't even safe. I am looking for something like $_SERVER['WINDIR'] to evaluate. Do not know if it is set on linux usually. I would expect not. Anyone?

This should do the trick:

    /**
     * Set the base path for the URL
     *
     * @param string|null $basePath
     * @return Zend_Controller_Request_Http
     */
    public function setBasePath($basePath = null)
    {
        if ($basePath === null) {
            $filename = basename($_SERVER['SCRIPT_FILENAME']);

            $baseUrl = $this->getBaseUrl();
            if (empty($baseUrl)) {
                $this->_basePath = '';
                return $this;
            }

            if (basename($baseUrl) === $filename) {
                $basePath = dirname($baseUrl);
                /* fix for windows, if detectable in HTTP_USER_AGENT */
                if( isset($_SERVER['WINDIR']) ){
                    $basePath = str_replace('\\','/',$basePath);
                }
            } else {
                $basePath = $baseUrl;
            }
        }
        $this->_basePath = rtrim($basePath, '/');
        return $this;
    }

Because my last commit was premature, I will be more carefull now. I will attach two patches to this issue, one holds the testfunction and the other the actual patch to Http.php

For OS-identification, use the constant PHP_OS, See first unit test of Zend_ProgressBar_Adapter_Console to see how it works.

Another thing is, that you should really read the coding standard definitions in the manual first and correct your code according to this. Also please correct the comment you inserted there, and add the issue number to which the fix refers to.

As for the thing with the backslashes in directory names: I just tested it on windows, and creating directories with a backslash in the name IS possible, tho not directly through the windows API. Check back with Matthew if you need to do any additional changes in the main code logic.

If that is done, re-add the new patches and I will validate them for you again.

Not yet resolved.

Matthew: Please check if the patch and test-patch are satisfactory.

new patch

new test patch

Basically it looks fine now, tho here my comments:

testGetBasePathsEmptyStringIFNoneSet() is a windows-only test, so should be skipped on other systems. As for the backslash thing: It is kinda unusual and hacky, that one exists in windows enviroments, so you are should just replace all backslashes with slashes (sorry for that disinformation). So you can also remove the second test again.

As for the coding standard, please insert one blank line before your if-block, and for the docblocks in the tests: There is no @group or @descr doc tags, write the description without @descr and for the issue number use @see.

After that, everything's fine.

Well, I am glad you can more or less approve of it now.

As for skipping the test, I am not sure. The test should be valid on all other systems too, if it is not, there is something terribly wrong. But if you insist on skipping, do you have a tip regarding phpunit and how to skip on the PHP_OS check?

I am glad my second test convinced you about the backslashes so that all backslashes may be replaced.

@group is in the contributors guide and also in some tests written by others, so either is wrong: you or the wiki. Please find out who is right.

I will remove @descr and add the blank line before if (had not come to that section yet with my reading of the coding standards)

Hm well yeah you are probably right about that the test should success on all systems, so well, let it this way. If you are however interested on how to skip on certain OS, see the first test case of Zend_ProgressBar_Adapter_Console again.

About the @group thingy: well, If it is also in other tests then well, I won't complain about it. Yet there's not real standard about the tests anyway, so who cares ..

On second thought, I think it was in the mailinglist where I first saw the @group and in an existing test. But I have replaced it with @see, because I am running out of time and I will just have to trust you in this :).

Have added new patches for both Http.php en HttpTest.php.

set issue type from bug to patch

Alright I'm fine with this. commit it and close the ticket. Good job.

Set to resolved, is now in svn.

Changing issues in preparation for the 1.7.0 release.