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

local.xml in themes is not always the "right" way #4

Closed
rgranadino opened this Issue Jun 15, 2013 · 12 comments

Comments

Projects
None yet
4 participants
@rgranadino

rgranadino commented Jun 15, 2013

I think there's some debate amongst developers over the use of local.xml when creating themes. I personally would not recommend using local.xml when building themes (in most cases). The "right" way becomes too blurred depending on the context and/or end goal (and what sort of flexibility one values).

One of the larger problems with the use of local.xml is that it will only be loaded at the "uppermost" theme. For instance:

my_package/default/local.xml
my_package/mobile/local.xml

Inheriting the functionality within the default local.xml isn't trivial. When the custom default theme does not contain a local.xml file, using a local.xml within the mobile theme makes more sense. The number of tweaks will probably be limited and more easily managed.

Depending on the design, the number of customizations in local.xml grows rapidly and can end up with a messy XML file to manage.

During upgrades it's a good idea to know which core layout files have changed. Placing everything into local.xml causes loss of context (where are those blocks defined?). When copying over catalog.xml and updating/commenting out blocks, one can easily compare the custom layout file to the new core file.

There's other reasons for and against each approach but none seem to compelling enough to always use one vs the other. Therefore, there is no single "right" way to go about handling layout updates and stuffing everything into local.xml. This is probably the reason why you have:

Don’t create catalog.xml, customer.xml, page.xml, etc, files unless absolutely necessary.

When is it absolutely necessary?

I like the idea of guiding developers (especially those new to magento) in the right direction but this seems like a topic with too many edge cases. It should be expanded upon appropriately.

@benmarks

This comment has been minimized.

Show comment
Hide comment
@benmarks

benmarks Jun 19, 2013

local.xml should never be used in redistributed modules. It should be for the end implementer to use only.

As noted, customizing core layout files should be avoided in general to help with the upgrade path. For the same reason, when there is a volume of changes that would make upgrading difficult if they were collected in local.xml, then it would be reasonable to implement customizations in customized versions of the core files.

benmarks commented Jun 19, 2013

local.xml should never be used in redistributed modules. It should be for the end implementer to use only.

As noted, customizing core layout files should be avoided in general to help with the upgrade path. For the same reason, when there is a volume of changes that would make upgrading difficult if they were collected in local.xml, then it would be reasonable to implement customizations in customized versions of the core files.

@bgorski

This comment has been minimized.

Show comment
Hide comment
@bgorski

bgorski Jan 17, 2014

Contributor

I'd like to revive the discussion. Currently, it's stated on the page that:

A reusable theme is, for all intents and purposes, an extension to Magento and should also not ship with a customised local.xml.

I don't believe that this statement is true. It's obvious that extensions shouldn't ship with their own local.xml, because in many cases this approach wouldn't work or would generate conflicts with other extensions doing the same thing.

Themes, though, are something entirely different. There are things that you may want to change in your reusable theme layout even if it's not shipped with any extension. For instance, you may want to remove the left.permanent.callout and right.permanent.callout blocks (because leaving them there may result in lots of emails asking how to remove them). Doing it by rewriting catalog.xml (where those two callouts are) may result in limiting compatibility only to certain Magento versions. That's why it's better to include such changes in local.xml.

So, to summarize things: in my opinion, module-related layout changes should always be in module-related xml files, to keep thins clean. But if you have some layout changes that are not related to any module (like the example above or a RTL theme in which you need to move the content of the left column to the right one and vice versa) - you should always include a local.xml file, no matter if your theme is reusable or not.

Contributor

bgorski commented Jan 17, 2014

I'd like to revive the discussion. Currently, it's stated on the page that:

A reusable theme is, for all intents and purposes, an extension to Magento and should also not ship with a customised local.xml.

I don't believe that this statement is true. It's obvious that extensions shouldn't ship with their own local.xml, because in many cases this approach wouldn't work or would generate conflicts with other extensions doing the same thing.

Themes, though, are something entirely different. There are things that you may want to change in your reusable theme layout even if it's not shipped with any extension. For instance, you may want to remove the left.permanent.callout and right.permanent.callout blocks (because leaving them there may result in lots of emails asking how to remove them). Doing it by rewriting catalog.xml (where those two callouts are) may result in limiting compatibility only to certain Magento versions. That's why it's better to include such changes in local.xml.

