Problem/Motivation
Current Block library for entities will list a lot of not needed entity_field and some entity_reference. This is a "normal" UI Patterns behavior to list sources without condition.
Problem is a lot of them are irrelevant inflating the list of blocks with noise, and other can provide unwanted behavior and even fatal or memory crash from recursion.
Current way is to use exclude provider and exclude_id, which works great for block source but not entity_field, and using it for entity_field generate a heavy and hard to maintain list like on the Bootstrap recipe.
entity_field
field:node:article:nid >> Probably not relevant
field:node:article:uuid >> Probably not relevant
field:node:article:vid >> Probably not relevant
field:node:article:langcode >> Probably not relevant
field:node:article:type >> Probably not relevant
field:node:article:revision_timestamp >> Not relevant
field:node:article:revision_uid >> Not relevant
field:node:article:revision_log >> Not relevant
field:node:article:status >> Not relevant
field:node:article:uid >> Probably not relevant
field:node:article:promote >> Not relevant
field:node:article:sticky >> Not relevant
field:node:article:default_langcode >> Not relevant
field:node:article:revision_default >> Not relevant
field:node:article:revision_translation_affected >> Not relevant
field:node:article:path >> Probably not relevant
field:node:article:menu_link >> Probably not relevant
field:node:article:field_display_full >> On entity override will CRASH
field:node:article:field_display_teaser >> On entity override will CRASH
field:comment:comment:thread >> Not relevant
field:comment:comment:pid >> Not relevant
field:comment:comment:cid >> Not relevant
field:comment:comment:field_name >> Not relevant
field:comment:comment:langcode >> Not relevant
field:comment:comment:default_langcode >> Not relevant
field:comment:comment:entity_id >> Not relevant
field:comment:comment:langcode >> Not relevant
field:comment:comment:status >> Not relevant
field:comment:comment:thread >> Not relevant
field:comment:comment:uid >> Not relevant
field:comment:comment:uuid >> Not relevant
field:user:user:langcode >> Not relevant
field:user:user:default_langcode >> Not relevant
field:user:user:init >> Not relevant
field:user:user:preferred_langcode >> Not relevant
field:user:user:preferred_admin_langcode >> Not relevant
field:user:user:uid >> Not relevant
field:user:user:status >> Not relevant
field:user:user:uuid >> Not relevant
entity_reference
entity_reference:node:article:menu_link:menu_link_content:menu_link_content >> Not relevant
entity_reference:node:article:revision_uid:user:user >> Not relevant
We should find a way to exclude easily these blocks without a manual config.
Main source loader is BlockLibraryPanel::getSources with usage of Drupal\ui_patterns\SourcePluginManager and rely on SourceWithChoicesInterface.
Proposed resolution
As UI Patterns is meant to be generic this probably must be resolved within Display Builder.
Some propistion:
- Extend filter sources in BlockLibrary with a more efficient filtering
- Add our own SourcePluginManager which implement our logic of filtering and will provide 'free' cache.
- ..?
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork display_builder-3620150
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:
Comments
Comment #2
mogtofu33 commentedComment #3
pdureau commentedEach base field already has an attribute telling if it is suitable for display building or not: FieldDefinition::isDisplayConfigurable() (more precisely:
BaseFieldDefinition::isDisplayConfigurable()).Checking each field against this method will be efficient to remove the fields we don't want without maintaining our own "black" list.
However, we will lose a few fields we want, like
node:title, so we may need a "white" list instead.Comment #4
mogtofu33 commentedGuess it have to be from UI Patterns, I see some other places like
EntityFieldSourceDeriverBasewhereFieldDefinitionis yield. Can you create an UI Patterns issue?Comment #5
pdureau commentedWe can propose to annotate the source plugin definitions generated by
EntityFieldSourceDeriverBasewith aui_hiddenboolean attribute: true only for fields::isDisplayConfigurable() === TRUEAnd then do the filter in BlockLibraryPanel.
(Layout Builder is doing such annotation with
_block_ui_hiddenbut is not using it in UI)Or, we can use the "standard"
no_uiif it is not hiding them also for UI Patterns source selector for slots.Are you OK with such proposal?
Comment #6
mogtofu33 commentedGreat, I am ok, I like the
no_uifor consistency with components.Comment #7
mogtofu33 commentedComment #8
mogtofu33 commentedDo we have an issue on UI Patterns or not yet?
Comment #9
mogtofu33 commentedComment #10
pdureau commentedTicket created #3624220: Add ui_patterns_source_choices hook
Comment #11
mogtofu33 commentedComment #13
mogtofu33 commentedReview must be done with #3624220: Add ui_patterns_source_choices hook
Comment #14
pdureau commentedTested with the current MR & https://git.drupalcode.org/project/ui_patterns/-/merge_requests/573
So, if my understanding is correct, we are keeping both:
FieldDefinition::isDisplayConfigurable('view') === TRUEcreated&changedSo, I need to check:
For example, would we want to keep the field marked with "⚠️" or "❓" ?
It is particularly sensible for Users entities. It would be hard to build a display with just changed, created & picture fields.
Comment #15
pdureau commentedA solution may be to finally not rely on
FieldStorageDefinitionInterface->isDisplayConfigurable('view)for base fields but to filter them out according to where they come from, like:RevisionLogEntityTrait::ownerBaseFieldDefinitions(): revision_created, revision_user, revision_log_messageEntityPublishedTrait::publishedBaseFieldDefinitions(): publishedContentEntityBase::baseFieldDefinitions(): id, uuid, revision (Revision ID), langcode (Language)EntityFieldManager::buildBaseFieldDefinitions(): revision_default, default_langcode (Default translation), revision_translation_affectedpath(URL alias), but i don't know where its is addedIt is relevant in my opinion because we know those fields already, they are strictly defined in code (careful, they are sometimes aliased, see
ContentEntityTypeInterface::getRevisionMetadataKey()).So, we will get those results:
@mogtofu: is it good enough? It is already cleaner :)
I can init a MR if you are OK with the proposal.
Comment #16
pdureau commentedGiving a quick try.
Comment #18
pdureau commentedHello Jean
What do you think about this proposal without
::isDisplayConfigurable()?I am the one who proposed to use in #3 but I know believe it was a bad idea.
I didn't' inject new service but I reused the one already adding in the first MR.
What do you think?
Comment #19
mogtofu33 commentedThis is a good proposition and cleaner, but the same fields also show in the contextual form, through the UI Patterns field select.
Proposal in #3624220: Add ui_patterns_source_choices hook: a
no_uiflag on derivable contexts, set by Display Builder inhook_derivable_context_info_alter().exclude_idstays for the library fine tuning.I will add a comment on the ui_pattern issue.
For the proposition here:
Tested the rules of !429 on every viewable content entity type (node, user, term, media, comment, block_content, file, menu_link_content...). Entity keys and revision metadata keys work well: they cover all entity types without naming fields. Some points:
statusis not something to display. You missed it in your table.'weight' => 'boolean'never matches, term and menu link weight areinteger. So Weight is still listed.langcodeis hidden by its key, butpreferred_langcodeandpreferred_admin_langcodeon user stay, and other entities have their own. Hiding thelanguagefield type covers all of them.field_display_fullandfield_display_teaserare still listed. They areui_patterns_sourcefields, the recursion crash of this issue. Hiding that field type covers them whatever their name.$definitions[$field]->getType()assumes the base field exists. UsinggetFieldStorageDefinitions()with a null check avoids it, and also sees config fields.testHideNotDisplayConfigurableFields.Not related here but a collateral that could be fixed in the same time for security:
HIDE_CHOICE_IDonly matchesfield:user:user:pass, the bundle-lessfield:user::pass(Views context) leaks. Hiding thepasswordfield type covers both, and any custom entity.Then the entity specific ones: Promoted to front page and Sticky on node, Initial email on user, thread / hostname / entity_type / field_name on comment. Email is also personal data, so I would hide it by default. Leaving them to
exclude_idbrings back the original problem: one line per bundle, see the Bootstrap profile. I would rather keep a short list per core entity type in code, and leaveexclude_idfor site specific fine tuning.Proposal for the generic part:
id,uuid,revision,langcode,published,default_langcode,revision_translation_affectedrevision_default)password,path,language,ui_patterns_sourcenode: promote, sticky.user: init, mail.comment: thread, hostname, entity_type, field_name. Plusweightby name.Comment #20
mogtofu33 commentedFollow-up of the discussion in #3624220: Add ui_patterns_source_choices hook: proposed there, an alter hook on the source choices, e.g.
hook_ui_patterns_source_choices_alter(). UI Patterns stays generic, consumers decide.If accepted, !429 changes shape:
BlockLibrarySourcesto a hook implementation inUiPatternsHooks.BlockLibrarySourcesgoes back toexclude_idand provider fine tuning only.entity_fieldsource instead of mocks.The fixes listed above (weight, language, password,
ui_patterns_source, storage definitions) still apply, wherever the rules live.Comment #21
pdureau commentedI kept it in purpose, because i was picturing displays rendering the published status as an icon or a label.
👌 It was done for reorderable entity types like Taxonomy Term, i will add a comment.
Done in purpose too.
preferred_langcodeandpreferred_admin_langcodeare "business" fields, telling stuff about the user, not internal/technical fields to make the translation API work. I would advice to keep it, administrators building User entity displays are supposed to be knowledgeable site builders.👌 The logic address only base fields now, but i can add a check by type, which will be useful for this use case.
👌 OK, will do
👌 Ok, will rename.
👌 Ok, this is the opportunity to merge the 2 mechanisms.
I would like to avoid checks by entity types, to not open the Pandora box. I will check
commententity type anyway.Is it the scenario are we trying to avoid?
Comment #22
mogtofu33 commentedI do not have precise scenario in mind, I see more the frontend building part and not really admin yet, because of postponed #3575891: Allow administration page display.
I was defensive until we are clear on admin pages, but I can revise that based on Drupal core and Views which allow to do it and the responsibility is on the builder.
As we are not opening something not possible before, then it's ok for me to not be so defensive. At the end only password is critical, other are more UX disturbance, and the crashing ones are handled so it's ok.
Comment #23
pdureau commentedI didn't see your comment and I have hidden email in my update: https://git.drupalcode.org/project/display_builder/-/merge_requests/429/...
All covered except
publishedbecause I can picture uses case when we want to add a renderable (a chip, a macaron..) to show the content publishing status. But I may be wrong and it is easy to add.Already the case, I guess.
Done.
Mail hidden. Others kept.
Covered.
Comment #24
mogtofu33 commentedPushed some fixes on top of !429, tested on every viewable content entity type.
Config fields were not hidden without a bundle
checkField()usedgetFieldDefinitions($type, $bundle). On a derivative without a bundle, likefield:node::field_display_fullin a Views context, the bundle is an empty string and that call returns the base fields only, so no configurable field was ever found. The override fields were still offered there, which is the recursion crash of this issue.Now the type check uses the base field definitions merged with the storage definitions, which cover every bundle.
A trap while doing it: storage definitions leave out the computed base fields, so
pathwas no longer found and "URL alias" came back in the library. The merge with the base fields is what keeps it hidden, and there is a comment about it.weight
Changed to
'weight' => 'integer'. Checked on a full site: the only core content entities with aweightbase field aretaxonomy_termandmenu_link_content, both integer, so the entry matched nothing. The type guard still prevents the rule from firing on another field of the same name.Possible warning
$definitions[$field]?->getType()still emits "Undefined array key" when the key is absent: only the call is guarded, not the lookup. Wrapped both lookups with?? NULL.Cleanup
$bundlebecame unused, so it is gone fromcheckField(). InisHiddenField()the list destructuring with an empty slot was replaced by named parts, which also removes thesubstr_count()guard: with fewer than four segments the field name is empty, which the same condition catches.Tests
testHiddenFieldsnow covers each case: the integer weight, the computed path, the password, and aui_patterns_sourcefield with and without a bundle. A keptfield_weightguards against the type rule firing on a name with no base field. The mock builder gained the storage definitions.Result on a Bootstrap recipe, no override field, no URL alias, no weight, no pass, in both contexts: node:article 15 choices with a bundle and 17 without, user 13 and 13, taxonomy_term 5 and 5, media 10 and 14, comment 17 and 17.
Comment #25
pdureau commentedOopsy. Thanks.
Comment #26
mogtofu33 commentedComment #29
pdureau commentedI can't believe I removed
publishedbase field in one of my commits 🤦