Change configuration structure for CE display 'components'

seems to be the main blocker atm

If quick and easy (I really don't know yet): add backward compatibility loading for already present 'alpha1 style' configuration. (That's being used in several places in an internal project.)

No, let's not spend efforts on BC with an alpha release. It was marked to be not ready for prime-time. Let's break it once now and move on.

Re #5: the thing is, it was actually quite easy to implement, especially compared to some other changes -- and makes my actions on converting internal projects a lot easier (and verifiable). No, there's no official compatibility, but I have implemented "backward compatibility loading".

The biggest time sink on this issue was trying to get "renaming keys" right -- which I unfortunately still haven't done (but I can use my work until now, in the followup). Now I'm strapped for time, and will already need to merge this.

I've tested this fairly extensively for about a week in Burgas + in the past days, so I'm confident there are no weird bugs.

Changing status to what it officially should be, but I'll merge it when I'm ready to cut a release.

Great to see this land!

I can totally see that renaming makes the form workflow complicated though. Let's make sure we have a good, consistent form reload/rebuilding workflow so it works reliable across reloads/ajaxRebuilds.

// Remove EntityForm::afterBuild(), which calls copyFormValuesToEntity();
// we cannot update $this->entity before form values are properly validated.
// Any other validation functions must take this into account.
if (!empty($form['#after_build'])) {
$form['#after_build'] = array_diff($form['#after_build'], ['::afterBuild']);
}

hm. This sounds like the standard entity building and validation workflow, but that's 100% not the case. A lot of time has been invested into that, such that entity-building works and is independent from validation. It should give you a copy of the entity, and (IF I REMEMBER RIGHT) not update $this->entity, for that the submit-entity phase is done. So this seems weird. Can you spot more light on it why it's needed?

You need to watch out that $form object and this $this->entity does not end-up in $form though, this messes things up since the $form array gets cached. be sure to use "::method" notation for callbacks.

* @param string $validate_component

* (Optional) Only make sure the basic properties of the specified component
* are updated, in order to be able to validate the full component's config.
* Do not copy the component's full (unvalidated) configuration yet; leave
* other component's values in an undefined state.
*/
protected function doCopyFormValuesToEntity(EntityInterface $entity, array $form, FormStateInterface $form_state, string $validate_component = '') {

This is not a good idea imo. The typical workflow is to build a new $entity based upon form values, then run validation on it. That's problematic for us since this means running ::submit() phase on (CE-)formatters before validate and core-formatters are used to not having that. So to me it seems we should simply not have it updated. What's the reason for the partial entity update?

taking a look on simplifying this:

• let'S add a dedicated "name" property to the component-options as we had it before. the component options key should have no meaning and be up to the UI to decide. So for now the UI can keep using the field-name, while our config format is more powerful.
• Have the UI working without re-keying component-options. That keeps things simpler. Thus drop https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a...
• let'S revert possible troublesome UI changes that affect the overal entity form worklow like https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... + https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a...
• fix/revert getRenderer to always return component options and remove the optional parameter - e.g. simplfy https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a...
• Related, simplify https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... to assume/require the new field_name key. Let'S remove support for old config formats. Things are complicated enough with one format. Let's assume and require proper config format and write simple code for that.
• keep code tidy. We don't want to maintain code for supporting non-released, alpha state configs. remove https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... and simply access $component_options['name']. If it's missing a warning will tell something is wrong, that's good enough.
• Drop https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... and add validation of no duplicates in dedicated ticket. Can be after 1.0, let'S just create a ticket.

in the end, I think we should try to revert most UI changes + focus on doing changes on the config code + the methods in the CE-entity. Given that there should be only a need to change the UI slightly here, for adding the 'field_name' key.

Side-notes:
* Non-specific todos like https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... I think we should keep in issues. Todos in code should be only added when concrete and be ususally short, refer to issues for more context.
* also I'd remove unsure things, which might just wrongly stay forever: https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a...

fix/revert getRenderer to always return component options and remove the optional parameter - e.g. simplfy https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a...

Related, simplify https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... to assume/require the new field_name key. Let'S remove support for old config formats. Things are complicated enough with one format. Let's assume and require proper config format and write simple code for that.

keep code tidy. We don't want to maintain code for supporting non-released, alpha state configs. remove https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... and simply access $component_options['name']. If it's missing a warning will tell something is wrong, that's good enough.

Of course these are going to be fixed. This is very much temporary code, and it's marked as such. It is simply an effect of the fact that BOTH the config structure AND the UI should be changed, as we planned. Doing both things at the same time in one giant PR would not have helped understanding.

Since the config change was needed first, that is what was done first. Now the UI code in the OBSOLETE (as we know) form is partly ugly. I know this and fully acknowledge this.

The way for these things is forward, not back. It makes much more sense to fix these and remove this code by doing the intended changes in 📌 Simplify entity-ce-display form and AJAX logic Active . That is what I prepared this code for.

--

As for your comments about the renaming: I acknowledge and understand.

But please give me 1-2 days to remove the above code by going forward, not back, This is why I told you I would like to prove out to you that this may be an OK route. (IMHO the tradeoff between the "so-called unstable" renaming code in the form, and the unnecessary complication in the configuration structure that is fine as a structure, and we have already rolled out in our internal project, will be better to see and discuss when the above / #3446485 is done.

If I may try to summarize your proposal as I see it:

You want to revert the UI changes and then do a config change without touching the UI at all. This (AFAICT) implies having a different config structure, that is not keyed by the "key / name" property but instead has both the field name + the key/name as properties. This format IMHO

• is somewhat 'inferior', when looking just at the config structure, because it contains unused info (the keys) and does not 'auto-de-duplicate' keys/names
• causes extra work for our internal projects which are already on the new format 'keyed by key/name'.

It may well be, that we still end up there. I can't form a definitive opinion on that right now. But reverting the UI change makes it required to end up there.

If we instead finish the UI changes intended per 📌 Simplify entity-ce-display form and AJAX logic Active (which will get rid of the ugly code), then we still have options to discuss. I believe that it may be possible to have some "hardcoded key, so that the renaming code in the form does not need to override the afterBuild() / copyValuesToEntity() code".... inside the form, only for the lifetime of the form, so that it's not necessary to actually change the new config structure.

But I can't think about this from the top of my head, in between. I need to sit down and write this out. Which I'll likely have from Friday afternoon onwards, because I've almost succeeded in "clearing my plate" and many people are now on vacation / there are no new projects going on that require my attention.

This is "needs work" (by me), even if the work to be done is just opening followup issues.

This format IMHO

is somewhat 'inferior', when looking just at the config structure, because it contains unused info (the keys) and does not 'auto-de-duplicate' keys/names

Imo that perfect usage of bits buys us nothing. But the redundancy removes complexity from the UI, so we can write simpler code. Less challenges, less problems.

This format IMHO

causes extra work for our internal projects which are already on the new format 'keyed by key/name'.

True. But the update process is not hard to do and there is no time-constraint with the update. The update has to be written once and is no further maintenance burden.

If we instead finish the UI changes intended per #3446485: Simplify entity-ce-display form and AJAX logic (which will get rid of the ugly code), then we still have options to discuss. I believe that it may be possible to have some "hardcoded key, so that the renaming code in the form does not need to override the afterBuild() / copyValuesToEntity() code".... inside the form, only for the lifetime of the form, and then eventually save the config in the current-new structure.

Imo #3446485 seems like a follow-up improvement necessary anyway, regardless on how we finish up here. Anyway, it will be just easier when we go with fixed config keys.

I think this would be the next steps we should take here to focus on shipping 1.0 asap:

• let'S add a dedicated "name" property to the component-options as we had it before. the component options key should have no meaning and be up to the UI to decide. So for now the UI can keep using the field-name, while our config format is more powerful.
  Have the UI working without re-keying component-options. That keeps things simpler. Thus drop https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... Related, simplify https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... to assume/require the new field_name key.
• Let'S remove support for old config formats. Things are complicated enough with one format. Let's assume and require proper config format and write simple code for that.
• let'S revert possible troublesome UI changes that affect the overal entity form worklow like https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... + https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a...
• Drop https://git.drupalcode.org/project/custom_elements/-/commit/aacafade016a... and add validation of no duplicates in dedicated ticket. Can be after 1.0, let'S just create a ticket.

Thanks! Merge the MR and reviewed the complete changes here after this merge again, looks good now!

I think we just need take care of creating all follow-ups and then we can call this one closed. Finally! :-)

Warning: this user interface is subject to change and contains some bugs. As a result:

If you want to rename a 'Key': immediately afterwards, press TAB and then press Save for each individual renamed key.

Tested this, it mostly works good, except when you press save immediately without the AJAX routien to run. I guess we could somehow improve the submit button code to detect whether it is run already and have it run there when needed. Anyway, we need a follow-up for solving this part and to get rid of the message. --> This I think should be release-blocking, since the message is ugly for screen-recording/screenshots.

> // @todo Safely remove the case for !$component_name in #3446485 / second
// call parameter, when all rows represent components.
$plugin = $component_name ? $this->entity->getRenderer($component_name)
: $this->entity->getRenderer($field_name, TRUE);

I think we need a follow-up for this todo introduced here. Seems not release-blocking.
Elsewhere it has * @todo remove $get_actual_field_name in #3446485.

The issue about simplify-ing UI in general, not specific about this topic. Let's better add a specific issue to fix getRenderer() to obey the interface / clean-it up and reference this since this is not necessary the same issue (if I'm not mistaken).