So, to summarize things: in my opinion, module-related layout changes should always be in module-related xml files, to keep thins clean. But if you have some layout changes that are not related to any module (like the example above or a RTL theme in which you need to move the content of the left column to the right one and vice versa) - you should always include a local.xml file, no matter if your theme is reusable or not.

@punkstar

This comment has been minimized.

Show comment
Hide comment
@punkstar

punkstar Jan 17, 2014

Owner

Welcome to the discussion!

Consider this example: I make a theme called superawesome/default which contains a local.xml that adds all of my (super awesome, I guess) changes to base/default; removing unneeded blocks, adding a carousel on every category page and adding the new JS libraries that I'm using. A store owner wants to use this theme, with some slight customisations. So, they make superawesome/mystore theme, extending my theme. They naturally create a local.xml in their theme directory resulting in all of superawesome/default's local.xml changes being ignored.

I propose that a reusable theme is an extension to Magento and should ship with a SuperAwesome_Core extension that adds its own layout file called superawesome/core.xml.

The local.xml file should be saved for local changes, and I think in this case it means the final store owner's theme.

Thoughts?

Owner

punkstar commented Jan 17, 2014

Welcome to the discussion!

Consider this example: I make a theme called superawesome/default which contains a local.xml that adds all of my (super awesome, I guess) changes to base/default; removing unneeded blocks, adding a carousel on every category page and adding the new JS libraries that I'm using. A store owner wants to use this theme, with some slight customisations. So, they make superawesome/mystore theme, extending my theme. They naturally create a local.xml in their theme directory resulting in all of superawesome/default's local.xml changes being ignored.

I propose that a reusable theme is an extension to Magento and should ship with a SuperAwesome_Core extension that adds its own layout file called superawesome/core.xml.

The local.xml file should be saved for local changes, and I think in this case it means the final store owner's theme.

Thoughts?

@benmarks

This comment has been minimized.

Show comment
Hide comment
@benmarks

benmarks Jan 17, 2014

@punkstar is right on. Think of local.xml as a layer which is always available to the end user to collect and effect layout customizations with the most precedence among layout files; the code clearly indicates this in Mage_Core_Model_Layout_Update::getFileLayoutUpdateXml():

public function getFileLayoutUpdatesXml($area, $package, $theme, $storeId = null)
{
    //snip...
    foreach ($updatesRoot->children() as $updateNode) {
        if ($updateNode->file) {
            $module = $updateNode->getAttribute('module');
            if ($module && Mage::getStoreConfigFlag('advanced/modules_disable_output/' . $module, $storeId)) {
                continue;
            }
            $updateFiles[] = (string)$updateNode->file;
        }
    }
    // custom local layout updates file - load always last
    $updateFiles[] = 'local.xml';
    //snip...
}

Given the function (and the comment!), it's clear that local.xml is to be reserved for the instance maintainer. Whereas custom themes can declare a layout update file, there is no reason to supersede the maintainer's needs.

benmarks commented Jan 17, 2014

@punkstar is right on. Think of local.xml as a layer which is always available to the end user to collect and effect layout customizations with the most precedence among layout files; the code clearly indicates this in Mage_Core_Model_Layout_Update::getFileLayoutUpdateXml():

public function getFileLayoutUpdatesXml($area, $package, $theme, $storeId = null)
{
    //snip...
    foreach ($updatesRoot->children() as $updateNode) {
        if ($updateNode->file) {
            $module = $updateNode->getAttribute('module');
            if ($module && Mage::getStoreConfigFlag('advanced/modules_disable_output/' . $module, $storeId)) {
                continue;
            }
            $updateFiles[] = (string)$updateNode->file;
        }
    }
    // custom local layout updates file - load always last
    $updateFiles[] = 'local.xml';
    //snip...
}

Given the function (and the comment!), it's clear that local.xml is to be reserved for the instance maintainer. Whereas custom themes can declare a layout update file, there is no reason to supersede the maintainer's needs.

@bgorski

This comment has been minimized.

Show comment
Hide comment
@bgorski

bgorski Jan 17, 2014

