Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#41107] - mod_articles_news: Only prepare content if introtext is shown

PR-4.4-dev Pending

User tests: Successful: Unsuccessful:

avatar ManuelHu
ManuelHu
3 Jul 2023

Pull Request for Issue #41086.

Summary of Changes

content.prepare is now only called if the module configuration has "show introtext" set to "Yes". This is similar to mod_articles_category:

joomla-cms/modules/mod_articles_category/src/Helper/ArticlesCategoryHelper.php

Lines 274 to 277 in 507922c

if ($show_introtext) {
$item->introtext = HTMLHelper::_('content.prepare', $item->introtext, '', 'mod_articles_category.content');
$item->introtext = self::_cleanIntrotext($item->introtext);
}

Testing Instructions

  • Have a site with at least one enabled content plugin that does some sort of content replacement and adds some scripts (i.e. image gallery tools, video embedding...)
  • Create an article that contains some content that triggers the content plugin (i.e. adds scripts to the output)
  • Add a mod_articles_news module to any position
  • Select the category of the article created above
  • Disable the showing of the intro text in the module options
  • Observe the HTML output in the frontend: The intro text is not shown (as expected), but scripts that were added are in the output

Actual result BEFORE applying this Pull Request

Content is always content.prepared, even if the introtext is not displayed. Anything that is changed by content plugins besides the content (e.g. added scripts and stylesheets) is contained in the frontend HTML code (and might have side effects...)

Expected result AFTER applying this Pull Request

Content is only content.prepared if it is actually selected to be shown. Anything that is changed by content plugins besides the content (e.g. added scripts and stylesheets) is NOT contained in the frontend HTML code.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 3 Jul 2023
Category ⇒ Modules Front End
avatar ManuelHu ManuelHu - open - 3 Jul 2023
avatar ManuelHu ManuelHu - change - 3 Jul 2023
Status New ⇒ Pending
avatar brianteeman
brianteeman - comment - 3 Jul 2023

There is already an option for this.

image

joomla-cms/modules/mod_articles_news/src/Helper/ArticlesNewsHelper.php

Lines 168 to 183 in 507922c

if ($triggerEvents) {
$item->text = '';
$app->triggerEvent('onContentPrepare', ['com_content.article', &$item, &$params, 0]);
$results = $app->triggerEvent('onContentAfterTitle', ['com_content.article', &$item, &$params, 0]);
$item->afterDisplayTitle = trim(implode("\n", $results));
$results = $app->triggerEvent('onContentBeforeDisplay', ['com_content.article', &$item, &$params, 0]);
$item->beforeDisplayContent = trim(implode("\n", $results));
$results = $app->triggerEvent('onContentAfterDisplay', ['com_content.article', &$item, &$params, 0]);
$item->afterDisplayContent = trim(implode("\n", $results));
} else {
$item->afterDisplayTitle = '';
$item->beforeDisplayContent = '';
$item->afterDisplayContent = '';

??

avatar ManuelHu
ManuelHu - comment - 3 Jul 2023

This option currently does not control the onContentPrepare event. Yes, the event is mentioned in the code of the module helper, but I'm not even sure what this does at this point. Before triggering the event in line 170, the text is reset to '', and content plugins usually work on text...

But currently onContentPrepare is also called by HTMLHelper::_('content.prepare, ...) on the introtext, even if $triggerEvents is false. This is the issue that I'm trying to fix here.

The question remains if the content.prepare on the introtext should be called if $triggerEvents is true, or if the introtext is actually shown, or even both?

avatar laoneo
laoneo - comment - 17 Aug 2023

I would even go one step further and also check if the introtext is not empty.

avatar ManuelHu
ManuelHu - comment - 6 Sep 2023

@laoneo This is a reasonable idea, but should this also be adjusted in the other modules (mod_article_category)?

avatar laoneo
laoneo - comment - 6 Sep 2023

For now just here and then we can see if we want to expand to other modules.

avatar ManuelHu ManuelHu - change - 6 Sep 2023
Labels Added: ? PR-4.4-dev
avatar HLeithner
HLeithner - comment - 7 Sep 2023

As already said by hannes, I'm also not happy to run this event optional. You don't know if not a plugin fills it without using the existing content.

avatar laoneo
laoneo - comment - 7 Sep 2023

With this particular call, all a plugin gets, is an object with a ->text attribute which is empty. This is not the event which gets triggered to prepare the article for custom fields, etc. This happens here.

avatar laoneo laoneo - change - 8 Sep 2023
Status Pending ⇒ Closed
Closed_Date 0000-00-00 00:00:00 ⇒ 2023-09-08 08:59:54
Closed_By ⇒ laoneo
Labels Removed: ?
avatar laoneo
laoneo - comment - 8 Sep 2023

Thanks @ManuelHu for the pr. Unfortunately we don't really know the side effects it can have when we trigger the event based on the params, even when the intro text is empty. From a first glance the changes should be obvious, but the impact it can have on existing extensions is too risky. So I'm closing this pr. Thanks for understanding.

avatar laoneo laoneo - close - 8 Sep 2023

Add a Comment

Login with GitHub to post a comment