Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Varien_Io_File has an erratic implementation of cd #66

Closed
masmrlar opened this issue Feb 10, 2016 · 4 comments · Fixed by #1020
Closed

Varien_Io_File has an erratic implementation of cd #66

masmrlar opened this issue Feb 10, 2016 · 4 comments · Fixed by #1020
Labels
bug Component: lib/Varien Relates to lib/Varien

Comments

@masmrlar
Copy link

    /**
     * Change current working directory
     *
     * @param string $dir
     * @return boolean
     */
    public function cd($dir)
    {
        if( is_dir($dir) ) {
            @chdir($this->_iwd);
            $this->_cwd = realpath($dir);
            return true;
        } else {
            throw new Exception('Unable to list current working directory.');
            return false;
        }
    }

The thrown exception is superflous and breaks the interface.

@tmotyl tmotyl added the bug label Mar 15, 2017
@drobinson
Copy link
Collaborator

drobinson commented Apr 1, 2017

I would say either we remove the exception or add @throws and insert error catching in all places in core using cd().

Seems there's no error checking now for any of those usages anyway (which seems like a pretty big oversight) so it may be best to add it in all of those places now...

Do you have thoughts on this @masmrlar?

Flyingmana pushed a commit that referenced this issue Jul 15, 2018
@seansan
Copy link
Contributor

seansan commented Sep 5, 2018

@Flyingmana @masmrlar Is this still an issue? (I see a pull request)

@Flyingmana
Copy link
Contributor

The referred PullRequest comes from a different repository, and is related to a different Issue (updating Magento)

For this Topic here, I would instead of trying to change anything there, put out the recommendation to use a better supported library for File/Directory Access.
We dont know, which code and impmenentations out there will break in unexpected ways because of a removed exception.
So I would suggest adding a @throw

Also I would not add any error checking at the moment, as this would mean we expect this error there, but we very likely dont know what others expect if this error happens. Also it would change the current error behaviour a lot.

Means: if we have an exception there, and it bubbles up to the top, than it has very likely a good reason for this.

@mattdavenport
Copy link
Contributor

Just pushed a PR for this one. I also removed return false, as it was an unreachable statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Component: lib/Varien Relates to lib/Varien
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants