Problem/Motivation

Before working on #3540247: Map source contexts with enums which is kind of risky, we need to make sure the mechanisms are understood and documented. We have already done #3590492: Tidy the source plugins tags, it is time to do the metadata attribute for Source and DerivableContexts plugins.

This plugin attribute is an under documented, loosely types associative array and feels a bit "magical".

Proposed resolution

  1. Convert metadata array to "Data Transfer Objects", PHP classes with strong typing and disctinction between optional and mandtory properties.
  2. Remove the unused properties
  3. Document as much as possible the remaining properties.

Issue fork ui_patterns-3591167

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.

pdureau’s picture

pdureau commented
Title: Tidy plugin metadata » Tidy metadata plugin attributes

pdureau opened merge request !504

pdureau’s picture

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

Works have started with #3590492: Tidy the source plugins tags as a starting point.

Mikael, can you help us document the properties of:

When finished, let's leave it in RTBC without merging, and discuss.

Maybe the change will break ecosystem (like ui_icons_patterns) and we will keep it for 2.1.0. Maybe it will be OK (because are DTO have ArrayAccess for example) and we can merge in 2.0.x

pdureau’s picture

pdureau commented
Assigned: just_like_good_vibes » pdureau

Maybe the change will break ecosystem (like ui_icons_patterns) and we will keep it for 2.1.0. Maybe it will be OK (because are DTO have ArrayAccess for example) and we can merge in 2.0.x

Working on this subject.

pdureau’s picture

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

Error: Cannot use object of type Drupal\ui_patterns\FieldMetadata as array in Drupal\ui_icons_patterns\Plugin\Derivative\FieldIconSourceDeriver->getDerivativeDefinitionsForEntityStorageFieldProperty() (line 43 of ui_icons/modules/ui_icons_patterns/src/Plugin/Derivative/FieldIconSourceDeriver.php).

Fixed by introducing a (temporary?) ArrayAccessTrait to implements <code>ArrayAccess interface on DTOs.

#3590426: Reduce the API scope of our plugin managers and #3590492: Tidy the source plugins tags must be merged first.

pdureau’s picture

pdureau commented

There is also SourceInterface::getCustomPluginMetadata() methods, called only twice:

  • $field_name = $this->getCustomPluginMetadata('field_name'); in FieldValueSourceBase
  • $property = $this->getCustomPluginMetadata('property'); in FieldPropertySource

They belong to SourceMetadata I guess, it was forbidden when annotating the properties with comments.

Does that mean they are "special" and something specific must be done?

pdureau’s picture

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

Let's investigate this before sending to review:

Warning: Undefined property: Drupal\ui_patterns\SourceMetadata::$group
pdureau’s picture

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

Let's talk

pdureau’s picture

pdureau commented

This is the 2nd part of a three steps effort to prepare UI Patterns Next while staying in 2.0.x branch for now: https://docs.google.com/presentation/d/11NsN1c5prlWrB0AlEKOxE4q5MOFHx-N_...

#3590492: Tidy the source plugins tags must be merged first.

Added DTO classes implementing ArrayAccess:

  • DerivableContextMetadata
  • FieldMetadata
  • SourceMetadata

Removed metadata:

  • FieldMetadata::configurable
  • FieldMetadata::editorial
  • FieldMetadata::parent_base
  • FieldMetadata::base
  • DerivableContextMetadata::group
pdureau’s picture

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

Oopsy. The change from array to DTO is breaking Display Builder and probably other modules from ecosytem:

TypeError: Drupal\ui_patterns\Attribute\Source::__construct(): Argument #9 ($metadata) must be of type ?Drupal\ui_patterns\SourceMetadata, array given, called in modules/display_builder_entity_view/src/Plugin/UiPatterns/Source/ExtraFieldSource.php on line 22 in src/Attribute/Source.php

Because we target 2.0.x branch, we need to add a compatibility layer.

pdureau’s picture

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

Display Builder compatibility fixed:

  • partially in the current MR with the addition of a remporary safeguard:
         public readonly ?array $context_requirements = [],
-    public readonly ?SourceMetadata $metadata = NULL,
+    public mixed $metadata = NULL,
     public readonly ?array $context_definitions = [],
-  ) {}
+  ) {
+    if (is_array($this->metadata) && !empty($this->metadata)) 
+      $this->metadata = new SourceMetadata(...$this->metadata);
+    }
+
+  }
  • partially in Dispaly Builder: #3608406: Remove metadata attribute from ExtraFieldSource

Adding a DTO is more risky than adding an enum, we may break stuff. Let's be very careful before merging it.

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

smovs’s picture

smovs commented

Hi @pdureau and @just_like_good_vibes!

I used Claude to double-check for edge cases, and it identified two potential concerns.

I added a NULL guard to getChoices() and SourcePluginBase.

There are also concerns about the metadata annotation using the wrong entity type. We currently use DerivableContextMetadata, while Claude suggested that source plugins should use SourceMetadata instead.

Claude also suggested several more invasive code changes, but I didn’t implement them without discussing them first.

Concerns identified by Claude:
1. getChoices() — unguarded NULL->provider (DerivableContextSourceBase.php:503-507)

  $metadata = $derivable_context['metadata'];   // was: ?? []
  'provider' => $metadata->provider,            // was: $metadata['provider'] ?? NULL

The DerivableContext attribute now defaults metadata to NULL. Any derivable-context plugin that doesn't route through EntityFieldSourceDeriverBase (i.e. any contrib one) has metadata === NULL → fatal.
Core's two plugins both inherit a SourceMetadata, which is the only reason it's green.

2. getCustomPluginMetadata() — unguarded property access (SourcePluginBase.php:397-403)
return $plugin_definition['metadata']->{$key}; // was: guarded, returned NULL on miss
Fatals if metadata is NULL (any non-field Source plugin) or a key is absent. Safe today only because the two callers are field sources that always have metadata — but this is a general protected helper any source subclass can call.