Wont this break all existing sites relying on content plugins

On 28 July 2016 at 17:23, Sabuj Kundu notifications@github.com wrote:

Pull Request for Issue # .
Summary of Changes

Changed context
Testing Instructions

I noticed that, context is used wrong, "com_content.article" context is
used in core component and the module should use something different,
otherwise it's no use of using context ! Content plugin developer may show
something for component but not for module.

So I changed context to "mod_articles_news.article"

You can view, comment on, or merge this pull request online at:

#11339
Commit Summary

• mod_articles_news Context update

File Changes

• M modules/mod_articles_news/helper.php https://github.com/joomla/joomla-cms/pull/11339/files#diff-0 (6)

Patch Links:

• https://github.com/joomla/joomla-cms/pull/11339.patch
• https://github.com/joomla/joomla-cms/pull/11339.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11339, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8XpW76vN3KU1zXpWmA9oaxTy1lNTks5qaNeQgaJpZM4JXZKq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

hi @brianteeman the answer can be from different sense

1. This is breaking something plugins people creating new, the concept of context is used wrong here and if we don't fix it now then it will break more in future.

2. Practically what will happen, if any plugin checked the context in code then it will fail return nothing and it's failsafe. The plugin developer can fix this in next release.

3. Otherwise while we plan any content plugin for component, same hooked output is going in module which we don't want.

Example: I want to show related articles after the any single/details article it's not wise to show the same thing in module where it's used to show just the latest news !

As a professional joomla extension developer I think we should fix this and move forward, otherwise the reason the context param was added in content plugin hook will go in vain.

thanks

I agree with @manchumahara. It is a kind of BC break, but it is important. It is better for the concept of the context parameter. Also it gives a developer more flexibility to hook specially in on this point in the program.

I think we have to make this change

Hmm is there a problem that we call both events? At least for b/c reasons and add a log statment that this context is depcrecated?

I dont't know. If I used this event I should do something like this:

if ($context != "com_content.article")
{
    return true;
}

if ($context == "com_content.article")
{
    // Do stuff
}

If we changed the context this code returns true, and there is no modification on the content of this module. Exactly what we want, so we have control on this code and the modifications. But if I expect that this plugin modifies the module it is not what we want.

But if we call the event twice, once for the com_content context and once for the mod_article_news context, all the plugin methods will be also run twice. Assuming all plugin check the context that will it won't be a big problem I think so?

@manchumahara
Thanks for your efforts and ideas

and of course i see the usefulness of this change

• but it breaks B/C totally
• and is not best / most flexible way to do it (also i think it is wrong way to do it)

It is 1 thing breaking B/C by mistake, but breaking it with knowledge of it, why ?

• only good reason would be security / big performance issue / major new feature with small B/C break

Sites using the module combined with plugins will have their display broken,
and the user will complain to plugin makers,
and plugin makers will update their plugins (hopefully fast enough)
and then things will work again

It is not that this PR annoys me, it is that it makes me worried, that B/C lately means less

Also using the context: "mod_articles_news.article" is not correct way to do it

Instead it is better to extend the API as this will be

• be B/C friendly
• more flexible

public function onContentBeforeDisplay($context, &$row, &$params, $page=0)

public function onContentBeforeDisplay($context, &$row, &$params, $page=0, $location=null)

Then if some plugin cares to have different behaviour for display in a module position ,

it will check location for location being:
mod_*

If it is mod_* then article is displayed at module position,
and then if plugin cares about it may even care to check for specific module:
$location=='mod_articles_news'

-1

That is exactly what the context parameter is for. Introducing yet another
parameter because someone screwed up royally is not the fix. So either
live with the B/C break in the middle of 3.x, or push it until 4.0 and
devise another way to check context (debug_backtrace() will help here) and
basically stop using the context parameter in your code.

On Saturday, July 30, 2016, Georgios Papadakis notifications@github.com
wrote:

@manchumahara https://github.com/manchumahara
Thanks for your efforts and ideas

