Problem/Motivation

In #3479850: [2.0.0-beta4] LinksPropType normalization minor update, it introduced:

  protected static function normalizeLink(array $item): array {
...
    if (!is_string($item["title"])) {
      $item["title"] = (string) $item["title"];
    }

In #3466028: Ensure media library popin is usable, I have a side effect compared with UIP1 + UIP settings due to: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/media...

$display_title = [
        '#markup' => $this->t('<span class="visually-hidden">Show </span>@title<span class="visually-hidden"> media</span>', ['@title' => $title]),
      ];
//      $display_title = $this->t('<span class="visually-hidden">Show </span>@title<span class="visually-hidden"> media</span>', ['@title' => $title]);
      if ($allowed_type_id === $selected_type_id) {
        $display_title = [
          '#markup' => $this->t('<span class="visually-hidden">Show </span>@title<span class="visually-hidden"> media</span><span class="active-tab visually-hidden"> (selected)</span>', ['@title' => $title]),
        ];
//        $display_title = $this->t('<span class="visually-hidden">Show </span>@title<span class="visually-hidden"> media</span><span class="active-tab visually-hidden"> (selected)</span>', ['@title' => $title]);
      }

I have tested with the commented out line it is then ok.

I don't know why Core adds a depth level with #markup, but I get "Array" as link title...

I will search why this i done like that, otherwise I will create a core issue to remove this. But I think links prop type should handle this case because I guess It can still happen elsewhere.

EDIT: nothing found with a blame in core, this code part had been untouched since 5 years. So here is the core issue #3488583: Media library links: remove #markup

Steps to reproduce

Trigger normalize of media library links in a _preprocess_links__media_library_menu:

$variables['preprocessed_items'] = LinksPropType::normalize(\array_filter(
      $variables['links'],
    ));

Proposed resolution

Check if title is a renderable array before cast?

Remaining tasks

CommentFileSizeAuthor
#13 Capture d’écran du 2024-11-21 16-31-33.png52.23 KBgrimreaper
#11 Capture d’écran du 2024-11-21 14-39-42.png112.68 KBgrimreaper

Issue fork ui_patterns-3488582

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

grimreaper created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue summary: View changes

grimreaper opened merge request !270

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Active » Needs review

I made a small change that fix my use case.

At least it can be used to open discussion.

pdureau’s picture

pdureau commented

We don't allow render arrays in prop values, so in links item titles.

Instead of skipping the cast to string, we may render the renderable instead.

Let's discuss

pdureau’s picture

pdureau commented
Title: Side effect in links prop type normalization casting » [2.0.0-beta5] Side effect in links prop type normalization casting
Assigned: Unassigned » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Needs work

Just made a quick call with @pdureau,

Let's add a normalize in StringPropType to handle those cases then call this normalize in LinksPropType.

Also use the occasion to potentially get rid of the normalizeAttributes method in LinksPropType to use directly the normalize of AttributesPropType.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » pdureau
Status: Needs work » Needs review

MR updated

pdureau’s picture

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

I see there were already a call to AttributesPropType::normalize() in LinksPropType::normalize(), but you removed some apparently useless logic executed before. I hope it was not done like that in purpose.

There are 3 phpunit failures in your MR:

  • 1) Drupal\Tests\ui_patterns_field_formatters\Functional\FieldFormatterRenderTest::testRender
  • Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.
  • 2) Drupal\Tests\ui_patterns_field_formatters\Functional\LayoutBuilderFieldFormatterRenderTest::testRender
  • Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.
  • 3) Drupal\Tests\ui_patterns_field_formatters\Functional\LayoutFieldFormatterRenderTest::testRender
  • Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.

Is it related to your change?

Also, maybe we can also move some string related logic (like ::normalizeObject()) from AttributesPropType::normalize() to StringPropType::normalize()

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

but you removed some apparently useless logic executed before. I hope it was not done like that in purpose.

Yes, I think it is a duplicated logic part and the change will give an equivalent result.

The failures are due to the normalize in StringPropType.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
StatusFileSize
new Capture d’écran du 2024-11-21 14-39-42.png112.68 KB

The problem is that it is a render array that is passed and if rendered then later on, the

are escaped. That's why the tests are failing.

I currently don't know if I should adapt the normalize or it highlights an existing problem.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » pdureau

I prefer to discuss it live when you will be back.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
StatusFileSize
new Capture d’écran du 2024-11-21 16-31-33.png52.23 KB

Other side effect of normalizing link title into a string with ui_icons_menu.

pdureau’s picture

pdureau commented

I have rebased and I got a look.

I got 2 phpunit error, but I don't know if it comes from the rebase or not:

1) Drupal\Tests\ui_patterns_field_formatters\Functional\FieldFormatterRenderTest::testRender
Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.
2) Drupal\Tests\ui_patterns_field_formatters\Functional\LayoutBuilderFieldFormatterRenderTest::testRender
Test 'field_formatter_default_2' failed for prop/slot 'props' of component ui_patterns_test:test-component. Selector .ui-patterns-props-string.

Let's talk.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: pdureau » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » pdureau
Status: Needs work » Needs review

  • pdureau committed 1906eb4c on 2.0.x authored by grimreaper 
    Issue #3488582 by grimreaper, pdureau: Side effect in links prop type...
pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Needs review » Fixed
pdureau’s picture

pdureau commented
Status: Fixed » Closed (fixed)