[com_fields] Move some events from system plugin to content plugin v2

[piotr-cz]

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

Actual result

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

Documentation Changes Required

none

cc @laoneo

[laoneo]

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.

[piotr-cz]

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.

[mbabker]

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.

[piotr-cz]

@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.

[mbabker]

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.

[piotr-cz]

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.

[mbabker]

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.

[piotr-cz]

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.

[mbabker]

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).

[Bakual]

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.

[piotr-cz]

@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.

[Bakual]

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.

[laoneo]

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.

[Bakual]

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.

[laoneo]

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.

[ghost]

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.}

[ghost]

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.}

[fancyFranci]

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.}

[ghost]

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

[ghost]

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.}

[piotr-cz]

@franz-wohlkoenig ok, done

[ghost]

I have tested this item 🔴 unsuccessfully on 627a401

After applying Patch got in Frontend:

---

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

[ghost]

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

[fancyFranci]

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

[piotr-cz]

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

[ghost]

Issue stay open.

---

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

[fancyFranci]

@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.}

[piotr-cz]

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

Thanks for assistance.

[ghost]

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

[sudhir-j-sapkal]

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

[piotr-cz]

@sudhir-j-sapkal Please do so

[piotr-cz]

@fancyFranci done in 8287e8c

[laoneo]

Can you rebase it to 4.2. So we can test it again and get it in.

[laoneo]

Thanks for your contribution with this pr to make Joomla better. This pr is not anymore suitable for Joomla 3. Can you rebase it to the latest Joomla 4 branch so we can reevaluate it again? In the meantime we are closing it, when done, don't hesitate to reopen again.

[piotr-cz]

Sorry, it's taken far too long (5 years) and I'm on different stack now.
I hope somebody will take over this and prepare PR for Joomla 4