> // @todo Change this structure in #3446485: $form keys will be component
// names, not field names. Rename '#fields' (here and elsewhere)?

This todo in code should also get a ticket. We should update the code with ticket number references quickly.

> // @todo When implementing static properties per #3446287, 'field_name'
// is not necessarily present (or will be reused for the static value,
// so we don't have differing config schema?). Also, its method
// signatures will be different. Distinguish based on $formatter's
// interface.

I don't think we should todos for future plans in code. Let's keep code in slim and better remove it. We can drop notes about future plans in the ticket where we make this plans, but until we have static properties implemented I'd not add any related todos in code.

Same with

* @todo the return type will likely be 'widened' with #3446287 (static
* props); implement this before beta.

minor:
it's the default fallback when "automatic processing * is enabled in a Custom Elements Display configuration.

Added https://git.drupalcode.org/project/custom_elements/-/merge_requests/78 quickly to address the last 3 points.

Tested this, it mostly works good, except when you press save immediately without the AJAX routien to run.

Exactly. I took a little look, but did not find a way to remove the AJAX behavior on the name field immediately, because it's too tied in with the Core display form.

And since we are going to untie that relationship anyway, in 📌 Simplify entity-ce-display form and AJAX logic Active ... I didn't waste time on this and just put in the warning (because more than one 'tester' was bitten by this behavior.)

I think we need a follow-up for this todo introduced here

(this and several other comments above.)

I don't understand. The followup is #3446485, which is mentioned in the todos. I don't think we need separate followups.

• This (config change) was a big ticket, especially because its structure is antithetical to the current form (because the parent-form code that is hardwired to "a row is always a field", which this code is explicitly moving away from)
• #3446485 (redoing the form and untying the parent form's "a row is a field" logic) is going to be a big ticket. I suspect this is hard to split out in several MRs. Changing the form is just... big. (But I'll keep in mind, whether I can stop at a certain point, after I untied the parent-form-class. If I can, I will create a sub-MR or subtask for #3446485.)
• So these two tasks are linked together. But it would have been foolish to do both at the same time in a single PR.
• Therefore we needed a temporary situation where the new config format is clunkily tied into the old incompatible form, with temporary 'glue' code, in several places.
• For this I added loads of @todos as reference / justification / reminder to undo all of these in #3446485. All that code is going to be re-simplified or deleted or (in the case of getRenderer()) un-done when it's not necessary anymore.

Yes, #3446485 will be quite big, and I am going from the assumption it cannot be split up (though I'll re-check that assumption when coding). The 'good news' is: much of it is going to consist of re-simplifying things.

Re. ✨ Allow configuration of static props Active comments

I don't think we should todos for future plans in code. Let's keep code in slim and better remove it.

I will keep that in mind. When I was looking at implementing the structure change (and trying to do all I could to prevent a future compatibility break / internal-project-update-path), I felt that my changes were so invasive that I needed to comment this.

Indeed it would have been better to clean up my code after my ideas were 'settled' and tested, and move these to comments on the MR.

I'll merge your MR (after removing double "are" in "Since v3 processors on entity-level are used only when ..."

ok, when the one issue is the right follow-up, we should be all good. Used web-editor to removed the double "are" and merged the previous MR as suggested. Thus, all done here!

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

For completeness: I did work on followup tasks in 📌 Simplify entity-ce-display form and AJAX logic Active , but the cleanup didn't happen there.

I totally admit to not having a detailed step-by-step plan until now, because there were too many unknowns for me in the current code. I now did a better writeup of what my plan was, in 📌 Ability to add a field multiple times Active -- which is now slated to be the "cleanup / followup issue".