? ? Pending

User tests: Successful: Unsuccessful:

avatar n3t
n3t
17 Feb 2017

Purpose

In many templates there is need to add some extra information to modules. It could be aither some extra text (subtitle), extra settings (text color), or anything else. This PR adds custom fields support to modules, so template developers can easily add fields to their template.

Question is, if support of automatic rendering should be added, this would mean to add calls to appropriate events in modules helper and modify default chrome styles. However, every module has title and body, so events would logically fit.

Summary of Changes

Added com_fields support to com_modules

Testing Instructions

Installl the patch, go to modules manager, and try to add some fields, fields categories and set some fields for modules.

avatar n3t n3t - open - 17 Feb 2017
avatar n3t n3t - change - 17 Feb 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 17 Feb 2017
Category Administration com_modules Language & Strings
avatar franz-wohlkoenig franz-wohlkoenig - test_item - 18 Feb 2017 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - test_item - 18 Feb 2017 - Tested unsuccessfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 18 Feb 2017

I have tested this item ? unsuccessfully on 5434f1b
Tested a Text-Field on Login-Module. Saved all fine, but didn't display in Frontend. Set different Display-Settings.

Test on

Joomla! 3.7.0-beta3
macOS Sierra, 10.12.3
Firefox 51.0.1

MAMP 4.1.1

  • PHP 7.0.15
  • MySQLi 5.6.35

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14127.
avatar infograf768
infograf768 - comment - 18 Feb 2017

I confirm @franz-wohlkoenig findings. Same test he did.

avatar Bakual
Bakual - comment - 18 Feb 2017

As he wrote in the PR theee is no automatic display of the field (yet). So it is expected they don't show up automatically.
However they should be added to the module properties if rhe proper events are triggered.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 18 Feb 2017

thanks for Info, @Bakual

avatar Bakual Bakual - test_item - 18 Feb 2017 - Tested unsuccessfully
avatar Bakual
Bakual - comment - 18 Feb 2017

I have tested this item ? unsuccessfully on 5434f1b

While it works to manage the fields for a module, the fields are not added to the module item when rendered. So the important part is missing ?
The module rendering process needs to either trigger the onContentPrepare event for the module or the system-fields plugin needs to be adjusted so it also listens to the onRenderModule event. Otherwise it will not work at all.


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

avatar n3t n3t - change - 19 Feb 2017
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 19 Feb 2017
Category Administration com_modules Language & Strings Administration com_modules Language & Strings Front End Plugins
avatar joomla-cms-bot joomla-cms-bot - change - 19 Feb 2017
Category Administration com_modules Language & Strings Front End Plugins Administration com_modules Language & Strings Libraries Front End Plugins Templates (site)
avatar n3t
n3t - comment - 19 Feb 2017

I made some changes,

1/ field values were saving directly to module params, now it is saving to #__fields_values as in any other extension. Note that small com_modules change was needed, but it should not have any impact.
Also changes to fields system plugin were required, as com_modules does not use onContentBeforeSave, but onExtensionBeforeSave.

2/ I add AfterTitle, BeforeDisplayContent, AfterDisplayContent for modules displaying. Using OnRenderModule would need some changes, and will not cover all three places fields offer.
I also modified system template chrome functions to display these values.

3/ Finally I modified Protostar and Beez3 chrome functions to display these values.

Please test, different fields, different mmodules positions, different module rendering styles.

Note that for 3rd party templates chrome functions will need modifications to display fields values, but I think it is better, than to hardcode field values directly to module content, as template developers could have more options to implement fields displaying.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 20 Feb 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 20 Feb 2017

I have tested this item successfully on 3db858e

Tested:

  • Protostar Position-7, Position-8, Banner
  • Modules Login, Article-Latest, Banner
  • all System Rendering Styles and noand wellProtostar.

Test on

Joomla! 3.7.0-beta3
macOS Sierra, 10.12.3
Firefox 51.0.1

MAMP 4.1.1

avatar Bakual
Bakual - comment - 20 Feb 2017

It indeed works now and is saved correctly.

What I wonder is what the usecase will be. While I agree there is a need for templates to be able to define custom chrome fields, this isn't something which should be solved using custom fields. That's not what it is meant for.

On the other hand we will be triggering a lot of plugin events and database queries here for no reason (in most cases). For components, that is only one cycle of events we do per pageload, but for modules this will be multiplied a lot per pageload, thus I'm a bit hesitant.

avatar n3t
n3t - comment - 20 Feb 2017

@Bakual That is why I didn't implement automatic display first. My original thought was only to provide administration part, so template developers can easily add fields to module administration, which values could be used in their chrome functions. For example "Subtitle" or any other custom parameters. Same like could be set in DP Fields - just provide admin interface, and let template developer to handle fields values however he needs.

avatar mbabker
mbabker - comment - 20 Feb 2017

When it comes to integrating templates with other parts of the system, it seems like the template should be supplying a plugin that hooks into the existing events for altering forms to add that stuff versus relying on com_fields. There is a massive difference between custom fields as defined through the UI compared to fields as defined through the existing PHP APIs and template developers are going to lean much more heavily on the latter versus the former for advanced functionality.