and of course i see the usefullness of this change

• but breaks B/C totally
• and is not best / most flexible way to do it (also i think it is wrong way to do it)

It is 1 thing breaking B/C by mistake, but breaking it with intension,
why ?

• only good reason would be security , or a major new feature with small B/C break

Sites using the module combined with plugins will have their display
broken,
and they will complain to plugin makers,
and they will update their plugins (hopefully fast enough)
and then things will work again

It is not that this PR annoys me, it is that it makes me worried, that B/C
lately means less

Also using the context: "mod_articles_news.article" is not correct way
to do it

Instead it is better to extend the API as this will be

• be B/C friendly
• more flexible

public function onContentBeforeDisplay($context, &$row, &$params, $page=0)

public function onContentBeforeDisplay($context, &$row, &$params, $page=0, $location='')

Then if someplugin cares to have different behaviour it will check
location for:
'mod_*'

if it is mod_* then article is displayed at module position, and then if
plugin cares about it may even care to check for specific module:
$location=='mod_articles_news.article'

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11339 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoSsTcLnwl7__y1P2wd4afh2_ium1ks5qa3T-gaJpZM4JXZKq
.

At least we agree it is a B/C break

about the new parameter 'location' it was just a thought

• a thought, if this is considered important enough to do now without a B/C break

Argument that it is not worthy to do it, ok, sure

About context i really hope you do not mean context like:

mod_articles_news.article

as the checking on such context, starts to get weird and inflexible in terms of checking it
and excuse me to say hack-looking without a good reason

Thinking (for future) if this would be better something like:
com_content.article@mod_articles_news

whatever just not: mod_somename.something
which i am afraid it is already used in some places ?

Truth be told the naming and structure with the context argument feels
funny. You can't name it well to include both an origination point and
what is being broadcast. mod_articles_news.article might cover it but an
article of what type is being broadcast here?

I don't think separate parameters for originating point and context is the
right answer. And I don't think excessively long context values works
either. For events passing a data object, you already have a record so you
can ascertain a type pretty easily leaving the context to tell the
originating point.

On Saturday, July 30, 2016, Georgios Papadakis notifications@github.com
wrote:

At least we agree it is a B/C break

about the new parameter 'location' it was just a thought

• a thought, if this is considered important enough to do now without a B/C break

Argument that it is not worthy to do it, ok, sure

About context i really hope you do not mean context like:

mod_articles_news.article

as the checking on such context, starts to get weird and inflexible in
terms of checking it
and excuse me to say hack-looking without a good reason

Thinking (for future) if this would be better something like:
com_content.article@mod_articles_news

whatever just not: mod_somename.something
which i am afraid it is already used in some places ?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11339 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoTIsXDT1IDiCVFPfQHTqhVfA4GDgks5qa4XngaJpZM4JXZKq
.

I agree and same time disagree. But I need better and practical logic + want to follow joomla standard architectural flow.

1. At first let me talk about the break. In joomla 3.6 the "editors-xtd" type plugin was moved to tinymce where for other editor like Editor-Codemirror, Editor-None has the old style loading of the buttons. I think some people thought it will be great but none thought that time that it may break some plugins. Don't think that every body will fire popup or just insert somthing into the editor using those buttons but can do more things that is involve with js, css and integration. When those buttons load in tinmce editor they get different class name and things are changed, when they are displayed under the editor they shows different class name in html. Again for user experience, when there is editor type plugin it's very much odd to break UX of displaying the buttons. I had 4/5 editor-xtd type plugin and they all broke but I made them compatible with the latest joomla to move forward, I meant to move forward.

2. Let me show what the joomla content plugin event docuementation page says. https://docs.joomla.org/Plugin/Events/Content

context = The context of the content being passed to the plugin - this is the component name and view - or name of module (e.g. com_content.article). Use this to check whether you are in the desired context for the plugin.

