Problem/Motivation

Related to #3620150: Block library content from Display Builder.

Proposed resolution

Just first proposal, to be challenged if someone come with a better one.

Annotate the source plugin definitions generated by EntityFieldSourceDeriverBase with a boolean attribute, true by default, and set:

  • with the return value FieldDefinition::isDisplayConfigurable()
  • except for some selected fields, which always true: "Label/Name/Title", "Authored on", "Authored by", "Changed by"... which ones exactly?

We can use:

  • _block_ui_hidden, already added by Layout Builder but not used in its UI:: web/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
  • the "standard" no_ui if it is not also hiding them also for UI Patterns source selector for slots. UI Patterns is a power-user tool, we may want to keep the full list.
  • Something else?

Issue fork ui_patterns-3624220

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

pdureau created an issue. See original summary.

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented

Hello,
i had some similar ideas when i first proposed the plugin deriver, when i kept a lot of metadata about the entity fields (base vs configurable...etc).
we don't have any module settings right now,
but what about introducing a way to control/limit the amount of sources generated/shown?

we could also consider such tuning could be in ui_patterns_ui rather than in the main module.

pdureau’s picture

pdureau commented
Issue summary: View changes

I am updating the ticket description which was a bit confusing.

i had some similar ideas when i first proposed the plugin deriver, when i kept a lot of metadata about the entity fields (base vs configurable...etc).

The metadata we need or the ones related to display. Maybe they were among the ones you proposed.

we don't have any module settings right now,

In general, it is betetr to avoid module-wide plain settings files in favor of:

  • having the best default behaviour
  • leveraging existing API for dynamic states
  • providing a config entity

we could also consider such tuning could be in ui_patterns_ui rather than in the main module.

It would be better if it can be done in main module, because not everybody is using ui_patterns_ui (which is mainly for agencies to configure end users experience).

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

mogtofu33 opened merge request !573

mogtofu33’s picture

mogtofu33 commented
Status: Active » Needs review

Proposal pushed on 3624220-annotate-fields.

What the noisy entries in #3620150: Block library content really are: field:* and entity_reference:* DerivableContext plugins, not Source plugins. That rules out both keys discussed:

  • no_ui only exists on #[Source], and ComponentFormBase::sourcesToOptions() already uses it to hide sources from the slot source selector. Not what we want for power users.
  • _block_ui_hidden is a block definition key, never read by core.

SourceMetadataKey is made for this: derivers write, consumers read. The field bag already holds type and cardinality.

Changes:

  • New SourceMetadataKey::DisplayConfigurable, the raw isDisplayConfigurable('view') value. Bundle-less derivatives: base field value, TRUE for configurable fields.
  • DerivableContextSourceBase::getChoices() exposes metadata on each choice. BlockSource and ComponentSource return an empty array, for a uniform shape.
  • Kernel tests in DerivedPluginIdsTest.

No setting, no UI change in UI Patterns. Filtering stays in consumers.

About the whitelist: on a standard site the flag is FALSE for every node, user and comment base field, title included. So consumers still need an allowlist. Display Builder will keep its own for now: #3620150: Block library content that are not revision metadata keys.

If we later want that rule shared, it can be a second key, e.g. displayable, added beside this one. display_configurable keeps mirroring core, so the follow-up would be additive, with no BC break.

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented

Hello,
i think we need to wait before implementing this issue, because in fact the original request formulation may not be the right solution.
Indeed, re-introducing metadata, but only display_configurable, is not the right answer imho.
have a look at what we deleted in #3591167: Tidy metadata plugin attributes. everything was in place for consumers to decide to filter exactly what they want : what the consumers consider "noisy" VS what is considered useful. i was thinking for a long time, advanced UI would need that data.

we had :
- configurable
- editorial
- parent_base
- base

yes because the fields you mentionned (title for nodes, name for terms..etc and published...etc) could be derived automatically (without hardcoded allowList) from smart metadata.

my guess would be to re-introduce not only display_configurable but a list which would allow a classification which makes probably more sense to the final user (sitebuilder + editor).

pdureau’s picture

pdureau commented
Assigned: Unassigned » just_like_good_vibes

[Edit] I have deleted my previous comment, because I am now understanding than /src/SourceMetadataKey.php is the union of 2 different levels of metadata.

Directly in the metadata attribute of Source plugins:

  • Field = 'field'
  • FieldName = 'field_name'
  • Property = 'property'
  • Provider = 'provider': FieldStorageDefinitionInterface->getProvider(): string

Inside SourceMetadataKey::Field:

  • Type = 'type': FieldStorageDefinitionInterface::getType(): string
  • Cardinality = 'cardinality': FieldStorageDefinitionInterface::getCardinality(): int

Where Jean is proposing:

  • DisplayConfigurable = 'display_configurable': FieldStorageDefinitionInterface->isDisplayConfigurable('view): bool

I was very confused, because it was supposed to be 2 different sets during our work on #3591167: Tidy metadata plugin attributes.

So, let's introduce a new field metadata, but are the rules to define display_configurable enough? In #3620150: Block library content, we have noticed than this filter remove important fields, like email and name from User entity,

Can we find other criteria?

  • FieldDefinition::isInternal() to remove "Default revision"
  • FieldDefinition::isComputed() to remove "Path"
  • ..

I don't believe the ones we have recently removed would help here:

  • configurable: not a base field
  • editorial: a base field defined by EditorialContentEntityBase
  • parent_base: a base field defined by a parent class
  • base: a base field defined by the entity class

Do we have other proposals?

Is it also the opportunity to add comment explaining each item of SourceMetadataKey.

pdureau’s picture

pdureau commented
Assigned: just_like_good_vibes » pdureau
Status: Needs review » Needs work

I may propose something directly in #3620150: Block library content

pdureau opened merge request !575

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Needs work » Needs review

I hope the changes proposed in #3620150: Block library content will make this ticket obsolete.

In my opinion, the logic based on ::isDisplayConfigurable() was wrong because this method was not implemented ina reliable way among the entity type provided by Drupal Core (see the example of User entity).

Also, I would prefer us to avoid extending SourceMetadataKey and using it outside of UI Patterns. It must be considered as an internal mechanism.

Let's see how the proposal will be received.

However, this work is the opportunity to bring some clarity. I have open a MR with some comments added to SourceMetadataKey : https://git.drupalcode.org/project/ui_patterns/-/merge_requests/575

Mikael, can you add the missing comments to this MR? Is it relevant to also add a hint telling the enum is internal?

mogtofu33’s picture

mogtofu33 commented

Agreed, closing my MR: no new SourceMetadataKey, the rules go to #3620150: Block library content.

But Display Builder also needs them in its contextual form, which uses the UI Patterns field select (DerivableContextSourceBase::getChoices()). Without an extension point, the only way is to swap the entity_field and entity_reference source classes, coupling Display Builder to UI Patterns internals.

Proposal, keeping UI Patterns generic: no filtering in UI Patterns, just an alter on the choices, e.g. hook_ui_patterns_source_choices_alter(array &$choices, SourceInterface $source). Consumers decide, with the source contexts at hand (Display Builder hides editorial noise in entity displays, keeps everything in Views). Default behavior unchanged.

OK to retitle this issue for that?

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

thanks, i will have a look soon

pdureau’s picture

pdureau commented
Title: Annotate the fields without configurable display » Document SourceMetadataKey & add a source choices hook
Status: Needs review » Needs work

I will try something in my MR:

  • Adding missing documentation
  • Adding a hook

Just to be sure, this hook will be used to filter the fields available for slots in ContextualPanel, not to filter choices for props?

If it is OK, I will update the description, send to review and create a follow-up (which will address some internal UI Patterns stuff, no impact on Display Builder and others ecosystem modules)

pdureau’s picture

pdureau commented

I can't test the hook because I have this every time I want to add a slot source from the Component Form:

TypeError: Drupal\ui_patterns\Element\ComponentFormBase::announceValues(): Argument #1 ($row) must be of type array, null given, called in /var/www/html/web/modules/custom/ui_patterns/src/Element/ComponentSlotForm.php on line 649 in Drupal\ui_patterns\Element\ComponentFormBase::announceValues() (line 611 of /var/www/html/web/modules/custom/ui_patterns/src/Element/ComponentFormBase.php)

Only on Display Builder Contextual Panel, it is OK when using UI Patterns only.

Edit: ticket created: #3626043: Config panel loses source settings on AJAX actions and saves raw values

pdureau’s picture

pdureau commented
Title: Document SourceMetadataKey & add a source choices hook » Add ui_patterns_source_choices hook
Assigned: pdureau » just_like_good_vibes
Status: Needs work » Needs review

Pipeline failing because of #3625969: Twig's 3.30 TypeError: Twig\Runtime\EscaperRuntime::escape(): Argument #4 ($autoescape) must be of type bool, null given, I guess