Every time I see a notification for this PR in my email, my feeling of "this is really borderlining on a niche use case" gets stronger. Not saying it's a bad idea in general, but I keep swaying toward "this isn't a solution that should be part of core right now" over "how can we embrace this and get it merged". I think the workflow has to be more of a "template advertises the features it offers and integrates them throughout Joomla" approach over what I see custom fields doing which is a "here's all this data for you to work out how to integrate" system.

avatar n3t
n3t - comment - 20 Feb 2017

From my point of view, but it is maybe very specific, it would be nice to have one click possibility to add custom fields almost anywhere. Articles, users and categories are very basic, modules, banners, and even for example menu items will be great. Many tim, when I create template I find out I woud need to add some extra field for some part of Joomla, to make template more flexible and to avoid creating dozens of output overrides just to change one line of text, or to avoid creating custom plugin for every simple template I need to create.

On the other hand, I do not need automatic display at all, as I always handle these specific fields in my custom templates by myself. For this the functionality of original DP fields was great, just go to plugin, add additional component and that is all. It works even for many 3rd party components.

avatar mbabker
mbabker - comment - 20 Feb 2017

That's why I see merit in it as a feature. I'm just not sold on it at the moment is all. Not even taking into consideration the potential performance impact it may have as pointed out, just thinking about it solely as advertised.

avatar n3t
n3t - comment - 20 Feb 2017

I understand possible performance impact, again that is why I didn't want automatic display first. I some kind of template feature are available, it would solve my issues same way.

However still basic functionality should be prepared in the component, so the template just says 'Want to use extra fields for modules' in its manifest, and all is set up.

To avoid possible performance impact at this point I see two options

  • remove automatic display completely from core, if template developer want to enable automatic display, dispatcher calls could be implemented in templates chrome functions
  • add com_modules global option for automatic display - if it is off do not trigger new events at all
avatar Bakual
Bakual - comment - 21 Feb 2017

To avoid possible performance impact at this point I see two options

The main impact will likely not come from the automatic display events (those are quite lightweight imho) but from the fields processing itself (additional database queries per module). I haven't checked yet how much that is.

But the main point imho is that it's the wrong approach (com_fields) to solve a valid issue (custom template/module chrome options). Imho, those options should be defined by the termplate eg in the templateDetails.xml file. If you rely on com_fields, the user will have to create them manually after installing the template and name it specific to some template instructions. That's far from user friendly and not intuitive at all.
I thought about that issue already earlier and while it should be simple to allow some template specific parameters in module config, the main issue I see is how to do the UI. Because a site may have multiple templates installed with different such parameters. We will have to make sure they don't collide and it has to be clear for the user which parameter is used in which template/chrome. That's where I struggled and gave up.

avatar n3t
n3t - comment - 21 Feb 2017

If there is no automatic display, there will be no additional queries. I mean in frontend. In backend ofcourse yes, but on loading and saving module, which should not have big impact.

I agree, that fields definition is not perfect solution, but at least it is some solution.

According module parameters in template - currently it is possible to define custom menu type easily, by defining extra XML in template component override folder. By defining parameters in this template, it is possible to add extra parameters to menu item. What about if modules follows this logic, so it will display parameters (lets say custom fields or anything else), defined in xml file based on their selected layout?

However this will solve just modules, not for example banners, or any 3rd party component.

avatar Bakual
Bakual - comment - 21 Feb 2017

If there is no automatic display, there will be no additional queries.

Ah I see, that's because you don't run the onContentPrepare event at all. Which would be wrong anyway. The way you do it the fields will only be added to the $module after the module itself is already processed and just before the content is passed to the template chrome. Which means those fields can only be shown in the chrome and not in the module content itself.
The "automatic display" events should actually only display the values, the values should also be present as properties of the item without those events so they can be used wherever one wants. That's what the onContentPrepare event would do.

But this means a database query per module and field. So if you have a couple of fields and module the database queries count up a lot. For 3 fields and 5 modules you look at 15 additional queries.
Changes are you have even more modules the page (1-2 menu, search, footer, login, ...)
Keep in mind it doesn't matter if the field has a value set or not, it will have to be looked up for each module and field.

avatar n3t
n3t - comment - 22 Feb 2017

The way you do it the fields will only be added to the $module after the module itself is already processed and just before the content is passed to the template chrome.

Yes, exactly. It is module fields, not fields of its content. If content is "Custom content" module, it call onContentPrepare by itself, if any other 3rd party module, it can call it also. IMO fields in this case should be only for chrome functions.

avatar Bakual
Bakual - comment - 22 Feb 2017

I don't remember that there is a different context for "module content". And even if there would be one added, it would be very confusing and not nwe5ded at all.
So no, that will not work at all.

Sorry to say but it looks like you're trying to solve a real issue with the wrong tool and at the same time don't allow the tool to be used in its intended way.

