Skip to content

Duplicate module titles#38571

Closed
brianteeman wants to merge 1 commit intojoomla:4.2-devfrom
brianteeman:duplicate_module
Closed

Duplicate module titles#38571
brianteeman wants to merge 1 commit intojoomla:4.2-devfrom
brianteeman:duplicate_module

Conversation

@brianteeman
Copy link
Copy Markdown
Contributor

In certain circumstances (see below) the module title is rendered multiple times.

Pull Request for Issue #37147 and #12888

Testing Instructions

Create a custom module and set the style to "card" and publish on all pages
Create an article and load the module inside the article using {loadmodule}
Check the frontend on any page thaat will display both the article and the module

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

image

In certain circumstances (see below) the module title is rendered multiple times.
@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Aug 23, 2022

This closes the seventh oldest bug report from 2016 and annoyingly the fix has been here on github for 6 months but no one answered.

@RickR2H
Copy link
Copy Markdown
Member

RickR2H commented Aug 23, 2022

I have tested this item ✅ successfully on a7c2191


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38571.

@viocassel
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on a7c2191

👍


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38571.

@RickR2H
Copy link
Copy Markdown
Member

RickR2H commented Aug 23, 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38571.

1 similar comment
@RickR2H
Copy link
Copy Markdown
Member

RickR2H commented Aug 23, 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38571.

@RickR2H RickR2H added RTC This Pull Request is Ready To Commit PR-4.2-dev labels Aug 23, 2022
@RickR2H
Copy link
Copy Markdown
Member

RickR2H commented Aug 23, 2022

@bembelimen who can check the drone errors? The seems to be not related?

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 23, 2022
foreach (explode(' ', $attribs['style']) as $style) {
if ($moduleContent = LayoutHelper::render('chromes.' . $style, $displayData, $basePath)) {
$module->content = $moduleContent;
$module = $moduleContent;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should not override $module variable, it will break much more here.
Example: later on $app->triggerEvent('onAfterRenderModule', array(&$module, &$attribs)); will triggered with incorrect $module value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Possibly that’s the reason why api tests are failing, but that’s only a gut feeling.

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Aug 24, 2022

As a quick fix, I would suggest just clone a module, before this code:

$displayData = array(
'module' => $module,
'params' => $params,
'attribs' => $attribs,
);

$module = clone $module;

$displayData = array(
  'module'  => $module,
  'params'  => $params,
  'attribs' => $attribs,
);

With comment why it is cloned.

More advanced fix, is to cache a crome result separated from content and have a flag $module->chromeRendered. Similar to what we done for content:

public static function renderRawModule($module, Registry $params, $attribs = array())
{
if (!empty($module->contentRendered)) {
return $module->content;
}

upd: forget about cache, it have other drawbacks for "chrome"

@brianteeman
Copy link
Copy Markdown
Contributor Author

Happy for you to submit a pr to my branch. Would be good to fix this very old bug

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Aug 24, 2022

I made alternative one #38582 please test

@brianteeman
Copy link
Copy Markdown
Contributor Author

Closed in favour of #38582

@brianteeman brianteeman deleted the duplicate_module branch August 24, 2022 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants