🇫🇷France @Grimreaper

As discussed, I went beyond to also propose start and end for buttons.

Assigning to me for a check after rebase now that UIP1 upgrade had been merged.

CI updated.

Still a problem with a phpmd file path.

grimreaper → created an issue.

Doing some test with CI.

I got feedbacks for PHPStan.

The ContentEntityInterface::getBundleEntity had been introduced in Core 11.3, so we need to ensure the code quality tasks are executed on 11.3.

I think instead of a new composer task for 11.3, need to override the default composer one.

For the remaining PHPStan error, I think it is a PHPStan bug, because the interface has the method.

I have asked if it is a PHPStan bug on Slack: https://drupal.slack.com/archives/C033S2JUMLJ/p1763572638687859

Fixing CI before review.

grimreaper → created an issue.

Hi,

Thanks for pushing this forward!

In the previous comment, I think the second link to Github issue was meant to target: https://github.com/VincentLanglet/Twig-CS-Fixer/issues/402

Because currently both links target the same issue.

grimreaper → created an issue.

Hello,

In 📌 Ci: add SDC Devel check in ci Active we will change variants to use underscore instead of caret.

grimreaper → created an issue.

Hi,

Yes ui_patterns_legacy remains enabled at the end of the updates in case some code in Twig templates had not been updated because ui_patterns_legacy provides a compatibility layer at execution time too.

I also thought about that.

Either: add a message at the end of the updates
Or: add a settings in settings.php to allow to skip UIP1 config upgrade
Or: skip the updates if we can detect that an UIP2 module or feature is already present
Or: add a "marker" (state) in UIP1 with an update and tell to update from UIP1 from this version

Hi,

All latest code review comments had been addressed.

Thanks for the new review round!

Follow up created: #3557461: Deprecate EntityTypeInterface::getOriginalClass →

Draft change record created: https://www.drupal.org/node/3557464 →

TODO: finish updating tests.

grimreaper → created an issue.

I think of a simple test to add is to ensure conversion is idempotent.

Hum, I think I know why you got the warning.

Are you upgrading a website from UIP1 to UIP2 or the website was already with UIP2 before the update?

Hi,

Thanks for your feedback on the config upgrade.

Can you please share the structure of the UIP1 config being updated?

To understand how this case can happen.

Seen together, the remaining bug of Pierre was from an outdated version of ui_styles.

grimreaper → created an issue.

Add link to Drupal l10n Composer plugin.

Hello,

As seen together, I do not reproduce the warning.

And messages block is never empty, there is a placeholder div in case JS generates messages.

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.