Open AlterWeb opened 3 years ago
aterjung
commented
3 years ago Thank you for writing this bug report! There is something wrong in the theme switcher code. I will fix this as soon as possible. As long as this is fixed and included in the next release, you could just move the skin overwrite to the website level.
AlterWeb
commented
3 years ago @aterjung Thank you for the quick response. I think this issue is not new but we never noticed it because the (old) default theme it is falling back to was the expected behavior. If I remember correctly from debugging it, the code responsable for this behavior was not edited. You even may say that it's a feature, but if it is than it is a confusing one ;-). Thank you for noticing that workaround, we already did do exactly that for now. So no big problem for now but it makes the configuration a bit more confusing.
aterjung
commented
3 years ago @AlterWeb where is the code you identified as source for this error? I am a little bit confused at the moment, my dev-environment where the theme switcher was developed does not show this behavior! But all of my customers live systems do have this error.
AlterWeb
commented
3 years ago I don't have access to my workstation at the moment (typing this on my phone) but if I remember correctly it was in the class Mage_Adminhtml_Controller_Action. Around line 163 there is a foreach loop setting those values. I can check this tomorrow.
AlterWeb
commented
3 years ago @aterjung I just checked it and this is indeed where those values are set. So this code:
foreach (array('layout', 'template', 'skin', 'locale') as $type) {
if ($value = (string)Mage::getConfig()->getNode("stores/admin/design/theme/{$type}")) {
Mage::getDesign()->setTheme($type, $value);
}
}
kiatng
commented
3 years ago I think it's worth considering making the new openmage theme as a separate package, which will resolve this issue and answer issue #1164. I made the following test, which requires renaming the current default/default directory to base/default like in frontend. The complete changes required are:
app/design/adminhtml/default/openmage to new directory app/design/adminhtml/openmage/default.skin/adminhtml/default/openmage to new directory skin/adminhtml/openmage/default.default/default to base/defaultapp/design/adminhtml/default/ to app/design/adminhtml/base/.skin/adminhtml/default/ to skin/adminhtml/base/.Add new file: app/design/adminhtml/openmage/default/etc/theme.xml
<?xml version="1.0"?>
<!--
/**
*
* @category design
* @package openmage_default
*/
-->
<theme>
<parent />
</theme>This file is copied from the frontend packages. See, for example rwd's theme.xml.
In app\code\core\Mage\Adminhtml\etc\config.xml, the following can be removed in entirety:
<stores>
<admin>
<design>
<package>
<name>default</name>
</package>
<theme>
<default>default</default>
<openmage>openmage</openmage>
</theme>
</design>
</admin>
</stores>After I removed the above, the backend is the old admin theme.
To use openmage package, we can add the following in any config.xml file:
<stores>
<admin>
<design>
<package>
<name>openmage</name>
</package>
<theme>
<default>default</default>
</theme>
</design>
</admin>
</stores>The above can be set in a switcher.
With the above changes, we should have infiite theme fallback. It should work but I have not tested it thoroughly.
aterjung
commented
3 years ago @kiatng thank you for this. This would be a solution for #1164 at least. I will prepare a fork for easy testing.
I am not shure if this can fix the strange behavior of the Mage_Adminhtml_Controller_Action`s preDispatch method. Probably i miss the point that is handled here:
Mage::getDesign()
->setArea($this->_currentArea)
->setPackageName((string)Mage::getConfig()->getNode('stores/admin/design/package/name'))
->setTheme((string)$theme);
foreach (array('layout', 'template', 'skin', 'locale') as $type) {
if ($value = (string)Mage::getConfig()->getNode("stores/admin/design/theme/{$type}")) {
Mage::getDesign()->setTheme($type, $value);
}
}For me it looks like it handles the selected theme layout/template/skin/locale settings for the backend. getConfig()->getNode should get the values form xml config. Indeed the returned values are the ones for the frontend (defined in core_config_data) if there are no xml definitions. I wasn't aware of this fallback until now. This happens as well for the package name. If you remove
<stores>
<admin>
<design>
<package>
<name>default</name>
</package>the package you define in the backend will be used for the adminhtml area!
Setting default values in the config.xml fixes this but breaks the theme switch that is implemented now:
<stores>
<admin>
<design>
<package>
<name>default</name>
</package>
<theme>
<default>default</default>
<openmage>openmage</openmage>
<layout>openmage</layout>
<template>openmage</template>
<skin>openmage</skin>
<locale>openmage</locale>
</theme>
</design>
</admin>
</stores>Thus, i think migration to a backend theme package let this problem disappear without resolving it. If i get it right, this is a problem of bad design back to the stating days of magento. :-) The backend theme uses the config of storeId 0 which is the "default config" everybody uses to put the fronted theme in. To set it fixed to "adminhtml" this value is overridden by xml config - but only for the package - and "default". If you add a "skin"-Overwrite (for the frontend) in the backend it is not found in adminhtml and therefore ignored falling back to "default".
Do we really need the layout, skin, template and locale overwrite for backend themes? Probably we should drop this foreach an fix the values there?
kiatng
commented
3 years ago Continue with my comment, I tested custom theme rewrite based on it.
As an example, I want to rewrite this file app/design/adminhtml/base/default/template/customer/tab/view.phtml.
Copy and paste the file to new package called ccs, the full path is then app/design/adminhtml/ccs/default/template/customer/tab/view.phtml.
Then I added this: <h1>Theme Rewrite in Package ccs</h1> to the copied file.
Add a new file app/design/adminhtml/ccs/default/etc/theme.xml
<?xml version="1.0"?>
<!--
/**
*
* @category design
* @package ccs_default
*/
-->
<theme>
<parent>openmage/default</parent>
</theme>Note: if the file is missing, it will fallback to the package base. But the rewrite still works:
In any config.xml:
<stores>
<admin>
<design>
<package>
<name>ccs</name>
</package>
<theme>
<default>default</default>
</theme>
</design>
</admin>
</stores>Voila:
app/design/adminhtml/default/default to app/design/adminhtml/base/default, BC? Instead of move, we can copy, which create bloatware?aterjung
commented
3 years ago @kiatng have you read my comment about the way the adminhtml package is forced in the Controller?
Mage::getConfig()->getNode('stores/admin/design/package/name') will return this line from core_config_data if no package is defined for store/admin in any config.xml:
That is because the admin area is considered as store "0". The Problem is that the "default" config for the frontend themes is also saved with scope_id 0. Normaly there will always be a config.xml setting the adminhtml package to a different value. If you put a overwrite for "template" for example in the backend config in "default" scope, lets say "mytemp", then this value form db will be set for the backend theme as well - but it is intended for frontend. That is becase no config.xml setting is made for the template path. If you use the default theme, that will not harm, cause there is no folder "mytemp" in the adminhtml folder which let the value gracefully fall back to "default". But using the openmage theme, the fallback to "default" is not what we want.
The change to an own package you suggest will heal this symptom of a broken design at first, because the theme would be called "default" again. But everybody wo builds his own custom theme by adding a theme to the openmage package would ran into this problem again.
From my point of view, we have to fix this "combined" config path for frontend and backend theme to get this fixed. If the openmage theme should be a package or a theme is a different discussion then.
aterjung
commented
3 years ago @kiatng I tested this with the same results. I build up theme packages just like you showed and created a custom package which I set as design package from config.xml file!
Going on from this point, my PR #1229 reverts the changes to Mage_Adminhtml_Controller_Action::preDispatch method for the legacy theme switcher and introduces a new option to overwrite admin theme from backend. This is probably not the final version, but it should work without problems for your example and let you chose which design package you want right from the backend. It would be great if you could confirm that this works for you.
theroch
commented
1 year ago We use the below approach for several years without problems for our own packages and themes.
We moved adminhtml/default/default to adminhtml/base/default for several years.
Now we have the problem with the new Openmage theme too and solved it like this:
Make a new openmage package
1. Move contents from `app/design/adminhtml/default/openmage` to new directory `app/design/adminhtml/openmage/default`. 2. Move contents from `skin/adminhtml/default/openmage` to new directory `skin/adminhtml/openmage/default`.Change
default/defaulttobase/default1. Change package directory name from `app/design/adminhtml/default/` to `app/design/adminhtml/base/`. 2. Change skin directory name from `skin/adminhtml/default/` to `skin/adminhtml/base/`.
We use in file: app/design/adminhtml/openmage/default/etc/theme.xml
<?xml version="1.0"?>
<!--
/**
*
* @category design
* @package openmage_default
*/
-->
<theme>
<parent>default/default</parent>
</theme>In app\code\core\Mage\Adminhtml\etc\config.xml, we set the following:
<stores>
<admin>
<design>
<package>
<name>default</name>
<openmage>openmage</openmage>
</package>
<theme>
<default>default</default>
</theme>
</design>
</admin>
</stores>In app/code/core/Mage/Adminhtml/Controller/Action.php you can use this code:
// get legacy theme choice form backend config
if (Mage::getStoreConfigFlag('admin/design/use_legacy_theme')) {
$package = Mage::getConfig()->getNode("stores/admin/design/package/name");
} else {
$package = Mage::getConfig()->getNode("stores/admin/design/package/openmage");
}
Mage::getDesign()
->setArea($this->_currentArea)
->setPackageName((string)$package)
->setTheme((string)Mage::getConfig()->getNode('stores/admin/design/theme/default'));
foreach (array('layout', 'template', 'skin', 'locale') as $type) {
if ($value = (string)Mage::getConfig()->getNode("stores/admin/design/theme/{$type}")) {
Mage::getDesign()->setTheme($type, $value);
}
}This is working for us.
There is some problem with the fallback logic because the skin from skin/adminhtml/default/default is never used although while the etc/theme.xml is set to default/default. Also the local.xml from openmage was never load.
This issue need some more investigation from us.
After some investigation and reading the very helpful information from Alana Storm and Eric Wiese to "Infinite theme fallback", I've found a working solution without breaking backwards compatibility.
First of all: All the stuff from above is needed.
app/design/adminhtml/openmage/default/etc/theme.xml to avoid the use of the local.xml.
You can read more about this in the links above.<?xml version="1.0"?>
<!--
/**
*
* @category design
* @package openmage_default
*/
-->
<theme>
<parent>default/default</parent>
<layout>
<updates>
<openmage_adminhtml_default>
<file>openmage.xml</file>
</openmage_adminhtml_default>
</updates>
</layout>
</theme>Rename from app/design/adminhtml/openmage/default/layout/local.xml to app/design/adminhtml/openmage/default/layout/openmage.xml
In app/code/core/Mage/Core/Model/Design/Fallback.php use this code for function getFallbackScheme:
public function getFallbackScheme($area, $package, $theme)
{
$cacheKey = $area . '/' . $package . '/' . $theme;
if (!isset($this->_cachedSchemes[$cacheKey])) {
//First we have to check if theme exists
$path = "$area".DS."$package".DS."$theme";
$fallback = false;
if (!is_dir(Mage::getBaseDir('design') . DS . $path)) {
//Fallback to default
$theme = (string)Mage::getConfig()->getNode('stores/admin/design/theme/default');
$fallback = true;
}
if ($this->_isInheritanceDefined($area, $package, $theme)) {
$scheme = $this->_getFallbackScheme($area, $package, $theme);
} else {
$scheme = $this->_getLegacyFallbackScheme();
}
if ($fallback) {
$first = array_shift($scheme);
$scheme = array_merge([$first], [['_package' => $package, '_theme' => $theme]], $scheme);
}
$this->_cachedSchemes[$cacheKey] = $scheme;
}I've added the fix from Eric Wiese also to the app/code/core/Mage/Core/Model/Layout/Update.php since I think it is in line with the expectations of the developers. But this code is not needed to get the adminhtml to work:
public function getFileLayoutUpdatesXml($area, $package, $theme, $storeId = null)
{
if (null === $storeId) {
$storeId = Mage::app()->getStore()->getId();
}
/* @var Mage_Core_Model_Design_Package $design */
$design = Mage::getSingleton('core/design_package');
$layoutXml = null;
$elementClass = $this->getElementClass();
$updatesRoot = Mage::app()->getConfig()->getNode($area.'/layout/updates');
//Newly added this function from Eric Wiese fix
$updatesRoot = $this->addFallbackThemesLayoutUpdates($updatesRoot);
Mage::dispatchEvent('core_layout_update_updates_get_after', array('updates' => $updatesRoot));
$updates = $updatesRoot->asArray();
...
/**
* Add layout files added via theme.xml to layout updates
* for all themes that are parents of this theme.
* Observes: core_layout_update_updates_get_after
*
* @param Varien_Event_Observer $observer
*/
public function addFallbackThemesLayoutUpdates(Mage_Core_Model_Config_Element $updates) {
/* @var $designPackage Mage_Core_Model_Design_Package */
$designPackage = Mage::getSingleton('core/design_package');
/* @var $fallback Mage_Core_Model_Design_Fallback */
$fallback = Mage::getModel('core/design_fallback');
$fallbacks = $fallback->getFallbackScheme($designPackage->getArea(), $designPackage->getPackageName(), $designPackage->getTheme('layout'));
for($i=count($fallbacks)-1; $i>=0; $i--) {
$fallback = $fallbacks[$i];
if(!isset($fallback['_package']) || !isset($fallback['_theme'])) {
continue;
}
$fallbackPackage = $fallback['_package'];
$fallbackTheme = $fallback['_theme'];
$themeUpdateGroups = Mage::getSingleton('core/design_config')->getNode("{$designPackage->getArea()}/$fallbackPackage/$fallbackTheme/layout/updates");
if(!$themeUpdateGroups) {
continue;
}
foreach($themeUpdateGroups as $themeUpdateGroup) {
$themeUpdateGroupArray = $themeUpdateGroup->asArray();
foreach($themeUpdateGroupArray as $key => $themeUpdate) {
$updateNode = $updates->addChild($key);
$updateNode->addChild('file', $themeUpdate['file']);
}
}
}
return $updates;
}Now it is possible to set custom values in design/theme for layout, locales, skin, templates without breaking the adminhtml design. It is now also possible to define custom adminhtml themes with the same name as in the frontend to customize the adminhtml for customers/clients.
dbachmann
commented
10 months ago @theroch Great job.
Just some thoughts. Wouldn't it be easiest to add a configuration section like the one for the frontend instead of using a static configuration by dropdown to enable the new theme? At the end we could offer alternative design packages for the backend and it would be also easier for less experienced users to install new themes.
theroch
commented
10 months ago @dbachmann
Wouldn't it be easiest to add a configuration section like the one for the frontend instead of using a static configuration by dropdown to enable the new theme?
Sure, this is possible, but I think this is out of scope of this topic. If you want such a feature then please create a feature request. With our approach, it is possible to define a custom adminhtml theme which is configured or better depends to the settings of the frontend. Here is an example: Frontend: Configured is custom_design_1. If you now create a theme with the same name for the adminhtml, you can put there your customizations. With the inheritance, OpenMage loads files in this way custom_design_1 -> openmage -> base.
At the end we could offer alternative design packages for the backend and it would be also easier for less experienced users to install new themes.
Yes, this possible too, all the necessary logic is already implemented. But from my experience, it is better to use a standard adminhtml theme for clients/merchants. It is easier to create training courses and users can use the internet to find useful information about the handling and functions of the backend. Sure, there are a lot of smart people out there, but there are also people they have problems if a menu entry was gray and now it is blue or a name switches from catalog to articles.
Preconditions (*)
Steps to reproduce (*)
Expected result (*)
Actual result (*)