Skip to content

Fix invalid asset resolution with numeric theme_id #7686

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

Closed
wants to merge 1 commit into from

Conversation

adragus-inviqa
Copy link
Contributor

@adragus-inviqa adragus-inviqa commented Dec 5, 2016

There are cases in which the theme ID is passed to the asset resolution, but the processing in this case doesn't handle numbers correctly. This has the direct effect of the system not finding valid template files, for example.

One case of a passed numeric theme ID is saving a widget instance, to create the layout update XML. Because the theme model is invalid, M2 will always resolve the template relative to vendor/ (M2), never for third parties/themes.

So the call would be equivalent to - say - $this->getThemeProvider()->getThemeByFullPath('frontend/5'), which, as much as I tried to think that there's some magic coversion happening somewhere, it wasn't the case, and the theme model was never found.

This bug was hidden this much time because the validation of the theme model is done incorrectly, assuming it can sometimes be falsy, which is not the case, as the model is a result of $themeCollection->getFirstItem(), which will always be an object (so truthy). The ID needs to be checked instead.

I'm possibly very wrong in my PR, or this is a pretty serious bug.

There are cases in which the theme *ID* is passed to the asset resolution, but the processing in this case doesn't handle numbers correctly. 

This bug was hidden this much time because the validation of the theme model is done incorrectly, assuming it can sometimes be falsy, which is not the case, as the model is a result of `$themeCollection->getFirstItem()`, which will always be an object (so truthy).
@adragus-inviqa
Copy link
Contributor Author

The PHP 7 unit test suite has failed (https://travis-ci.org/magento/magento2/jobs/181459425), because of this wrong assumption. It'll never be null; not even the phpdoc of \Magento\Framework\View\Design\Theme\ThemeProviderInterface::getThemeByFullPath() specifies null.

Unfortunately, I don't have the time to fix the tests.

@waynetheisinger
Copy link
Contributor

To pass the unit tests you need to test for nulls and objects:

if ($theme) {                                                                                       
    $params['themeModel'] = $this->getThemeProvider()->getThemeByFullPath($area . '/' . $theme);    
    if (!$params['themeModel']) {                                                                   
    if (is_numeric($theme)) {                                                                       
        $params['themeModel'] = $this->getThemeProvider()->getThemeById($theme);                    
    } else {                                                                                        
        $params['themeModel'] = $this->getThemeProvider()->getThemeByFullPath($area . '/' . $theme);
    }                                                                                               
    if (  is_null($params['themeModel'])                                                            
          || ( is_object($params['themeModel'])                                                     
          && !$params['themeModel']->getId())                                                       
    ) {                                                                                             
        throw new \UnexpectedValueException("Could not find theme '$theme' for area '$area'");      
    }                                                                                               
} elseif (empty($params['themeModel'])) {                                                           

@okorshenko okorshenko self-assigned this Jun 30, 2017
@okorshenko okorshenko added this to the June 2017 milestone Jun 30, 2017
@okorshenko
Copy link
Contributor

okorshenko commented Jun 30, 2017

Hi @adragus-inviqa Thank you for your PR. Unfortunately we can not accept it in current state. We executed internal tests and a lot of tests failed. I reported internal ticket MAGETWO-70402 to investigate the issue.
Closing this PR.

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.

3 participants