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

Throw exception for invalid (missing) template in dev mode #8119

Conversation

convenient
Copy link
Contributor

@convenient convenient commented Jan 12, 2017

In developer mode php notices/warnings etc are promoted to being exceptions. This means the developer is a bit more diligent with cleaning up any messes as they occur. This does not seem to apply to layout declarations with invalid templates.

I noticed this error on a Magento 1 client which is filled with "Not valid template file", I checked in M2 and this error seems to persist. Developer mode does not promote invalid template errors to being an exception.

This change should make sure developers do not leave any dead templates in their layout.xml. It seems like an easy win to me.

Don't we all want clean log files? :)

In developer mode php notices/warnings etc are promoted to being exceptions. This means the developer is a bit more diligent with cleaning up any messes as they occur.

I noticed this error on a Magento 1 client which is filled with "Not valid template file", I checked in M2 and this error seems to persist.

This change should make sure developers do not lead any dead templates in their layout.xml.
@vrann vrann self-assigned this Mar 1, 2017
@vrann vrann added this to the March 2017 milestone Mar 1, 2017
$errorMessage = "Invalid template file: '{$templatePath}' in module: '{$this->getModuleName()}'"
. " block's name: '{$this->getNameInLayout()}'";
if ($this->_appState->getMode() === \Magento\Framework\App\State::MODE_DEVELOPER) {
throw new \InvalidArgumentException($errorMessage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use \Magento\Framework\Exception\ValidatorException instead

@vrann
Copy link
Contributor

vrann commented Mar 1, 2017

@convenient Thanks for the contribution, it definitely makes sense to output exception rather way for the developer so there is insensitive to fix it.

Added comment to the code review, please process.

@convenient
Copy link
Contributor Author

Updates pushed, @vrann Is that alright mate?

@vrann
Copy link
Contributor

vrann commented Mar 2, 2017

@convenient perfect, thanks!

@convenient
Copy link
Contributor Author

Awesome thanks @vrann.

I wonder how much screaming there is going to be

Boo the latest magento version breaks when using developer mode what happened?

The greater good!

@vrann
Copy link
Contributor

vrann commented Mar 2, 2017

@convenient wait, what was the intent of PR? Checking \Magento\Framework\View\Element\Template\File\Validator::isValid method, it will check only for template being in the right location. Have you meant to throw the exception in case if a template has syntax errors?

public function isValid($filename)
    {
        $filename = str_replace('\\', '/', $filename);
        if (!isset($this->_templatesValidationResults[$filename])) {
            $this->_templatesValidationResults[$filename] =
                ($this->isPathInDirectories($filename, $this->_compiledDir)
                    || $this->isPathInDirectories($filename, $this->moduleDirs)
                    || $this->isPathInDirectories($filename, $this->_themesDir)
                    || $this->_isAllowSymlinks)
                && $this->getRootDirectory()->isFile($this->getRootDirectory()->getRelativePath($filename));
        }
        return $this->_templatesValidationResults[$filename];
    }

@vrann
Copy link
Contributor

vrann commented Mar 2, 2017

@convenient or is that exactly what happens when you leave the dead template in the layout? Yeah, then we might hear boos :) (in developer mode only)

@convenient
Copy link
Contributor Author

I meant the latter. I'm wondering how many dead templates exist in m2 websites today, quietly logging away never being cleaned up.

We'll find out when this is released and the developers start to complain I guess.

@convenient convenient changed the title Throw exception for invalid template in dev mode Throw exception for invalid (missing) template in dev mode Mar 2, 2017
@okorshenko okorshenko self-assigned this Mar 9, 2017
@magento-team magento-team merged commit de4f2c1 into magento:develop Mar 11, 2017
@okorshenko
Copy link
Contributor

@convenient thank you for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants