Revised field_group with new base

[ceford]

Pull Request for Issue #31385 and related to #31396. Revised version of #31408

Summary of Changes

Added code so that field groups can have alternatives to ul layouts and headings.

Testing Instructions

Apply the patch

Create two field groups: Ingredients Set Options / Render Tag to ul Procedure Set Options / Render Tag to ol

Create fields with Ingredients as Field Group with Options / Render Options / Label set to Hide (Optional) Item 1 Item 2 Item 3

Create fields with Procedure as Field Group with Options / Render Options / Label set to Hide (Optional) Stage 1 Stage 2 Stage 3

Edit an article Fill out the Ingredients form Tab Water Tea Milk Fill out the Procedure form Tab Boil the water Pour the water on the tea Add the milk

Actual result BEFORE applying this Pull Request

A single unordered list

Expected result AFTER applying this Pull Request

Documentation Changes Required

Yes - I will do that when merged.

[brianteeman]

I have tested this item :red_circle: unsuccessfully on 27299d65bbff2baca007d61a2267976eec340e76

Tested with blog sample data and the front end displays

Warning: Undefined property: stdClass::$group_params in C:\htdocs\joomla-cms\components\com_fields\layouts\fields\render.php on line 77

---

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

[ceford]

Sorry, I am confused - is that extra tab before ->bind supposed to be there or not?

[ceford]

When fixing the white space in FieldsModel.php I inadvertently reverted the line that caused Brian's test to fail.

[dgrammatiko]

This is totally the wrong solution. A proper solution is to have Layouts that are selectable at the field group level. Then, since layouts are overridable, people could create a simple override for every case. In sort, please use the existing platform architecture instead of monkey patching things...

My 2c

[richard67]

Sorry, I am confused - is that extra tab before ->bind supposed to be there or not?

See my new review comments, this time with suggestions.

[brianteeman]

The biggest problem with this PR is that by using default values it changes all existing field groups

[Quy]

Sorry, I am confused - is that extra tab before ->bind supposed to be there or not?

In general, please do not include unrelated changes such as coding style in order to not pollute the PR. It should be done in a separate PR. Thanks.

[ceford]

I give up! What seemed like a simple problem a year ago, only having bulleted lists available for fields even when the field is an image, remains the status quo. See #31396

---

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

[richard67]

@ceford Please don’t give up. It was not my intention to block you, I only wanted the unrelated code style changes to be reverted so that drone passes. Sorry if I have confused you with that.

[Quy]

@ceford Please don't be discouraged and take it the wrong way. My suggestion is to only help you in your contributions which we appreciate very much!!!

[dgrammatiko]

@ceford so I checked the existing code and I found some interesting things

• The templates overrides creator correctly lists the layouts for com_fields:

• so an override is just a click

• then anyone can do whatever they want in that file.

Eg my test code on templates/cassiopeia/html/layouts/com_fields/fields/render.php was

templates/cassiopeia/html/layouts/com_fields/fields/render.php

<?php
/**
 * @package     Joomla.Site
 * @subpackage  com_fields
 *
 * @copyright   (C) 2016 Open Source Matters, Inc. <https://www.joomla.org>
 * @license     GNU General Public License version 2 or later; see LICENSE.txt
 */
defined('_JEXEC') or die;

use Joomla\Component\Fields\Administrator\Helper\FieldsHelper;

// Check if we have all the data
if (!array_key_exists('item', $displayData) || !array_key_exists('context', $displayData))
{
	return;
}

// Setting up for display
$item = $displayData['item'];

if (!$item)
{
	return;
}

$context = $displayData['context'];

if (!$context)
{
	return;
}

$parts     = explode('.', $context);
$component = $parts[0];
$fields    = null;

if (array_key_exists('fields', $displayData))
{
	$fields = $displayData['fields'];
}
else
{
	$fields = $item->jcfields ?: FieldsHelper::getFields($context, $item, true);
}

if (empty($fields))
{
	return;
}

$output = array();

foreach ($fields as $field)
{
	// If the value is empty do nothing
	if (!isset($field->value) || trim($field->value) === '')
	{
		continue;
	}

	$class = $field->name . ' ' . $field->params->get('render_class');
	$layout = $field->params->get('layout', 'render');
	$content = FieldsHelper::render($context, 'field.' . $layout, array('field' => $field));

	// If the content is empty do nothing
	if (trim($content) === '')
	{
		continue;
	}

	$output[] = '<div class="field-entry ' . $class . '">' . $content . '</div>';
}

if (empty($output))
{
	return;
}
?>
<div class="fields-container">
	<?php echo implode("\n", $output); ?>
</div>