So the above description says, the context will be who is firing the event. pattern com_xyz.view_name , here there is no proper example for module, so it sohuld mod_xyz.layout_name or module can use the same layout name for component.

1. Com_content is not the only component that uses onContentAfterDisplay or such event, any component can use such event and the context makes says from which component and which view the event is fired. same type the it should go with module.

2. Let me show how the current core breaks the law. the event onContentPrepare is done

in Com_content article view

$dispatcher->trigger('onContentPrepare', array ('com_content.article', &$item, &$item->params, $offset));

Same thing in mod_article_news

$item->introtext = JHtml::_('content.prepare', $item->introtext, '', 'mod_articles_news.content');

So, any one explain me why context "mod_articles_news.content" is used in module for same event type where 'com_content.article' in joomla core com_content's article view. Don't you think this change d context "mod_articles_news.content" is used to make a difference ? Then why do you think it's not ok to change same way for onContentBeforeDisplay or onContentAfterDisplay

1. So, if there is any thing wrong before, sometimes it's need to take hard way to fix it and force extension developer to update it.

2. As extension developer helping joomla (also same time taking help from joomla) to be more useful to end users joomla should think about them.

7.

$item->event = new stdClass;
$results = $dispatcher->trigger('onContentAfterTitle', array('com_content.article', &$item, &$item->params, $offset));
$item->event->afterDisplayTitle = trim(implode("\n", $results));

  $results = $dispatcher->trigger('onContentBeforeDisplay', array('com_content.article', &$item, &$item->params, $offset));
  $item->event->beforeDisplayContent = trim(implode("\n", $results));

  $results = $dispatcher->trigger('onContentAfterDisplay', array('com_content.article', &$item, &$item->params, $offset));
  $item->event->afterDisplayContent = trim(implode("\n", $results));

7.1 If we keep the current code base -> who wants to show something only for component will also appear in module or vice versa.
7.2 if we change the above event's context in module it will give more power to extension developer.
7.3 if context is changed, those who doesn't check context will not break. but who checks context(I think every one should check context to write a better content plugin ) will not be able to show output in module but it's fail safe (you can call it break but not harmful)

1. I don't understand why we need to care what is broadcast as content or joomla core com_content components shows a standard ways how to build the params array for event like onContentAfterDisplay and every 3rd party extension developer follows that. We should think who is broadcasting or who is the originator as these https://docs.joomla.org/Plugin/Events/Content events can be used in any component.

2. Any one please provide a better solution that solves the purpose of 7.2 and 7.3 I will agree and appreciate

Notes: Events or hooks are provided to extend joomla, extension developers use them. My views are from a extension developer and helping joomla to move forward.

This PR is no different than #8610 of course, just a different context.

@roland-d this PR is not for testing anymore?

@franz-wohlkoenig I think, like the other PR I mentioned, is more likely to see life in Joomla 4 rather than in 3,x series.

@roland-d can there be a Label like "4.0" to this and similar PR?

@wilsonge This relates to your PR #12289 Do you want this included in Joomla 4 or can we close this?

@wilsonge can you please answer Comment above by @roland-d?

This is a good change for J4.

@wilsonge thanks for Comment on @roland-d.

@manchumahara I know this is a very old PR but do you want to update it or should someone else take over? Thank you.

@roland-d please someone take lead for this pr, sorry for my late reply.

@roland-d I think plugins context's checks will need to be updated with this - we should keep the existing plugins to still apply after this patch

$dispatcher->trigger('onContentPrepare', array ('com_content.article', &$item, &$item->params, $offset));
Same thing in mod_article_news
$item->introtext = JHtml::_('content.prepare', $item->introtext, '', 'mod_articles_news.content');116.pdf

@franz-wohlkoenig Please delete the spam comment

What should happen to this PR?
I once tried a solution for 4.0 #27930. Nobody was interested in that.
There are conflicts here now. @manchumahara Are you still interested in the PR or can it be closed?

1st of all thank you for your pr
i really wish you'll find the time to propose this for the j4 branch