Contributor

Maybe it's my misunderstanding - I understood it as "a reusable theme IS an extension when it comes to using/not using local.xml", not as "reusable theme should ship WITH an extension", it that's what @punkstar meant. Shipping it with an extension does makes sense.
But I don't think that its layout files should go base/default as in case of "normal" extensions. It would force store owners to disable module output for every store view that's not using the theme (as main theme or as a fallback). Theme-specific layout files should be placed inside the theme itself.

Contributor

bgorski commented Jan 17, 2014

Maybe it's my misunderstanding - I understood it as "a reusable theme IS an extension when it comes to using/not using local.xml", not as "reusable theme should ship WITH an extension", it that's what @punkstar meant. Shipping it with an extension does makes sense.
But I don't think that its layout files should go base/default as in case of "normal" extensions. It would force store owners to disable module output for every store view that's not using the theme (as main theme or as a fallback). Theme-specific layout files should be placed inside the theme itself.

@benmarks

This comment has been minimized.

Show comment
Hide comment
@benmarks

benmarks Jan 17, 2014

@bgorski Theme & extension developers have no knowledge regarding configuration; this is why base/default was created in CE1.4. In general any new theme asset SHOULD be in base/default; this has been confirmed by the core team on several occasions. This will handle the majority of sites which will only have one theme. When there are multiple theme variations for a site, it is necessarily a customization, and a user could use DMO to exclude the layout XML file without touching the filesystem, or could reach into the filesystem and move things around.

benmarks commented Jan 17, 2014

@bgorski Theme & extension developers have no knowledge regarding configuration; this is why base/default was created in CE1.4. In general any new theme asset SHOULD be in base/default; this has been confirmed by the core team on several occasions. This will handle the majority of sites which will only have one theme. When there are multiple theme variations for a site, it is necessarily a customization, and a user could use DMO to exclude the layout XML file without touching the filesystem, or could reach into the filesystem and move things around.

@bgorski

This comment has been minimized.

Show comment
Hide comment
@bgorski

bgorski Jan 17, 2014

Contributor

@benmarks, if you ship your theme along with a theme-specific extension that's doing layout manipulations that make sense only on this one particular theme, what good comes from placing your layout files in the base/default directory? It doesn't change anything on sites having only one theme and makes things worse for multi-stores (because module output has to be disabled for websites using other themes).

Contributor

bgorski commented Jan 17, 2014

@benmarks, if you ship your theme along with a theme-specific extension that's doing layout manipulations that make sense only on this one particular theme, what good comes from placing your layout files in the base/default directory? It doesn't change anything on sites having only one theme and makes things worse for multi-stores (because module output has to be disabled for websites using other themes).

@benmarks

This comment has been minimized.

Show comment
Hide comment
@benmarks

benmarks Jan 17, 2014

@bgorski We might want to take this chat online; I fear there may be a bit getting lost in translation.

It doesn't change anything on sites having only one theme

I'm not sure what you mean here; if a layout XML file is added to base/default and configured under frontend/layout/updates/[module]/file>my_theme.xml then its directives will be merged just like all the others and it most certainly WILL have an effect. Further, there is no mechanism for installing the file into some other design package's theme, as these are arbitrarily variable from instance to instance (as a developer how could I know that you are using foo/whee as your package/theme?). Extensions are packaged with each file having a specific destination, and base/default is the only universally-resolvable location for theme assets.

[it] makes things worse for multi-stores (because module output has to be disabled for websites using other themes)

As I stated, if multiple themes are being used in a Magento instance, adding a custom theme for use in just one of those contexts absolutely means that the user has to do something beyond installing the module; even using your approach, the user has to manually place theme files in one theme directory or another. Given that multi-theme instances are certainly in the minority AND that Magento extensions are intended to work solely via the Connect panel (meaning no direct FS manipulation), the base/default + DMO approach seems to accommodate the larger set of needs. When an instance maintainer is savvy enough to work with the filesystem, they can move theme files however they prefer.

Does this at least make sense, even if you don't agree with the logic?

benmarks commented Jan 17, 2014

@bgorski We might want to take this chat online; I fear there may be a bit getting lost in translation.

It doesn't change anything on sites having only one theme

I'm not sure what you mean here; if a layout XML file is added to base/default and configured under frontend/layout/updates/[module]/file>my_theme.xml then its directives will be merged just like all the others and it most certainly WILL have an effect. Further, there is no mechanism for installing the file into some other design package's theme, as these are arbitrarily variable from instance to instance (as a developer how could I know that you are using foo/whee as your package/theme?). Extensions are packaged with each file having a specific destination, and base/default is the only universally-resolvable location for theme assets.

[it] makes things worse for multi-stores (because module output has to be disabled for websites using other themes)

As I stated, if multiple themes are being used in a Magento instance, adding a custom theme for use in just one of those contexts absolutely means that the user has to do something beyond installing the module; even using your approach, the user has to manually place theme files in one theme directory or another. Given that multi-theme instances are certainly in the minority AND that Magento extensions are intended to work solely via the Connect panel (meaning no direct FS manipulation), the base/default + DMO approach seems to accommodate the larger set of needs. When an instance maintainer is savvy enough to work with the filesystem, they can move theme files however they prefer.

Does this at least make sense, even if you don't agree with the logic?

@bgorski

This comment has been minimized.

Show comment
Hide comment
@bgorski

bgorski Jan 18, 2014

Contributor

It does, of course.
I may in fact not presented my thoughts clearly in my previous comment - I'll try to do it here. I'll start with emphasizing that I'm commenting on:

I propose that a reusable theme is an extension to Magento and should ship with a SuperAwesome_Core extension that adds its own layout file called superawesome/core.xml.

and not addresing where xml files should be placed in general.

What I meant is when you have a single-theme website, you don't really care (as a shop owner) if a layout xml file is inside base/default or mypackage/mytheme that's currently used - the result is the same. But when your Magento instance contains more than one store view, and those store views use different themes, you don't want theme-specific xml files to be in base/default as they will affect all your store views (it forces you to use the DMO approach to prevent it).

