🇫🇷France @Grimreaper

Not sure how to add the tests for that.

grimreaper → created an issue.

Doing a quick fix

grimreaper → made their first commit to this issue’s fork.

Needs to fix code quality.

As discussed this morning in weekly UIP2, merged!

Thanks everyone!

grimreaper → created an issue.

Hi,

Facing the same issue.

Thanks for the patch and MR!

PHPUnit failure in MR seems unrelated to the change.

grimreaper → created an issue.

grimreaper → created an issue.

But then that would be a big change in the philosophy of the module

Totally.

That it uses JSON:API as its transfer method. We'd be saying that we use JSON:API for the content, but custom endpoints for relationships.

No change for this aspect, no special endpoint for relationships.

"Just" an endpoint to retrieve the list of JSON:API list endpoint URLs to retrieve.

As I wrote in comment 15:

The client website can then do the batches on those lists. And when handling relationships, if the referenced entity exists, just reference it and that's it, it will be updated later in the batch. If the referenced entity does not exists, create a placeholder/empty entity (I guess like what Migrate does for chicken/egg problem).

Need to see when implementing if this be ok.

So we'd either need a new family of plugins

Yes, that's what I wrote in comment 15:

It would require:
- a new endpoint on entity share server
- a new method on import processor plugins to get the query parameters to add in case it is enabled
- a new plugin type on entity share server to make the endpoint response dynamic and which will listen to the query parameters.

In either case we'd need some sort of config on the server that mirrors the import config on the client.

I don't think so, with these new server side plugins, there is no need for them to be enabled or not, they can be active all the time.

And if we do that, then I would seriously consider moving away from bundle channels, as having to make a channel for every bundle is a pain. Bundle-based endpoints is part of Drupal JSONAPI's opinionated design, which works for using JSONAPI in general, but isn't really useful or helpful for Entity Share.

Great idea! That would be the occasion to improve this system in depth too.

Hi,

Review comments had been addressed and/or waiting for reply.

Back to needs review.

Hi,

All code review threads had been fixed.

Issue still in needs review if other feedback are needed.

Hi,

Review on my side too.

A few nits, otherwise looks good to RTBC.

Thanks!

As asked in https://drupal.slack.com/archives/C079NQPQUEN/p1762178751187219?thread_t...

Here are 3 use cases:
- Core ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed
- Contrib: Layout Builder Browser #3206268: Allow image paths to be relative to a module or theme →
- Contrib: UI Styles (example from UI Suite Bootstrap)

...
  options:
    border:
      label: "Additive All"
      # icon: 'theme://ui_suite_bootstrap/assets/images/icons/bootstrap/border-outer.svg'
      icon: "data:image/svg+xml;base64,[base64EncodedString]"
...

More globally, at minimum any module using '#theme' => 'image', and I guess other element/theme hook with URI.

Hello,

Last code review taken into account.

Per comment 67, back to RTBC.

Closing as fixed in original issue.

Ok, should I let you do the revert in the other issue?

Not sure what I can do more.

If there is anything I can do, ping me.

I have completed the test coverage.

Remaining test failures in the pipeline are unrelated.

It seems that beyond the PHPStan error there is regression in this test method: https://git.drupalcode.org/issue/drupal-3554820/-/jobs/7067842

Backwards Compatibility Class Loader (Drupal\KernelTests\Core\ClassLoader\BackwardsCompatibilityClassLoader)
     ✔ Translation wrapper
     ✔ Module moved class
     ✘ Doctrine exception
       ┐
       ├ Failed asserting that string matches format description.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊  @expectedDeprecation:
       ┊ -%A··Class·Doctrine\Common\Annotations\AnnotationException·is·deprecated·in·drupal:11.3.0·and·is·removed·from·drupal:12.0.0,·use·Drupal\Component\Annotation\Doctrine\AnnotationException·instead.·See·https://www.drupal.org/node/3551049
       ┊ -··Method·"Twig\Extension\ExtensionInterface::getFunctions()"·might·add·"array"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·implementation·"Drupal\Core\Template\TwigExtension"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··The·"Twig\Environment::getTemplateClass()"·method·is·considered·internal.·It·may·change·without·further·notice.·You·should·not·extend·it·from·"Drupal\Core\Template\TwigEnvironment".
       ┊ +··Method·"Twig\Extension\ExtensionInterface::getFunctions()"·might·add·"array"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·implementation·"Drupal\Core\Template\TwigExtension"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··Method·"Twig\Extension\ExtensionInterface::getFilters()"·might·add·"array"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·implementation·"Drupal\Core\Template\TwigExtension"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··Method·"Twig\Extension\ExtensionInterface::getNodeVisitors()"·might·add·"array"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·implementation·"Drupal\Core\Template\TwigExtension"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··Method·"Twig\Extension\ExtensionInterface::getTokenParsers()"·might·add·"array"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·implementation·"Drupal\Core\Template\TwigExtension"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··Method·"Twig\Loader\FilesystemLoader::findTemplate()"·might·add·"?string"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·child·class·"Drupal\Core\Template\Loader\FilesystemLoader"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··Method·"Twig\Loader\LoaderInterface::exists()"·might·add·"bool"·as·a·native·return·type·declaration·in·the·future.·Do·the·same·in·implementation·"Drupal\Core\Template\Loader\StringLoader"·now·to·avoid·errors·or·add·an·explicit·@return·annotation·to·suppress·this·message.
       ┊ +··The·"cache.backend.memory"·service·is·deprecated·in·drupal:11.3.0·and·is·removed·from·drupal:13.0.0.·Use·cache.backend.memory.memory·instead.·See·https://www.drupal.org/node/3546856
       ┊ +··The·"Drupal\Core\Database\Query\Select::hasAllTags()"·method·will·require·a·new·"string·...·$tags"·argument·in·the·next·major·version·of·its·interface·"Drupal\Core\Database\Query\AlterableInterface",·not·defining·it·is·deprecated.
       ┊ +··The·"Drupal\Core\Database\Query\Select::hasAnyTag()"·method·will·require·a·new·"string·...·$tags"·argument·in·the·next·major·version·of·its·interface·"Drupal\Core\Database\Query\AlterableInterface",·not·defining·it·is·deprecated.
       │
       │ /builds/issue/drupal-3554820/core/tests/Drupal/TestTools/Extension/DeprecationBridge/ExpectDeprecationTrait.php:72
       ┴

grimreaper → created an issue.

Hello,

Thanks for your reviews and feedbacks.

Draft CR created: https://www.drupal.org/node/3554585 →

I removed the todos in favor of proper deprecation warnings.

Should I create a follow-up issue for the deprecated code removal in Core 12?

Hi,

Thanks for the reivew!

Did an update regarding small changes. Waiting for discussion approval for the remaining points.

For comment 64, I think you are right, I have not tested the hook_info_alter for styles. Looking at DefaultPluginManager:

protected function alterDefinitions(&$definitions) {
    if ($this->alterHook) {
      $this->moduleHandler->alter($this->alterHook, $definitions);
    }
  }

So the alterDefinition of the stylePluginManager should be changed as???:

  protected function alterDefinitions(&$definitions) {
    parent::alterDefinitions($definitions);
    if ($this->alterHook) {
      $this->themeManager->alter($this->alterHook, $definitions);
    }

    /** @var \Drupal\Core\Theme\Style\StyleDefinitionInterface[] $definitions */
    foreach ($definitions as $definition_key => $definition) {
      if (!$definition->isEnabled()) {
        unset($definitions[$definition_key]);
      }
    }
  }

Hello,

Thanks @rikki_iki for your feedbacks.

Same answer for both points.

This API is driven by the Design token specification (currently in draft) https://www.designtokens.org/tr/drafts/format/. For interoperability with design tools, either creating or consuming design token tools.

And for the moment the only allowed units are "px" and "rem" https://www.designtokens.org/tr/drafts/format/#dimension. I was also surprised that for the dimension type only "px" and "rem" units were allowed.

This can be discussed upstream in https://github.com/design-tokens/community-group (where I know drupalers are already interacting :)) to avoid to have a custom Drupal "layer" or specification.

It seems that there is already an issue about that https://github.com/design-tokens/community-group/issues/245

Discussed with @pdureau,