I wouldn't merge this PR as it is. We need a better way to support template specific chrome parameters and the modules itself imho don't need custom fields.

avatar mbabker
mbabker - comment - 22 Feb 2017

We need a better way to support template specific chrome parameters and the modules itself

Can it not be done already with the existing plugin hooks for form related events? Or is the use of the plugin system for this too complex, not documented well enough, or some other issue? Yes, I realize that means you have to get your hands dirty in PHP while the custom fields approach is more end user friendly.

avatar n3t
n3t - comment - 22 Feb 2017

while the custom fields approach is more end user friendly
Exactly, while creating template with need of custom field, you will have to create additional plugin. Moreover, the plugin will do almost the same, or very similar, as custom fields, so my idea was, why to write the same code again and again, when there is ready solution in core. This was my original idea, just to provide admin interface, and if template creator wants to use it, just switch it on, and load custom filed value however and whereever he wants using FieldsHelper.

Maybe to make it less confusing, Automatic display switch in field configuration could be disabled (by components helper for example), or maybe admin interface could be switched on not based on settings in com_modules, but if there is at least one frontend template saying "I want custom fields for modules".

avatar mbabker
mbabker - comment - 22 Feb 2017

Except using custom fields in this case IMO makes it harder for the template to actually use the data, at least in my limited knowledge of how com_fields actually works. Because you're basically working with unkeyed arrays and non-standardized values, so at best all you can do is render the result of a plugin hook unless you're going to check every field value and find something loosely matching what it is you're actually looking for.

Custom fields is really intended for the non-developer types for things like building standardized layouts (we're working on a solution for our upgraded company site that allows our marketers to build landing pages by filling in a few fields) or possibly adding some form/CCK like elements to a site. I think this specific use case though of a template needing to add additional features to different contexts of the site is really better suited for the existing hook points for forms as that template is going to have to be more aware of what those extra integrations are. If you wanted me to add a parameter to add extra classes to the body tag in the template, I wouldn't add custom fields specific to each context; I would use the plugin system and hook the relevant forms.

avatar n3t
n3t - comment - 22 Feb 2017

Every custom field has alias, so you can access field values by its alias.

avatar mbabker
mbabker - comment - 22 Feb 2017

It still has to be a known value though. And for me, if I told a consumer
of my extensions or had an extension I use tell me I had to set up custom
fields to use their features versus them adding support on their own, I'd
call it a fail.

Fields has a use case of a user adding extra stuff to an extension without
needing to be PHP savvy. So from that aspect this is a good idea. Fields
isn't a replacement for devs to integrate features. IMO it has too many
unknowns for a wide distributed extension to rely on.

This really needs to be thought through and well communicated. Right now
it is adding architecture without an implementation (yes I get the
implementation exists in the core module chromes but how does a user know
what template or chrome supports this?). From that aspect I'd find it
difficult to work with as an end user.

In general I'm not against this idea. I just am still not convinced this
is the right way to go about things.

On Wed, Feb 22, 2017 at 8:44 AM n3t notifications@github.com wrote:

Every custom field has alias, so you can access field values by its alias.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#14127 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfobwb9aeSab87uhQZLvulQlAudu0mks5rfEnUgaJpZM4ME2NQ
.

--

  • Michael Please pardon any errors, this message was sent from my iPhone.
avatar Bakual
Bakual - comment - 22 Feb 2017

Can it not be done already with the existing plugin hooks for form related events?

I think it should be possible, since com_fields actually doesn't do any different at this point. An own plugin would actually have the advantage that you can store the value in the modules params db field. During display, that value will automatically be present and ready to be used.
But it means the template has to ship with a plugin just for this parameter(s). That's the only downside I see. Still better than misusing com_fields.

avatar mbabker
mbabker - comment - 22 Feb 2017

Considering a lot of the mass distributed templates are already shipping with frameworks that most likely include at least a library and probably some form of plugin I don't think that's adding too much onto their workload. For something like our joomla.org site template that's just a template it would be a "downside" to include a plugin. But considering that's also a more explicit custom build versus something built for mass market, it would also probably fit the niche of using com_fields over the plugin route too.

avatar Bakual
Bakual - comment - 22 Feb 2017

True enough. Although I dislike those framework based templates and would like to offer ways to reduce the need for them ?

avatar n3t
n3t - comment - 22 Feb 2017

Exactly, mass templates had their own frameworks, this kind of solution would not be for it propably. For custom templates it would make creating template simpler, extra plugin does not need to be provided, and changed every time extra field is needed.

avatar Bakual
Bakual - comment - 22 Feb 2017

Still, this solution is the wrong approach. Please think about another solution for that issue. One which doesn't need user interaction and misusing of com_fields.

avatar n3t
n3t - comment - 22 Feb 2017

Ok, for now I'll close this pull request.

avatar n3t n3t - change - 22 Feb 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-02-22 20:05:39
Closed_By n3t
avatar n3t n3t - close - 22 Feb 2017

Add a Comment

Login with GitHub to post a comment