I have tested this item ? unsuccessfully on 27299d6

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

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

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

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

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.

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

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.

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

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

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

@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

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

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.

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

@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

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

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 #36275 as an alternative

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

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

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.

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

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

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.

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

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

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?

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

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

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

@ceford The answer to both questions is: No. I restart drone now to see if it passes this time.

@ceford Drone restart did not help .. now I have used the button to update the branch. Let's see if that helps.

Possibly you created your branch at a time (or when it was once for the 4.0-dev branch then time when it was rebased to 4.10-dev) when in the 4.1-dev branch there was an error in an SQL script for new installations? At least the output from the system tests look as if that was the case.

So or so it was not your fault or your mistake.

@ceford After having kicked the drone a few times, it passed. Now in case if you need to make some modifications to that PR, pull the changes from remote before working in order to get the updates from the branch update.

This pull request has automatically rebased to 4.2-dev.

This pull requests has been automatically converted to the PSR-12 coding standard.

@crystalenka something for you. We talked about this last week iirc

Thank you for your ideas.

We discussed it in CMS Maintenance Team and we think that the way is to use overrides / alternative layouts.