Problem/Motivation

As figured out after working and discussion in #3356918: Custom_elements_ui module: essential enhancements for alpha release the current implementation of custom_elements_ui has several limitations and needs major changes to make it cleaner and more maintainable in the future. After multiple discussions, we agreed that it's better to reduce the relation on entity_view_display and stop modifying it for our needs. Instead will be better to implement separate entity_ce_display entity type in a similar way as entity_view_display and entity_form_display are implemented in core. It should help us to get a solution that will be implemented following the pattern that Drupal suggests and as a result, it should be more flexible and easier to maintain in the future.

Proposed resolution

We propose re-implementing custom_elements_ui with a new concept:

  • Implement new config entity type entity_ce_display
    • Resulting configuration can look like this core.entity_ce_display.ENTTIY.BUNDLE.yml
    • Have tabs, menu links etc similar to existing "Manage display" and "Manage form display"
  • New plugin type for field output configuration (like widget and formatter)
    • To consider deriving and re-using existing formatters
    • For MVP we can consider using formatter plugin type
  • The toggle in the "Manage display" will control whether we take over default entity rendering as custom element accordingly to the configuration in the appropriate entity_ce_display entity
  • Don't think that we need to have separate entity_ce_mode (like entity_view_mode, entity_form_mode ) because maybe it will be more reasonable to re-use existing entity_view_mode available per bundle

Remaining tasks

During implementation will be better to split work into smaller parts and create follow-up issues.

User interface changes

todo

API changes

it will be implemented in 3.x branch so we can afford not BC-compatible changes but better avoid.

Data model changes

custom_element configuration will be moved out from third_party_settings of entity_view_display to new entity_ce_display config entities

Issue fork custom_elements-3359601

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mostepaniukvm created an issue. See original summary.

mostepaniukvm opened merge request !27

mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Status: Active » Needs review

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

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?

mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Assigned: mostepaniukvm » fago
fago’s picture

fago
Primary language German
Location Vienna
commented
Assigned: fago » mostepaniukvm
Status: Needs review » Needs work

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.

mostepaniukvm’s picture

mostepaniukvm
he/him
commented

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;
mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Parent issue: » #3362617: [META] custom_elements_ui: 3.x alpha release

  • mostepaniukvm committed 7045b56e on 3.x 
    Issue #3359601 by mostepaniukvm, fago: custom_elements_ui: re-implement...
mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Assigned: mostepaniukvm » fago
Status: Needs work » Needs review

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 #3362618: Auto-generate default CE display config per entity type 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.

fago’s picture

fago
Primary language German
Location Vienna
commented

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.

fago’s picture

fago
Primary language German
Location Vienna
commented

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!

mostepaniukvm opened merge request !28

mostepaniukvm’s picture

mostepaniukvm
he/him
commented

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

fago’s picture

fago
Primary language German
Location Vienna
commented
Assigned: fago » Unassigned
Status: Needs review » Needs work

junkuncz made their first commit to this issue’s fork.

junkuncz’s picture

junkuncz
Primary language Hungarian
Location Budapest
commented
Status: Needs work » Needs review
fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

thx. I added a few more remarks.

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

junkuncz’s picture

junkuncz
Primary language Hungarian
Location Budapest
commented
Status: Needs work » Needs review

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

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

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

junkuncz’s picture

junkuncz
Primary language Hungarian
Location Budapest
commented
Status: Needs work » Needs review

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)

  • fago committed af9c8f75 on 3.x authored by mostepaniukvm 
    Issue #3359601 by mostepaniukvm, junkuncz: Improve custom elements...
fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Fixed

thx, all good now! merged.

Status: Fixed » Closed (fixed)

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