custom_elements_ui: re-implement with new concept

I finished with the initial implementation. I can't say that implementation is complete but it's ready for testing basic functionality.
Before continuing I'd like to get this reviewed and tested to get familiar with it before further discussion.

Open questions:
1. For field output configuration it uses formattes plugins and I added 2 dummy formatters for "raw" and "auto".
We need to think what will be best final solution:
- go with formatters;
- introduce new plugin type (with the possibility to reuse formatters as derivatives or maybe some tricky plugin discovery)

2. "Is slot" implementation.
- will we have some conditional formatters available depending on if the field is slot or not?
- consider creating two regions for attributes and slots instead of one

3. Discuss rendering logic:
- how entity_ce_display work together with core's entity_view_display?
- how entity_ce_display work together with existing processors? will it replace processors at some point?

4. Test coverage

5. helper functionality:
- delete entity_ce_display entities when somebody deletes bundle
- delete entity_ce_display entities on hook uninstall
- auto-generate/sync entity_ce_display from entity_view_display

6. ExtraFields support is not tested. is needed?

For reference, the original concept: https://www.drupal.org/project/custom_elements/issues/3295753 →

// Generate entity_ce_display entity from appropriate entity_view_display
// when open form for the first time.
if (empty($entity_ce_display)) {
// @todo Generate default in a better way?
$entity_view_display = $this->entityDisplayRepository->getViewDisplay($entity_type_id, $bundle, $mode);

We should not re-invent the wheel but do the same as core does with entity-view-displays. Or does speak anything against that?

We need to think what will be best final solution:
- go with formatters;
- introduce new plugin type (with the possibility to reuse formatters as derivatives or maybe some tricky plugin discovery)

Let's discuss today.

2. "Is slot" implementation.
- will we have some conditional formatters available depending on if the field is slot or not?
- consider creating two regions for attributes and slots instead of one

I don't think it's necessary. Field formatters produce html, so can always go fine to slot content, just as props.
Only "Auto" does not go well with "Is slot", so we could hard-code it to grey it out. Raw text can go with in a slot as well.
Thus, only valid data we can produce should be what we allow to be set as $value when adding CE-Slots, i.e. markup, raw data or nested CE for structs.

3. Discuss rendering logic:
- how entity_ce_display work together with core's entity_view_display?
- current solution to avoid an endless rendering loop is not perfect. Needs to be re-think.
- how entity_ce_display work together with existing processors? will it replace processors at some point?

I think there is no reason to change what we have planed / done so far. The default entity processor should take the CE-display-config by default. If there is none, the fallback can be rendering nothing. We can consider an update-hook writing default-config to ease updating, but let's leave this out for now. (follow-up) Processors may override this with totally custom render logic.

4. Test coverage

could we use the thunder module for that, convert this to config and then test the configuration works, similar to what we had before? Seems to be a good validation that our configuration can meet common use-cases also.

6. ExtraFields support is not tested. is needed?

Good question. I don't think so, we can leave it out for now.

We had a productive discussion with @fago

As part of this issue, I need to do a few corrections:

• remove <code>label</code> from components in entity config export;
• comment and explain better isSingleFieldDisplay() method;
• clean-up code about checkbox in entity_view_display, probably will go in follow-up;

Next issues that must be done for alpha (to create meta issue and sub-issues):

• we need to auto-generate default CE display configuration on module install, in hook_update(), and on entity type creation;
• CustomElementFormatter" plugin:
  • introduce new plugin type in addition to formatters;
  • make plugin type not configurable;
  • have attribute to decide whether it uses field formatters or not; usesFieldFormatters
  • in UI display only select with available options;
  • default plugins in a module are:
    • auto
    • raw
    • formatted
  • show in new column next to "isSlot", dropdown of CE-formatters. Try to make field-formatters drop-down disabled based upon "usesFieldFormatters" value of the selected plugin
  • plugin adds data to custom-element object given the configruation (name)
  • it thus can use string|MarkupInterface for adding a slot, or a single CE or an array of CE objects
    • raw plugin with toArray() might need something like var_export() to map to String
    • if not a slot, passing an array is fine

More issues to create:

• clean-up on bundle removal or module uninstall;
• the follow-up to check/extend config schema;
• move checkbox in entity_view_display to follow-up issue;

I've made a few changes accordingly to what we discussed at the last meeting.
- better explanation for ::isSingleFieldDisplay() method
- tried to remove label from components in entity config export but apparently it's a little more tricky. After deleting it from config schema I've got failed tests. I added few workarounds to force clean it up but I always run into new bugs. The reason is that I still have EntityCeDisplay::displayContext = "view" Probably we can consider changing it but still not sure it is a good idea until we still using entity_view_mode entities and not completely new.
- made changes to DefaultContentEntityProcessor to use entity_ce_display configurations by default with the fallback to default view mode if the given doesn't exist. I kept the current logic still as fallback but after 📌 Auto-generate default CE display config per entity type Active it can be deleted.

@fago
Despite I merged the PR still assigning this to you for review. As already discussed, I'd recommend looking at 3.x -> 2.x diff as it will give you a better overview. And you can write all remarks here or in the follow-up issue.

tried to remove label from components in entity config export but apparently it's a little more tricky. After deleting it from config schema I've got failed tests. I added few workarounds to force clean it up but I always run into new bugs. The reason is that I still have EntityCeDisplay::displayContext = "view" Probably we can consider changing it but still not sure it is a good idea until we still using entity_view_mode entities and not completely new.

I see, so let's keep it. should we just use "label" instead of "name" or just keep it as null value?

I kept the current logic still as fallback but after #3362618: Auto-generate default CE display config per entity type it can be deleted.

Let's better just delete it right now since it's less confusing. It's ok if there are troubles with new netity-types until it is fixed, it's a bloccker anyway.

regarding code diff:

# @todo For is_slot logic we can consider collect fields in
# different region instead of checkbox.

As disucssed, this is not needed, so comment can be removed.

operation-link should be "Manage custom element display" just as the tab, not
'title' => t('Manage CE display'),

public function __construct(array $values, $entity_type) {
$this->pluginManager = \Drupal::service('plugin.manager.field.formatter');

parent::__construct($values, $entity_type);
}

This must use proper DI, constructor may not use \Drupal.

public function getPluginCollections() {
// @todo Check/correct.
$configurations = [];
foreach ($this->getComponents() as $field_name => $configuration) {

Not sure what should be wrong here, please clarify.

if ($field_definition instanceof BaseFieldDefinition || $field_definition instanceof FieldDefinition) {
// Override displayConfigurable to true for all field definitions.
$field_definition->setDisplayConfigurable($this->displayContext, TRUE);

Comment why you are doing it, not what you are doing. We want all the fields.

if (!empty($entity_ce_display)) {
$this->buildConfiguredCustomElement($entity, $element, $entity_ce_display);
}
// Otherwise render it from layout builder config if enabled.

as discussed, we should render via $entity_ce_display by default, else if not existing, we can render nothing / an empty CE.

default:
// Render the field using the selected view mode.
if ($field_component['is_slot']) {
$build = $field_items->view($display->getMode());
$custom_element->setSlotFromRenderArray($field_component['name'], $build);
}
else {
// @todo If it's just attribute then take raw value. It seems no a
// usecase but better have raw values for now.
$property = $field_items->getFieldDefinition()->getFieldStorageDefinition()->getMainPropertyName();
$values = array_column($field_items->getValue(), $property);
if (count($values) === 1) {
$values = reset($values);
}
$custom_element->setAttribute($field_component['name'], $values);
}
break;

It should always render using the field formatter. If not a slot, simply set the string value as attribute value - but the value should not be different just because the "is_slot" checkbox got checked!

> I see, so let's keep it. should we just use "label" instead of "name" or just keep it as null value?
I'm a little uncertain if it's a good idea to use label. I quickly tried to change but faced a problem with default values. Just reverted commit. I know it's not clean to keep it but it can be just an iceberg of a bigger problem. I'd first work on other tickets needed for alpha and return to this problem later in a separate issue if it will still be a case.

> Let's better just delete it right now since it's less confusing. It's ok if there are troubles with new netity-types until it is fixed, it's a blocker anyway.
Ok, I hope we understood each other correctly. I deleted all previous code from DefaultContentEntityProcessor and forced processor to generate custom element only from entity_ce_display config entity. Please take a look at PR and if something I will revert/change the commit.

> As disucssed, this is not needed, so comment can be removed.
done

> operation-link should be "Manage custom element display" just as the tab, not 'title' => t('Manage CE display'),
Corrected

> This must use proper DI, constructor may not use \Drupal.
Was copy-pasted. Moved into a separate method.

> Comment why you are doing it, not what you are doing. We want all the fields.
Totally make sense. Corrected.

> as discussed, we should render via $entity_ce_display by default, else if not existing, we can render nothing / an empty CE.
ok, it is deleted

> It should always render using the field formatter. If not a slot, simply set the string value as attribute value - but the value should not be different just because the "is_slot" checkbox got checked!
Alright, as I get renderable array in $build and not a string I need to render it first. I made the change to do it but I see it weird that attribute values can contain other HTML tags. Maybe I understood you wrong. Please see a change in PR.

thx. I added a few more remarks.

Naming is hard, yes :D But I think the name is fine.

Thanks @fago.
Remarks are addressed, please take a look.

Thank you. This seems almost ready now, but I founds two remaining remarks. Could you check that?

Sure, changes have been added.
Looks like test runner is still wrong here, did a check locally and all are still green.

PHPUnit 9.6.17 by Sebastian Bergmann and contributors.

Testing /var/www/html/web/modules/contrib/custom_elements
......                                                              6 / 6 (100%)

Time: 00:28.257, Memory: 4.00 MB

OK (6 tests, 32 assertions)

thx, all good now! merged.

Automatically closed - issue fixed for 2 weeks with no activity.