Which produces:

Source HTML code:

In short, the proper way already exists...

[ceford]

White space problem fixed.

@brianteeman the title tags were those for which I could see a use case. hx would be complicated by the need to get the hierarchy right and almost certainly extra css. Existing field groups use ul, which is why it is the default. dl would take us back to J3!

@dgrammatiko thanks for the extra explanation. In this case I do not see how using a template override can reproduce the Ingredients / Procedure lists in my testing instructions because there is nothing in the Field Group parameters to tell the layout which tag to use.

[dgrammatiko]

Just add in the fields group one, one field named layout where you can select one of the available layouts. That’s the solution, adding fields in the field level is not the right approach

[brianteeman]

@ceford the same argument about the title tags heirarchy could have been made about modules yet we have the option there. Trust me if you offer one then you can guarantee users will ask why not the other - especially when it is called a title and yet there are no traditional title tags in the list

[brianteeman]

@ceford You have identified a very valid issue and we do need the ability to select a layout for a field group.

You can then include some standard group layouts eg ul, dl as you have already created AND the site owner has the ability to create their own layouts for the group

[dgrammatiko]

@laoneo can you have a look here. The aim is to have a layout field in the group level

[brianteeman]

@dgrammatiko just needs a new field - basically a clone of https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Field/ComponentlayoutField.php

and then add the layouts

[laoneo]

@laoneo can you have a look here. The aim is to have a layout field in the group level

On the front there is no distinction to render the fields per group yet. Groups are only used in forms for different tabs. This would be a new way to render fields per group and not just flat as it is now.

[dgrammatiko]

This would be a new way to render fields per group and not just flat as it is now.

Could you implement it? My objection here is that the fields try to control the container, which should be considered the wrong approach in any well-designed system. Usually, we want to follow the hierarchy, outer -> inner

Anyways there's https://github.com/joomla/joomla-cms/pull/36275 as an alternative

[laoneo]

@ceford would it be ok to switch to ##36275 which is more inline with the Joomla4 way to have as less parameters as possible and do more stuff in layouts?

[dgrammatiko]

@ceford I don't want to take over your PR therefore you can use the code from my PR and continue here. I just open the other one to showcase a possible solution (obviously the code needs some more refinement)

[ceford]

Take over / switch to #36275 - my first reaction is yes, I would be content for someone else to take this forward. On the other hand I guess you could all do without the distraction. I can see that there is a layout for each tag type so end users don't have to create overrides. I applied the patch and my fields disappear! So there is still some work to do. Give me a day to see how far I get.

[ceford]

@laoneo @dgrammatiko Can I have some advice please? Look at lines 440 onwards in plugins/system/fields/fields.php - at that point there is a single list of fields and they are all rendered with the same layout as the first item in the list. So if there are two field groups involved the list needs to be split into two lists that will be rendered separately. Seems a bit clunky but I do not see where else the list might be encoded as an array. So at the moment I propose to split the list there. Good/bad proposal?

[laoneo]

@ceford would be better you leave your pr in the initial state, so people can decide which way to go. And help bringing #36275 to a stable state. So we do not have two pr's with the same code. No need to make your pr a copy of the other. So can you revert your last commit?

[ceford]

Revised to use code provided by @dgrammatiko - currently waiting for automated tests to complete. Example outcome:

In this example I have left in the label of the first item in each group, just to show items with and without labels. The group title is set as h3. The last item, the image, is a field not in a field group. It uses ul by default. If the bullet is not wanted it could be styled out. Or the image could be put in a group. The default ul appearance, which is a change since J3, is what started this PR a year ago.

[ceford]

@laoneo I don't know what to make of your last comment! It was my understanding that @dgrammatiko was leaving this to me with the suggestions that I use his code. That is what I have done. Are you suggesting that I revert to my monkey-patched version? For me, that was more satisfying but it would leave dgrammatiko to complete his version - it does not include group titles.

[AndySDH]

@laoneo can you have a look here. The aim is to have a layout field in the group level

On the front there is no distinction to render the fields per group yet. Groups are only used in forms for different tabs. This would be a new way to render fields per group and not just flat as it is now.

Not true, you can do {fieldgroup 5} in your editor to render fields from fieldgroup with ID 5.

[ceford]

I am not sure what to do about the This branch is out-of-date message. Should I be fixing that on my clone and then pushing that to my fork? I merged the latest into my clone and then switched to this pr branch. Should I have merged into my local pr branch instead?

[richard67]

@ceford As long as there are no conflicts shown in addition, you can ignore that branch out-of-date message.

[ceford]

Are the drone errors something I can fix? Is that the result of the out-of-date message?