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

Also change the default when calling setTheme #1736

Merged
merged 3 commits into from
May 14, 2023
Merged

Conversation

Asfolny
Copy link
Contributor

@Asfolny Asfolny commented Jul 16, 2021

Description (*)

When a custom controller is mailing multiple orders, first in a store where the design/theme/default is set (to "fr") and then in a store where it isn't set, we ran into problems where skinurls would refer to the first store used.

This is because calling getTheme("skin") the package would first look in $this->_theme["skin"], and if it found nothing (as emulation calls setTheme('')), the packager will defer to call getTheme("default"), which returns "fr", rather than the expected "default" for the store.

Manual testing scenarios (*)

  1. Create an extra store view (if you only have 1)
  2. Add an extra folder to skin/frontend/base/
  3. Add an image to both skin/frontend/base/default/images/ and skin/frontend/base/$FOLDER_STEP_2
  4. Change the theme in one store view under Configuration > General > Design > Themes > Default
  5. Create, change the hardcoded store ids and run the following shell script under shells/
<?php
require_once 'abstract.php';

class Storefront_TestStoreEmulation extends Mage_Shell_Abstract
{
    public function run()
    {
        // For testing purposes only, change if need be
        foreach (['3', '1'] as $store) {
            $appEmulation = Mage::getSingleton('core/app_emulation');
            $initialEnvironmentInfo = $appEmulation->startEnvironmentEmulation($store, 'frontend');

            // Must be an existing image
            echo Mage::getDesign()->getSkinUrl('images/logo.png') . "\n";

            $appEmulation->stopEnvironmentEmulation($initialEnvironmentInfo);
        }

        echo 'DONE!';
    }
}

$shell = new Storefront_TestStoreEmulation();
$shell->run();

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Jul 16, 2021
@Flyingmana
Copy link
Contributor

From the code this looks ok and makes sense. But this should really be tested by one of the Reviewers to make sure it has no odd side effects.

@github-actions github-actions bot removed the Component: Core Relates to Mage_Core label Oct 1, 2022
@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Oct 2, 2022
kiatng
kiatng previously approved these changes Oct 2, 2022
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Tested.

@sreichel
Copy link
Contributor

sreichel commented Oct 2, 2022

Should this be added to Mage_Adminhtml_Controller_Action::preDispatch() too?

luigifab
luigifab previously approved these changes Oct 10, 2022
Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

In production for two weeks, seems good.

@fballiano fballiano changed the base branch from 1.9.4.x to main May 12, 2023 17:26
@fballiano fballiano dismissed luigifab’s stale review May 12, 2023 17:26

The base branch was changed.

@fballiano fballiano changed the base branch from main to 1.9.4.x May 12, 2023 17:27
@fballiano fballiano changed the base branch from 1.9.4.x to main May 12, 2023 17:30
@github-actions github-actions bot added Component: Admin Relates to Mage_Admin Component: Cm/RedisSession Relates to Cm_RedisSession ddev documentation htaccess Mage.php Relates to app/Mage.php php-cs-fixer phpcs PHPStorm phpunit and removed Component: Core Relates to Mage_Core labels May 12, 2023
@github-actions github-actions bot added the Component: Core Relates to Mage_Core label May 12, 2023
@fballiano
Copy link
Contributor

I think this is now ready to be merged

@fballiano fballiano requested review from kiatng and luigifab May 12, 2023 17:38
@fballiano fballiano merged commit 5ab1fd6 into OpenMage:main May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants