PR-staging

Failure

User tests: Successful: Unsuccessful:

avatar piotr-cz
piotr-cz
24 Mar 2017

Pull Request for Issue -.

Summary of Changes

Plugin events are triggered by the dispatcher in order specified by the plugins administration component. However some plugin types like system plugins are dispatched before content plugins.

The pull requests moves onContentAfterTitle, onContentBeforeDisplay and onContentAfterDisplay from fields system plugin to fields content plugin - so it's possible to define where output will be rendered on the page.

I've prepared an example where I'm using both vote plugin and fields plugin to show a gallery. Both are configured to show on onContentBeforeDisplay event.
The goal is to

Testing Instructions

  1. Switch On the vote plugin
    Extensions > Plugins > Content - vote, make sure the Position is set to Top
  2. Add new Custom field
    Content > Fields > New, make sure the Automatic Display is set to Before Display
  3. Change ordering o plugins
    Extensions > Plugins > Search Tools
    Select Type: Content, Sort table by: Ordering Ascending

Expected result

User can choose the order of content plugins to display
j-fields-after

Actual result

The fields plugin output is always displayed before any other content plugins.
j-fields-before

Documentation Changes Required

none

cc @laoneo

avatar piotr-cz piotr-cz - open - 24 Mar 2017
avatar piotr-cz piotr-cz - change - 24 Mar 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Mar 2017
Category Front End Plugins
avatar laoneo
laoneo - comment - 24 Mar 2017

Nice one. Guess it's the only way to use the fields after some other content plugin. Should we not move all onContent events to this plugin? The next move would be then to move the user events to a user field plugin and the finder to also a separate plugin. Line that we can eliminate the system plugin.

avatar piotr-cz
piotr-cz - comment - 24 Mar 2017

Probably yes, we should move all the event listeners but let's hear others opinions - maybe it should be done in another PR.

Moreover this is more correct when it comes to syntax - content events should be set in content plugins.

avatar mbabker
mbabker - comment - 24 Mar 2017

Honestly, this is a bandaid fix. We really need to improve the UI management as a whole. It really should be possible to globally order plugins, not be locked in based on the group they are imported with.

avatar piotr-cz
piotr-cz - comment - 24 Mar 2017

@mbabker It's up to another discussion if user should be able to change plugins order outside their types/ groups.
I see some sense in that system plugins are triggered before content plugins.

For now just should make sure that plugins don't register events that are not supposed to - like here system plugin registering content events.

avatar mbabker
mbabker - comment - 24 Mar 2017

There is nothing saying "X plugin group should not register events in Y plugin group". I actually think that would be a bad standard to enforce, then again I've also made it clear in the past that I'm not seeing the benefit of this group structure anymore other than to limit the number of loaded event listeners based on an arbitrary code path which results in weird behaviors and inconsistent results.

avatar piotr-cz
piotr-cz - comment - 24 Mar 2017

The only and IMHO important benefit I see is that you may easily filter plugins by group in the administration area (this is really helpful as there are around 66 native plugins).

This really helps when you want to see which plugins are manipulating the content, disable all quickicons plugins and so on.

But again each group should register own events otherwise filtering functionality is useless.

avatar mbabker
mbabker - comment - 24 Mar 2017

And I'd say the ordering functionality is flawed. You should be able to order plugins globally, not only within a specific filter group.

We should not be continuing to promote the mentality of "system plugins only for onAfterRender" or "content plugins only for onContent*" type logic.

The filtering logic is good for the UI. But IMO it doesn't translate as well into the code level.

avatar piotr-cz
piotr-cz - comment - 24 Mar 2017

To sum up: to fix the issue I've described we can either implement global ordering of plugins or keep events within groups like in the above commit.

avatar mbabker
mbabker - comment - 24 Mar 2017

For where we're at now, it's fine. I'd just like to see us move away from creating an artificial separation of these things and make plugins and their grouping/ordering integrate better at a global scale (so you can order a content plugin before a system plugin for when those have been imported, though I'd still like to phase out importing of different groups at different times but that's probably a battle I'll never win).

avatar Bakual
Bakual - comment - 24 Mar 2017

The downside of this is that you can't turn of the content plugin if you don't want it to be parsing the article content for possible field tags. You would turn off also the automatic display.
Other than that it should be fine.

I wouldn't extend it to other events. In this case it's fine since the content plugin already exists, but if you have to create a new field plugin for eg user it gets out of hand fast imho.

I also agree with Michael that there is a serious flaw in our plugin system. It would be great if plugins really would "register" the events they want to listen to. Currently it's the dispatcher checking each plugin if there is a matching method specified, which is also why we have the groups. So the dispatcher doesn't have to always look into hundred plugin classes to see what methods they support.

avatar piotr-cz
piotr-cz - comment - 24 Mar 2017

@Bakual You are right, I didn't know that there is a functionality of inserting fields using {field} and {fieldgroup} tags.

But still I think that the PR is fine as it is. If somebody won't want to display fields, then (s)he should disable content plugin and it will be disabled for both cases - for manual tags as well as for automatic display as it refers to content.

avatar Bakual
Bakual - comment - 24 Mar 2017

If somebody won't want to display fields

If someone doesn't want to display fields, he should disable the system - fields plugin because that one actually does the bulk of the work (and performance hit).
With this PR, you would always either have both plugins enabled or disabled. Having one enabled and the other disabled doesn't make any sense.

Just some explanation: The content plugin exists because I created it to support those field tags in articles. It goes together with the fields editor button plugin. This feature was added after fields were already working. It is the sole purpose of that plugin currently.

I get the need for the ordering, and for that I can see the purpose of this PR as a workaround because we currently can't order the plugins across groups.
Imho the better fix would be to allow the ordering of plugins independant of plugin group. But that may be tricky as well, I don't know.

avatar laoneo
laoneo - comment - 25 Mar 2017

you would always either have both plugins enabled or disabled

Not necessarily, with the content plugin disabled, you can turn off automatic display only. The rest is still working as expected.

avatar Bakual
Bakual - comment - 25 Mar 2017

Yeah. You can still add values to the fields and create them. But without template overrides, the values will not show anywhere.
So I think it is at least a rare case where only the content plugin would be disabled.

avatar laoneo
laoneo - comment - 25 Mar 2017

It's true for the core, but perhaps some 3rd party extensions only want to work on the fields array and don't want to automatic display. I know it is just guessing. But somehow I like the idea to not have a global system plugin which is initialized on every page request, but only plugins which are actually needed.

avatar franz-wohlkoenig franz-wohlkoenig - change - 7 Apr 2017
Category Front End Plugins com_fields Front End Plugins
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 5 Jun 2017

I have tested this item successfully on 887a0cf


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

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 5 Jun 2017 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 26 Oct 2017
Easy No Yes
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Oct 2017

This is an easy Test (having fine Test Instructions) which needs a second Test.


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

avatar FPerisa FPerisa - test_item - 19 Dec 2017 - Tested successfully
avatar fancyFranci
fancyFranci - comment - 19 Dec 2017

I have tested this item successfully on 887a0cf

The patch sets the voting above the custom field.
But unlike in the instructions, I needed to set the ordering inside the vote plugin and not in the ordering of plugins.


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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 19 Dec 2017

@piotr-cz can you please resolve conflicting File so this Pull Request can be set "Ready to Commit"?

avatar franz-wohlkoenig franz-wohlkoenig - change - 19 Dec 2017
Status Pending Information Required
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Dec 2017

can anyone than @piotr-cz resolve conflicting Files?


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

avatar joomla-cms-bot joomla-cms-bot - change - 28 Dec 2017
Category Front End Plugins com_fields Front End Plugins
avatar piotr-cz
piotr-cz - comment - 28 Dec 2017
avatar franz-wohlkoenig franz-wohlkoenig - test_item - 28 Dec 2017 - Tested unsuccessfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 28 Dec 2017

I have tested this item 🔴 unsuccessfully on 627a401

After applying Patch got in Frontend:

bildschirmfoto 2017-12-28 um 10 59 17


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14875.
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 Jan 2018

If this PR get no Response, it will be closed at 28th February 2018.

avatar fancyFranci
fancyFranci - comment - 15 Feb 2018

@piotr-cz please just add the two missing use Joomla\CMS\... statements and it will work again.

avatar piotr-cz
piotr-cz - comment - 15 Feb 2018

@FPerisa which ones?
Sorry, but I'm out of loop for long time

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 28 Feb 2018

Issue stay open.


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

avatar fancyFranci
fancyFranci - comment - 28 Feb 2018

@piotr-cz
You need to move all three use statements

use Joomla\Registry\Registry;
use Joomla\CMS\Factory;
use Joomla\CMS\Language\Multilanguage;

from system\fields\fields.php to content\fields\fields.php (and remove it from the first one).

But coming from another testing, I found a new error. The "/tagged-items" page throws an exception, because you need to copy the system fields private function prepareTagItem($item) to the content one too!

But after all this it is finally working :)


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

avatar piotr-cz piotr-cz - change - 1 Mar 2018
Labels Added: PR-staging
avatar piotr-cz
piotr-cz - comment - 1 Mar 2018

@FPerisa
I've moved these dependencies and copied prepareTagItem method over to content plugin.

Thanks for assistance.

avatar franz-wohlkoenig franz-wohlkoenig - change - 7 Apr 2018
Status Information Required Pending
avatar ghost
ghost - comment - 19 Jul 2019

@piotr-cz can you please resolve the conflicting files so this can be tested?

avatar franz-wohlkoenig franz-wohlkoenig - change - 19 Jul 2019
Status Pending Information Required
avatar sudhir-j-sapkal
sudhir-j-sapkal - comment - 2 Nov 2019

@franz-wohlkoenig - Is it OK to send separate PR with solving the conflicts , If @piotr-cz is not available ?

avatar piotr-cz
piotr-cz - comment - 2 Nov 2019

@sudhir-j-sapkal Please do so

Add a Comment

Login with GitHub to post a comment