Make the Twig extension link() accept Attribute objects

Very interesting point, that came up briefly in #2342745-32: Allow Twig link function to pass in HTML attributes but didn't make it into any patch.

Do you think it should be able to accept either?

Let's allow both array and object.

@Lukas von Blarer try this out and let us know.

Looks perfect to me.

Looks like a missing use statement for Attribute?

My tests were shit too:)

Looks RTBC, needs a bit of an issue summary update and beta eval, don't have time right now to do so.

@Lukas von Blarer Mind updating the issue summary?

Let's update some old change notice as well.

@jibran I agree but I don't think one exists. We could expand the docs on https://www.drupal.org/node/2486991 after commit.

Updated the issue summary. RTBC with a tiny docs addition to add the @return, didn't think that was worth holding this up.

Awesome. RTBC +1. Thanks for reporting the issue @Lukas von Blarer. Nice fix @joelpittet.

@jibran Do you happen to know where or if there was an old change record?

1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -262,6 +262,22 @@ public function __toString() {
   +    /** @var \Drupal\Core\Template\AttributeValueBase $value */
   

   Instead of doing this, it should be on $this->storage:

   @@ -43,10 +43,11 @@
     * htmlspecialchars() and the entire attribute string is marked safe for output.
     */
    class Attribute implements \ArrayAccess, \IteratorAggregate, SafeStringInterface {
   +
      /**
       * Stores the attribute data.
       *
   -   * @var array
   +   * @var \Drupal\Core\Template\AttributeValueBase[]
       */
      protected $storage = array();
    
   

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -260,17 +260,20 @@ public function getUrlFromPath($path, $options = array()) {
   +   * @param array|Attribute $attributes
   

   This should have FQCN: \Drupal\Core\Template\Attribute

3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -260,17 +260,20 @@ public function getUrlFromPath($path, $options = array()) {
   +      if ($attributes instanceOf Attribute) {
   

   instanceof

4. +++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
   @@ -57,6 +58,7 @@ public function linkGeneratorRender() {
   +      '#attributes' => new Attribute(['class' => ['llama', 'kitten', 'panda']]),
   

   Also, it seems weird that we have #attributes (and $attributes above) getting a single Attribute

@tim.plunkett

re #18.1 It won't merge properly if we don't get the value(). Actually I think this needs another test because it should be a deep merge regardless and that is a bug before this patch.

#18.2 cool thanks, will fix:)

#18.3 interesting didn't know that was the way we were doing it but ok cool.

#18.4 I actually should have put a comment for why I did that because I wanted to explicitly test the attribute object and not assume that special cased conversion. It would do the same thing as we have it but I didn't want the test to make extra assumptions.

18.1, I just meant skipping the inline @var, and moving it to the property itself.

18.3,

$ ag instanceOf --ignore-dir vendor | wc -l
      34
$ ag instanceof --ignore-dir vendor | wc -l
     669

@tim.plunkett well you helped me find a bug, so cool anyways:)

Here's those fixes from #18

I'm not sure we should be changing those other instanceOf, just doing the new one lowercase. Otherwise, looks good.

@Cottser of course you are right ;) Moved those to a novice follow-up #2515018: Lowercase the instances of camelcase'd instanceOf in core for consistency

Nice, back to RTBC. Thanks @joelpittet @tim.plunkett.

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
@@ -43,7 +43,11 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
   $items['twig_theme_test_link_generator'] = array(
...
+    'variables' => [

This is not a good thing because at least my texteditor will give an warning for having new syntax inside old syntax :/

We aren't blocking on array syntax diff.

Lets fix it anyway :)

OK, still RTBC;)

1. This looks like a sensible improvement. It feels almost like a bug fix looking at the issue summary.

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -260,17 +260,20 @@ public function getUrlFromPath($path, $options = array()) {
   +   * @param array|\Drupal\Core\Template\Attribute $attributes
       *   An optional array of link attributes.
   

   These docs need updating.

3. +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
   @@ -43,7 +43,11 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
   -    'variables' => array('test_url' => NULL, 'test_url_attribute' => NULL),
   +    'variables' => [
   +      'test_url' => NULL,
   +      'test_url_attribute' => NULL,
   +      'attributes' => [],
   +    ],
   

   Why is this change necessary?

@alexpott re #30.2 Because we need to define any variable your template expects. Added 'attributes' to explicitly pass an Attribute object to the template. I tried to change the logic to make it looser on defining the variables here but got shot down:P If you want clarity, @fabianx is the one to ping.

Thanks for the note on the docs #30.1, missed that.

#31 addresses #30

Thanks for addressing my feedback. Committed df44c00 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.