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

[Theming] setting version and parent in theme.xml vs composer.json #840

Closed
Vinai opened this issue Dec 19, 2014 · 12 comments
Closed

[Theming] setting version and parent in theme.xml vs composer.json #840

Vinai opened this issue Dec 19, 2014 · 12 comments

Comments

@Vinai
Copy link
Contributor

Vinai commented Dec 19, 2014

According to the documentation at http://devdocs.magento.com/guides/v1.0/frontend-dev-guide/themes/theme-create.html#fedg_create_theme_how-to_declare the theme should be registered using a theme.xml file.
From the next paragraph it seems that the theme composer.json file is optional and only required when then theme should be distributed and installable via composer.

Not too long ago creating a theme with just a theme.xml worked.
In the current release Magento ver. 0.42.0-beta1 however both the theme.xml AND the composer.json files are required.
There is no indication that is the case however. If one of the two files is missing, the theme simply does not show up in the admin.

This happens because the title and media attributes are read from the theme.xml and the version and parent are read from the composer.json.

Here the excerpt from \Magento\Framework\Config\Theme::_extractData():

        if (!empty($configContent)) {
            $dom = new \DOMDocument();
            $dom->loadXML($configContent);
            // todo: validation of the document
            /** @var $themeNode \DOMElement */
            $themeNode = $dom->getElementsByTagName('theme')->item(0);
            $data['title'] = $themeNode->getElementsByTagName('title')->item(0)->nodeValue;
            /** @var $mediaNode \DOMElement */
            $mediaNode = $themeNode->getElementsByTagName('media')->item(0);
            $previewImage = $mediaNode ? $mediaNode->getElementsByTagName('preview_image')->item(0)->nodeValue : '';
            $data['media']['preview_image'] = $previewImage;
        }

        if (!empty($composerContent)) {
            $json = json_decode($composerContent);
            $package = new Package($json);
            $data['version'] = $package->get('version');
            $parents = (array)$package->get('require', '/.+\/theme-/');
            $parents = empty($parents) ? null : array_keys($parents);
            $data['parent'] = empty($parents) ? null : array_shift($parents);
        }

I would prefer to simply do a pull request, but I'm unsure about the intended behaviour. Is this a bug or is it the intended behavior?
The theme.xsd indeed only references the title and media types.

Probably the reasoning behind moving the version and the parent theme into the composer.json file was that otherwise both files might become inconsistent. That makes sense, but at least to me it is ugly to require a composer.json file, even if the theme should not be distributed.

I think the most developer friendly process would be to keep the composer.json optional.

Please allow setting the version and the parent via the theme.xml file.
Then merge the composer.json settings.
Should the version or parent be set to a different value in the files, throw an exception.

This would be easy to implement, too.


On a side note:
The validators logging a message

'Field can't be empty' in .../app/code/Magento/Core/Model/Theme.php:326

is not very helpful without even the name of the field that can't be empty....

@nyov
Copy link

nyov commented Dec 19, 2014

+1
Hadn't even noticed yet, but I second that. Being required to have some 3rd party boilerplate code like a composer.json in a theme sure doesn't look like the elegant way.

Please allow setting the version and the parent via the theme.xml file.
Then merge the composer.json settings.

That seems a sound idea.

@alankent
Copy link

Early on the design constraint was runtime of a store should not read from composer.json files so use of Composer is available but optional. This code violates that constraint so is a defect. The reason was also so we could migrate easily to a new packaging system in the future.

However, it is clear Composer is now mandatory and central to Magento. For dev beta we are saying "use git clone to play with the code". In production (at GA) it will be "only ever use composer so you know the exact version of what you are running, never git clone mainline".

I am going to investigate internally, but welcome any thoughts on whether you think its better to retain the original design constraint and potentially have some duplicated information (like version number) in composer.json and theme.xml or its better to only have the data once (but have a built in dependence on Composer).

I might write a quite note blog on the Composer interaction/model to expand.

@Vinai
Copy link
Contributor Author

Vinai commented Dec 19, 2014

In my experience from training developers and frontend developers I am sure they would appreciate not having to create a composer.json in a theme that is only used within their company.

As written in the original post I think having potential duplicate information is preferable, especially if consistency is validated by displaying an appropriate error message in the admin area if version or parent theme don't match.
Anybody who wants to distribute their theme can simply omit the values in the theme.xml file.

@nyov
Copy link

nyov commented Dec 19, 2014

I have no strong feelings either way, though from an integration perspective having the software depending on the packaging support system to know it's own components looks a bit off.

It's probably okay in the core code; requiring theme designers to know about composer semantics might be a bit much, though.

@antonmakarenko
Copy link

This change was done by accident and we are working to fix it.

Theme parent should be declared in theme.xml. Any information relevant to how a theme works, should be in theme.xml.

The composer.json file in theme is intended for packaging/distribution purposes only.

@alankent
Copy link

https://alankent.wordpress.com/2014/12/19/magento-2-composer-and-dependence-on-it-quick-note/ has some detail on the principle in case interesting to people (basically supporting what Anton just said).

@shivkumarsingh7
Copy link

any one got solution for create/list new theme in magento backend?

@Vinai
Copy link
Contributor Author

Vinai commented Jan 16, 2015

WIthin the current develop branch you need to specify the parent and the version in the composer.json file, and the title and (optionaly) the preview file in the theme.xml file.

@shivkumarsingh7
Copy link

Hi @Vinai

following one is my app/design/frontend/Magento/magento2/theme.xml

<theme xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../../../lib/internal/Magento/Framework/Config/etc/theme.xsd">
    <title>Magento 2</title>
    <version>0.1.0</version>
    <parent>Magento/blank</parent>
    <media>
        <preview_image>media/preview.jpg</preview_image>
    </media>
</theme>

and following app/design/frontend/Magento/magento2/composer.json

{
    "name": "magento/magento2",
    "description": "N/A",
    "require": {
        "php": "~5.4.11|~5.5.0",
        "magento/theme-frontend-blank": "0.42.0-beta3",
        "magento/framework": "0.42.0-beta3",
        "magento/magento-composer-installer": "*"
    },
    "type": "magento2-theme",
    "version": "0.42.0-beta3",
    "license": [
        "OSL-3.0",
        "AFL-3.0"
    ],
    "extra": {
        "map": [
            [
                "*",
                "Magento/magento2"
            ]
        ]
    }
}

whats wrong I am doing here please help me to create custom theme. composer.json is copy of luma theme with some customization.

@Vinai
Copy link
Contributor Author

Vinai commented Jan 16, 2015

You posted the root composer.json file. Each theme is treated as a composer package, so each theme needs its own composer.json file.
See the composer.json of the luma theme for reference.

PS: Please use code blocks in the markdown. Its much mode readable that way.

@joanhe
Copy link
Contributor

joanhe commented Jan 23, 2015

Thanks for raising the issue. This issue should be fixed with internal pull requests 16 and 30 in the history.

@muasir
Copy link

muasir commented Jan 23, 2015

closed based on @joanhe comment.

@muasir muasir closed this as completed Jan 23, 2015
fe-lix- pushed a commit to fe-lix-/magento2 that referenced this issue Apr 6, 2018
MSI-824: Search in Low Stock Report grid is broken
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants