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.

Sign up for GitHub

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

Jump to bottom

Allow to use $before parameter in addCss/addJs from XML #4151

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Allow to use $before parameter in addCss/addJs from XML #4151

merged 1 commit into from
Aug 21, 2024

Conversation

Caprico85
Copy link
Contributor

Description (*)

In 57563fc you added the ability to use addCss, addJs and addItem to add a new script or stylesheet before or after another specific item.

This works fine if you call it from PHP:

// Insert first.js before (true) or after (false) second.js. Works similarly for addCss and addItem
$head->addJs('first.js', '', 'second.js', true);
$head->addCss('first.css', '', 'second.css', true);
$head->addItem('js', 'first.css', '', null, null, 'second.css', true);

But usually you define your JS and CSS in XML. The <before> parameter doesn't work there.

<reference name="head">
    <action method="addCss">
        <script>first.css</script>
        <params/>
        <referenceName>seconds.css</referenceName>
        <before>true</before>
    </action>
    <action method="addJs">
        <script>first.js</script>
        <params/>
        <referenceName>seconds.js</referenceName>
        <before>true</before>
    </action>
</reference>

You'll find the reason for this in Mage_Page_Block_Html_Head::_sortItems(). This method sorts the newly added JS/CSS to the correct position. The method states in its PHPDoc comment that the $before parameter can be string or bool.

/**
 * @param string $referenceName
 * @param string|bool $before
 * @param string $type
 */
protected function _sortItems($referenceName, $before, $type)

But then $before is compared using strict comparison.

if ($key === $referenceName && $before === true) {

So, setting $before to a string value would never work. The <before>true</before> that you set in the XML earlier comes in as string "true", not as boolean true, and so would not have worked either.

To fix this, my PR adds a call to filter_var(...). filter_var(...) converts any boolean-like values ("true", "false", "1", "0", "on", "off", ...) into actual boolean values.

Manual testing scenarios (*)

Magento's rwd theme by default has these stylesheets in the <head> section:

    <link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/rwd/default/css/styles.css" media="all" >
    <link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/rwd/default/css/madisonisland.css" media="all" >

Imagine that on the home page, you want to add a new stylesheet my.css. For some dependency reasons you want to have it BEFORE the madisonisland.css. So, you add this to the Layout Update XML of the CMS page "home".

<reference name="head">
    <action method="addCss">
        <script>my.css</script>
        <params/>
        <referenceName>css/madisonisland.css</referenceName>
        <before>true</before>
    </action>
</reference>

It doesn't matter if the my.css actually exists.

Without my PR, the my.css will simply be added to the end, even if you set <before>true</before>:

<link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/rwd/default/css/styles.css" media="all" >
<link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/rwd/default/css/madisonisland.css" media="all" >
<link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/base/default/my.css" media="all" >

With my PR, it is correctly inserted before the madisonisland.css:

<link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/rwd/default/css/styles.css" media="all" >
<link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/base/default/my.css" media="all" >
<link rel="stylesheet" href="https://orig.vispronet.de/skin/frontend/rwd/default/css/madisonisland.css" media="all" >

Questions or comments

You might argue that we could just make the comparison non-strict. This would work with "true", "1", "0" and similar, but not with the string "false", which would still be interpreted as boolean true. That's why I used filter_var().

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)
  • Add yourself to contributors list

@github-actions github-actions bot added the Component: Page Relates to Mage_Page label Aug 8, 2024
sreichel
sreichel reviewed Aug 8, 2024
View reviewed changes
Copy link
Contributor

@sreichel sreichel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works for me.

@mattdavenport mattdavenport merged commit b6bba37 into OpenMage:main Aug 21, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiatng kiatng kiatng approved these changes

@mattdavenport mattdavenport mattdavenport approved these changes

@sreichel sreichel sreichel approved these changes

@empiricompany empiricompany empiricompany approved these changes

@ADDISON74 ADDISON74 ADDISON74 approved these changes

Assignees
No one assigned
Labels
Component: Page Relates to Mage_Page
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Caprico85 @kiatng @mattdavenport @sreichel @empiricompany @ADDISON74