Changes related to removing special case of #item_attributes to use #attributes instead moved to a dedicated issue ✨ Use #attributes instead of #item_attributes Active to simplify the MR of style API issue.

grimreaper → created an issue.

I have completed the tests:

- Attribute object
- Attribute helper
- renderer
- bubbleablemetadata

I have not found an existing test for the one line addition of HtmlResponseAttachmentsProcessor

I started to write kernel test for the renderer to see resulting render array after processing, but this was equivalent to unit tests already done.

Same for calling Attribute in Twig, with the unit tests on it PHP side I think it is ok.

Now waiting for reviews and feedbacks!

Thanks!

Updating remaining tests to write.

To test:
-
-
-
-
-
-
-
- Attribute object addStyle and changes
- Attribute object addStyle through Twig
- Attribute helper changes
- style application logic
- bubbleablemetadata change
- unit or kernel or functional test on renderer change, aka apply on render array

Pipeline is green now!

Adding new tests.

Removing tag "Needs issue summary update" per comment 45.

Reworked how Attribute object handle attachments to avoid side effects during rendering process and fix existing tests.

Only remaining existing test not passing is core/modules/syslog/tests/src/Kernel/SyslogTest.php

Because syslog config is null in the service during execution. I think it is due to the logger factory service added to the renderer service. So during test execution, the logger service is not recreated with the module's config imported.

But I had not the time to ensure that and figure out a fix yet.

Updating issue title and summary.

Back to RTBC.

Hi,

Canvas is not the only alternative.

There is also https://www.drupal.org/project/display_builder → , related to https://www.drupal.org/project/ui_suite → .

Disclaimer: I am involved in Display Builder development and UI Suite core team.

PS: Hi @murz, hope you enjoy the apron from Trivia night ;)

Parent issue is now merged!

To restore when Core 11.3 will be released.

And need to update Core requirement.

Updated MR to fix existing core tests and code quality.

Added an admin permission to the DesignToken entity for easier integration with existing tests like rest and JSON:API tests. And also at least now the entity has an admin permission for later APIs.

PS: remaining test failure seems unrelated.

Finally green tests.

Fixed provided on ui_examples 🐛 Using item_list in examples break rendering Active .

Please port the fix here.

grimreaper → made their first commit to this issue’s fork.

Thanks for the quick merge :)

Just thinking.

With ✨ Add a style utility API Active , I think it will be possible to set:

attached:
  styles:
    my_style: my_option

And with the proposed event subscriber (removed from Core for now, but it could be moved in ui_styles until then) then styles could be attached to body from the render array of the example.

grimreaper → created an issue.

Hi @anmolgoyal74,

Thanks for the MR!

I put 2 review comments.

Also please ensure code quality jobs are ok.

Hi,

Form element updated for easier override of definitions obtention logic.

I will complete the tests during the coming days.

In the meantime if people can give reviews and feedbacks to ensure architecture.

I will appreciate to not write tests on stuff which would potentially be removed or reworked ;)

Thanks!

Thanks @cedric_a for the tests.

About styles.yml it will be specified in the change record.

About #styles not working, it is because you have tested in a preprocess and potentially put #styles NOT on a render element. If you put #styles on a render element it will work:

$build['test'] = [
  '#type' => 'container',
  '#styles' => [
    'background_color' => 'dark',
  ],
];

Hello,

Rebased done.

I also updated tests to fix some.

I think the remaining tests failure are not related to the MR.

Proposed new title:

Different modules can't override the same entity type class

Proposed summary:

If an entity class is overridden by multiple modules, then static call of intermediary overriding class will raise error as not possible to find matching entity type.

Example with entity_view_display overridden by Layout Builder LayoutBuilderEntityViewDisplay then overridden by another contrib or custom module.

In layout_builder.install, the following code will raise an error:

function layout_builder_install(): void {
...
  $displays = LayoutBuilderEntityViewDisplay::loadMultiple();
...

Proposed resolution

The entity type should be aware of all intermediary class overrides.

Thanks for the merge!

Before removing, I will do a test with @nod_ suggested usage of DOM. Then will remove.

And about remaining todo:

- update form element to be able to filter by target, provider, not just theme and kind.

I got an idea, I will isolate in a protected method the gathering of definitions, so that it will be easier to only override this part for contrib needs.

Thanks for the feedbacks.

JSON schemas: removed

Discovery: I tried something but see review comment in MR, YamlDirectoryDiscovery expects an identifier key which is not present as token identifier is its path.

grimreaper → created an issue.

grimreaper → created an issue.

Pushed WIP JSON:Schema validation.

Put similar mechanism as Icon API and Style API MR, but it validates against invalid/non-existing type. I think it is because of remote schema references. Didn't have time to ensure it is due to that yet.

DONE:
- minimum config entity
- config schema
- rework design token value plugin data injection
- CSS generation/serialization from config entities

TODO:
- validate tokens.yml againsi JSON schema

DONE:
- design token value plugin types
- some design token values to POC, simple and a composite one
- put parsing logic entirely in design token plugin manager

TODO:

- need to take care of name with spaces.
- config entity
- tests

Dropping some notes after discussion with @pdureau. Plus personal thinking.

Plugin type PHP, token type
- applicable (either method or in plugin annotation)
- toCssVariable
- fromDtcg
- getFormElement (for Core 11.4)
In DesignToken plugin, value should be an instance of a design tokenValue plugin with value.
Serialization only for config entity.

design_token.group__group__border_radius.yml:
id: 'group__group__border_radius'
type: 'border'
scopes:
:root:
value: '5'
unit: 'px'
%foo:
value: '10'
unit: 'em'

Config schema:

design_token.type.border:
type: mapping
...
design_token.type.font_family:
type: string
...

grimreaper → created an issue.

Hello,

Thanks for your suggestion.

In short, I would say yes it will still be possible in extension as JSON schema allow additional unknown properties. But that's not the direction we want to promote.

I understand the feeling of code duplication. Know that with UI Suite Bootstrap, I went that way :D

The problem with such syntax is that it ties the style declaration to a form element and with our experience in UI Suite (UI Patterns, UI Styles, UI Skins, etc.) is that we want to decouple the declaration of the design system artifact (component, style, design token) to the UI forms (data can come from something else than a form).

1: because it is the front dev who declares the styles and how it will be applied, not how it will be configured. The style definition should know nothing (or as less as possible, maybe only suggest stuff) about Drupal Form API, where it will be configured. This would introduce drupalism into the declaration
2: having such link between style declaration and form element, would prevent (or make it harder) other contrib or custom alteration to choose something else.
3: concrete example of styles with Bootstrap:

flex_order:
  category: "Flex"
  label: "Flex order"
  description: "Change the visual order of specific flex items with a handful of order utilities. We only provide options for making an item first or last, as well as a reset to use the DOM order. As order takes any integer value from 0 to 5, add custom CSS for any additional values needed. Additionally there are also responsive order-first and order-last classes that change the order of an element by applying order: -1 and order: 6, respectively."
  links:
    - 'https://getbootstrap.com/docs/5.3/utilities/flex/#order'
  options:
    order_first:
      label: "First"
      value: "order-first"
    order_last:
      label: "Last"
      value: "order-last"
    order_0:
      label: "Order 0"
      value: "order-0"
    order_1:
      label: "Order 1"
      value: "order-1"
    order_2:
      label: "Order 2"
      value: "order-2"
    order_3:
      label: "Order 3"
      value: "order-3"
    order_4:
      label: "Order 4"
      value: "order-4"
    order_5:
      label: "Order 5"
      value: "order-5"

spacing_margin:
  category: "Spacing"
  label: "Margin"
  links:
    - 'https://getbootstrap.com/docs/5.3/utilities/spacing/#notation'
  options:
    m_0:
      label: "0"
      value: "m-0"
    m_1:
      label: "1"
      value: "m-1"
    m_2:
      label: "2"
      value: "m-2"
    m_3:
      label: "3"
      value: "m-3"
    m_4:
      label: "4"
      value: "m-4"
    m_5:
      label: "5"
      value: "m-5"
    m_auto:
      label: "Auto"
      value: "m-auto"

You will regularly have some styles with options mixing number and string so no possible to get a range.
4: Form alteration is possible (I am doing it in UI Styles with a different form element extending the one provided in this MR). But style application is currently done by the style definition (or at least providing helper in it), maybe that's not the correct approach, which will check options and not other properties.

Hi,

Thanks for pushing the MR forward.

My personal preferences (regarding your last comment):
- {{ attributes.addClass(['foo', 'bar']): so the opposite of current state of Drupal files, and what TwigCS does by default.
- {{ include('template.twig', {}, with_context: false) }}: agreed with you.

Not sure if TwigCS configuration had been discussed in this issue or in another one.

Hello,

It is in the scope (see comment 16) and already implemented in the MR.

- preprocess HTML to set global theme: to replace with event subscriber executed before the one in Core handling html_attributes and body attributes, so able to set attachments.
- theme form settings alter: to put in UI Styles as now modes will be styles managed.
- 2 styles form elements: one for html, one for body. Need to filter out styles by target (not just kind)
- that way possible to handle ✨ Add an interface to apply ui styles to the page body? Active

EDIT: no need of other event subscriber supposedly.

grimreaper → created an issue.

grimreaper → created an issue.

That way more complicated stuff will be doable like #3259377: Add an ImportProcessor plugin to fetch translations → , ✨ Handle default language and its translations at once Needs work or ✨ Support Crop API Active where crop entities does not reference the image properly only with an ID.

And for the original problem of memory/timeout, the client website can then do the batches on those prepared lists.

So the batch could have more fine grained operations.

And during entity list import, no more need of the current entity reference/embedded entities, etc mechanism which pops other imports.

Also for JSON:API Extras field enhancer plugins, no more need to alter JSON:API output to provide data for the client to search for referenced blocks, etc. Server website will have already prepared the JSON:API urls to search directly.

TODO

UI Styles:
- Update Stylesheet generator:
- Check if drilling is still needed:
- Check if MachineNameTrait is still needed:
- tests updated:
- provide Drush code upgrade command:
- update documentation

UI Styles Block:
- current features:
- tests updated:
- config update:

UI Styles CKE5:
- current features:
- tests updated:
- config update:
- need to pass to a system of custom attributes to able to create a filter plugin that will be able to add attributes and attach library.
- on-the-fly style generation wrapped in .ck-content may be simplified to only load the library of the enabled style plugins. No more need to load all the libraries with parent theme recursively.

UI Styles Entity Status:
- current features: OK
- tests updated:
- config update:

UI Styles Layout Builder:
- current features:
- convert hook_entity_view_alter into hook_entity_view?
- tests updated:
- config update:

UI Styles Page:
- current features: OK
- Add utilities on html and body to have equivalent of ui_skins mode.
- tests updated:
- config update:

UI Styles Library:
- current features: OK
- Rework library:
- tests updated:

UI Styles UI Patterns:
- current features:
- tests updated:
- config update:

UI Styles Views:
- current features:
- tests updated:
- config update:

UI Styles 1:
- warn extra deprecated
- mark stuff as deprecated

I will start/continue to update contrib modules to test with the new API.

Most of the stuff remaining here are to write tests.

If some review could be done to indicate if there is a blocker or an architecture problem before writing tests on stuff that will require rework it would be nice.

Thanks!

TODO:
-
-
-
-
-
-
-
- support of prefers-color-scheme or light-dark()

To test:
-
-
-
- StyleDefinition applyOrBubbleOnAttribute
- StyleDefinition applyOnAttribute
- StyleDefinition applyOrBubbleOnArray
- Plugin manager getDefinitionsForTheme.
- Form element with theme.
- Attribute object
- Attribute helper
- style application logic
- event subscriber
- bubbleablemetadata change

TODO:
- finish event subscriber for html/body
- add helper/refactor how to apply styles. Put logic into Style definition to avoid duplication between renderer and Attribute?
- drop short syntax for options.
- description of option.
- item_attributes? When adding a style, add optional parameter to specify attribute key?
- add lifecycle/deprecation properties on style and/or on style options?
- ensure big_pipe compatibility? not sure if relevant that a style alters body/html attributes asynchronously after page had been rendered.

To test:
- Plugin manager getDefinitionsForTheme.
- Form element with theme.
- target property is now an enum
- Attribute object
- Attribute helper
- no more option short syntax
- description on option
- style application logic
- event subscriber