So, when shipping your reusable theme with a module creating new layout update files (so you don't have to rewrite standard xml files like catalog.xml or page.xml and you may leave local.xml for the instance maintainer to create and use), it's better to include those xml files inside your theme instead of putting them in base/default. For example, when creating a theme that resides in app/design/frontend/myCompany/myBestTheme that's shipped with a module defining a new layout update file named myLayoutChanges.xml, it's best to put the file in app/design/frontend/myCompany/myBestTheme/layout/myLayoutChanges.xml instead of app/design/frontend/base/default/layout/myLayoutChanges.xml. This is possible, because you ship both the theme and the module in a single Magento Connect package, so the module knows how the theme is named.

Of course, this is the case only if the layout update xml file contains changes that are intended to work ONLY on one specific theme they are shipped with in one single package. If a theme is packed with a modules that may work on any theme (like universally-styled sliders or some jQuery lightbox modules), their layout update xml files should be placed in base/default.

Contributor

bgorski commented Jan 18, 2014

It does, of course.
I may in fact not presented my thoughts clearly in my previous comment - I'll try to do it here. I'll start with emphasizing that I'm commenting on:

I propose that a reusable theme is an extension to Magento and should ship with a SuperAwesome_Core extension that adds its own layout file called superawesome/core.xml.

and not addresing where xml files should be placed in general.

What I meant is when you have a single-theme website, you don't really care (as a shop owner) if a layout xml file is inside base/default or mypackage/mytheme that's currently used - the result is the same. But when your Magento instance contains more than one store view, and those store views use different themes, you don't want theme-specific xml files to be in base/default as they will affect all your store views (it forces you to use the DMO approach to prevent it).

So, when shipping your reusable theme with a module creating new layout update files (so you don't have to rewrite standard xml files like catalog.xml or page.xml and you may leave local.xml for the instance maintainer to create and use), it's better to include those xml files inside your theme instead of putting them in base/default. For example, when creating a theme that resides in app/design/frontend/myCompany/myBestTheme that's shipped with a module defining a new layout update file named myLayoutChanges.xml, it's best to put the file in app/design/frontend/myCompany/myBestTheme/layout/myLayoutChanges.xml instead of app/design/frontend/base/default/layout/myLayoutChanges.xml. This is possible, because you ship both the theme and the module in a single Magento Connect package, so the module knows how the theme is named.

Of course, this is the case only if the layout update xml file contains changes that are intended to work ONLY on one specific theme they are shipped with in one single package. If a theme is packed with a modules that may work on any theme (like universally-styled sliders or some jQuery lightbox modules), their layout update xml files should be placed in base/default.

@benmarks

This comment has been minimized.

Show comment
Hide comment
@benmarks

benmarks Jan 20, 2014

"when you have a single-theme website, you don't really care (as a shop owner) if a layout xml file is inside base/default"

Ah, but you do, the second that the package is changed! It's the same problem with enterprise/default. As a store owner, changing package settings shouldn't break functionality (theoretically).

it's best to put the file in app/design/frontend/myCompany/myBestTheme/layout/myLayoutChanges.xml

As a module developer, how does the module know where to put the files? Theoretically, you could detect the theme settings and prompt the user at installation, but that would require a workflow which doesn't really exist. The only universally-available theme is base/default.

Remember, Magento has to work for Connect users and devs.

benmarks commented Jan 20, 2014

"when you have a single-theme website, you don't really care (as a shop owner) if a layout xml file is inside base/default"

Ah, but you do, the second that the package is changed! It's the same problem with enterprise/default. As a store owner, changing package settings shouldn't break functionality (theoretically).

it's best to put the file in app/design/frontend/myCompany/myBestTheme/layout/myLayoutChanges.xml

As a module developer, how does the module know where to put the files? Theoretically, you could detect the theme settings and prompt the user at installation, but that would require a workflow which doesn't really exist. The only universally-available theme is base/default.

Remember, Magento has to work for Connect users and devs.

@bgorski

This comment has been minimized.

Show comment
Hide comment
@bgorski

bgorski Jan 27, 2014

Contributor

As a module developer, how does the module know where to put the files?

When you create a theme package for the purpose of publishing it on Magento Comment, you can also include a module there. In the same package. This is how the module knows where to put its files - because they're already in a directory where the theme author put them, so no detection methods or installation prompts are needed.
Remember, the initial idea proposed by @punkstar is to ship a theme with a module, because it allows you (among other things) to create your own layout update xml files.

As a store owner, changing package settings shouldn't break functionality (theoretically).

If a layout update file contains things intended to work with a single theme (the one it was shipped with), then by all means changing a package should make those things ignored by the system.

The approach you're suggesting (base/default + DMO) works, of course, but it's a less practical one, because it requires admins to take additional actions (DMO in case of more store views using different themes) besides just installing the theme and setting it as an active one. In my opinion it should be used only in case of modules that are intended to work with any theme, not just a specific single one - so if you ship your theme with a module developed to work with any theme, then (and I propose: only then) you should of course put its layout update xml files in base/default.

Contributor

bgorski commented Jan 27, 2014

As a module developer, how does the module know where to put the files?

When you create a theme package for the purpose of publishing it on Magento Comment, you can also include a module there. In the same package. This is how the module knows where to put its files - because they're already in a directory where the theme author put them, so no detection methods or installation prompts are needed.
Remember, the initial idea proposed by @punkstar is to ship a theme with a module, because it allows you (among other things) to create your own layout update xml files.

As a store owner, changing package settings shouldn't break functionality (theoretically).

If a layout update file contains things intended to work with a single theme (the one it was shipped with), then by all means changing a package should make those things ignored by the system.

The approach you're suggesting (base/default + DMO) works, of course, but it's a less practical one, because it requires admins to take additional actions (DMO in case of more store views using different themes) besides just installing the theme and setting it as an active one. In my opinion it should be used only in case of modules that are intended to work with any theme, not just a specific single one - so if you ship your theme with a module developed to work with any theme, then (and I propose: only then) you should of course put its layout update xml files in base/default.

@punkstar

This comment has been minimized.

Show comment
Hide comment
@punkstar

punkstar Sep 16, 2016

Owner

Strolls in two and a half years later..

Added a link to this discussion on the site. 🎉

Owner

punkstar commented Sep 16, 2016

Strolls in two and a half years later..

Added a link to this discussion on the site. 🎉

@punkstar punkstar closed this Sep 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment