Contrib.social

[2.0.0-alpha2] Formatters: technical feedbacks from alpha1

Open on Drupal.org →
Created on 11 April 2024, 8 months ago
Updated 30 April 2024, 7 months ago

hook_field_formatter_info_alter

Can you tell in a comment the reason why you are altering your own plugins in .module:

function ui_patterns_field_formatters_field_formatter_info_alter(array &$info) {
  /** @var \Drupal\Core\Field\FieldTypePluginManagerInterface $field_type_manager */
  $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
  $field_types = array_keys($field_type_manager->getDefinitions());
  $info['ui_patterns_all']['field_types'] = $field_types;
  $info['ui_patterns_each']['field_types'] = $field_types;
}

It is because it is impossible to assign a field formatter to every field types using the plugins annotations, but not everybody know that.

Why do we need so much cache invalidation?

3 times in .module:

\Drupal::service('plugin.manager.ui_patterns_source') ->clearCachedDefinitions();

2 times in UiPatternsFieldFormattersEntitySchemaSubscriber

$this->uiPatternsSourcePluginManager->clearCachedDefinitions();

Maybe there is something wrong about cache definition somewhere.

Mapping is done on field types instead of field property types

FieldPropertiesSourcePropDeriver::getPropTypeToFieldTypeMap() has a $prop_mapping variable with field types as values:

    $prop_mapping = [
      'string' => [
        'changed',
        'created',

This is not the mechanism described by the specifications.

Using field types instead of field property types, we are forced to maintain an always evolving list of field types, from core, contrib & custom.
We are pulling the data from field properties to component props anyway, so why not mapping field property types to prop types?

Remove PHPMD Naming annotations

Example:

 * @SuppressWarnings(PHPMD.CamelCaseParameterName)
 * @SuppressWarnings(PHPMD.CamelCaseVariableName)
 * @SuppressWarnings(PHPMD.LongClassName)
 * @SuppressWarnings(PHPMD.LongVariable)

Because we are not running this test which is incompatible with Drupal standard. We are only running codesize, unusedcode & design.

So much plugins :)

I know you warn us about this implementation decision in January, but I am still uncomfortable with deriving source plugins, one source for each property of each field of each content of each bundles of each content entitty types of the website. Even if we never use 99% of them.

📌 Task
Status

Fixed

Version

2.0

Component

UI Patterns Field Formatters [2.x only]

Created by

🇫🇷France pdureau Paris

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @pdureau
  • First commit to issue fork.
  • Status changed to Needs review 7 months ago
  • Comment 7 months ago →
    🇫🇷France just_like_good_vibes PARIS

    Hello,
    All modified code is done in related Issue #3440319 🐛 [2.0.0-alpha2] Formatters: functional feedbacks from alpha1 Needs review .

    All answers to questions here :

    1. Comment(s) added in hook_field_formatter_info_alter in .module
    2. All cache invalidation removed in .module, also because source plugins no longer depend on bundle. cache invalidation in the event subscriber is needed to respond to modeling modifications without needing to clear the cache in order to see the corresponding source plugins appearing or disappearing.
    3. the Mapping you are challenging had bad comments. it is in fact a mapping between ui patterns prop types and data types (e.g. types for props). So it should be ok, right? should we provide a mechanism to let eventual new prop types from contrib be handled ?
    4. PHPMD Naming annotations removed.
    5. The number of plugins has been reduced to reflect the triplets (entity type, field_name, property name). bundles are managed in a more subtle way.
  • Status changed to Needs work 7 months ago
  • Comment 7 months ago →
    🇫🇷France pdureau Paris
    Comment(s) added in hook_field_formatter_info_alter in .module

    ✅ Great.

    All cache invalidation removed in .module, also because source plugins no longer depend on bundle. cache invalidation in the event subscriber is needed to respond to modeling modifications without needing to clear the cache in order to see the corresponding source plugins appearing or disappearing.

    ✅ OK, got it.

    the Mapping you are challenging had bad comments. it is in fact a mapping between ui patterns prop types and data types (e.g. types for props). So it should be ok, right?

    ✅ Mapping between ui patterns prop types and typed data types (so, field properties types) is the way to go.

    should we provide a mechanism to let eventual new prop types from contrib be handled ?

    Interesting. Let's talk about that.

    PHPMD Naming annotations removed.

    ❌ not yet :)

     $ grep -r SuppressWarnings.*PHPMD | wc -l
28
    The number of plugins has been reduced to reflect the triplets (entity type, field_name, property name). bundles are managed in a more subtle way.

    ✅ that's wonderful.

  • Status changed to Needs review 7 months ago
  • Comment 7 months ago →
    🇫🇷France just_like_good_vibes PARIS

    PHPMD Naming annotations finally removed :)
    (still done in issue #3440319 🐛 [2.0.0-alpha2] Formatters: functional feedbacks from alpha1 Needs review )

  • Status changed to Fixed 7 months ago
  • Comment 7 months ago →
    🇫🇷France pdureau Paris
  • Comment 7 months ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024