[beta4] Fix title attribute handling in button pattern

Hi Spryah

Thanks for your proposal.

Indeed, using $variables['target'] looks better than relying on $variables['attributes']['target']

However, why do you set $variables['target'] = 'blank'; ? This setting doesn't exist in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/d37e350fb3913efb...

Hello @pdureau

Thank you for your reply.
I'm not used to working with UI Patterns so please forgive me if I made a mistake.
I see a target variable in the settings, line 38. Was it not appropriate to use it here?

Hello @spyrah

Oops, my bad. I did a wrong copy-paste in my last comment.

Why did you set $variables['is_external'] = TRUE; ? This setting doesn't exist in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/d37e350fb3913efb...

Aaah, you're absolutely right. Now you're making me realize that we're missing an important behavior in case of an external link defined in the button pattern. The element should get a rel="noopener" attribute, similar to the external links defined in link pattern. Am I allowed to fix this in the same MR or do you require that I open a different issue?

You are allowed ))

is_external prop is still missing from https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/edf77db82ee5a773... and used without being set in https://git.drupalcode.org/project/ui_suite_dsfr/-/blob/edf77db82ee5a773...

In my opinion, instead of adding this prop to the definition and working in 2 steps (preprocess, then twig), we must stay in preprocess only:

  if (UrlHelper::isExternal($variables['url'])
    && !UrlHelper::externalIsLocal($variables['url'], \Drupal::request()->getSchemeAndHttpHost())) {
    $variables['attributes'].setAttribute('rel', 'noopener') = TRUE;
    $variables['target'] = 'blank';
  }

Ah. My bad, I'll fix this.
And I'll open an issue to do the same for the link pattern.

Hi @pdureau,
We had some trouble using your code snippet: we get an error that says "Call to undefined function setAttribute()". We could not manage to figure out in which cases $variables['attributes'] would be empty or null (we tried using instanceof Attribute for example), so instead, we suggest setting an empty Attribute in case $variables['attributes'] were NULL.

  if (UrlHelper::isExternal($variables['url'])
    && !UrlHelper::externalIsLocal($variables['url'], \Drupal::request()->getSchemeAndHttpHost())) {
    $attribute = $variables['attributes'] ?? new Attribute;
    $attribute->setAttribute('rel', 'noopener');
    $variables['attributes'] = $attribute;
    $variables['target'] = 'blank';
  }

Would that be okay with you?

My code snippet was not tested, it was just a draft proposal.

I don't know what would be the best implementation. If your implementation works well, I will merge it.

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