Move Attribute classes under Drupal\Component

Thanks for this. For reference here is the issue where Attribute was first introduced: #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess()

If we do go this route I am almost certain we'd need to keep \Drupal\Core\Template\Attribute and related classes as wrappers for backwards compatibility.

This could probably be categorized as a task but either way I think it would land in 8.1.x at the earliest so changing the version.

Thanks, sending for bot review :)

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -7,345 +7,8 @@
+class Attribute extends \Drupal\Component\Attribute\Attribute { }

I don't think these fit with our coding standards, I think we need to maybe use as something else. https://www.drupal.org/node/1353118

If we do that, we should also move \Drupal\Tests\Core\Template\AttributeTest to core/tests/Drupal/Tests/Component

Discussed this with @alexpott a bit at the sprints at DrupalCon Asia.

He pointed out the Attribute currently has a dependency on \Drupal\Component\Utility\SafeMarkup which is deprecated, it would be really nice to remove that dependency before moving these classes to the Component namespace. So it would be great to get #2579691: Remove usages of SafeMarkup::isSafe() in before this.

@aleksip
The way around that here is to implement a custom implementation of MarkupInterface inside the file, so something like:

class TestMarkup implements MarkupInterface {
  use MarkupTrait;
}

#2579691: Remove usages of SafeMarkup::isSafe() is in so this needs a reroll now.

Thanks!

1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -7,344 +7,10 @@
   +use Drupal\Component\Attribute\Attribute as AttributeAttribute;
   
   +++ b/core/lib/Drupal/Core/Template/AttributeArray.php
   @@ -7,100 +7,10 @@
   +use Drupal\Component\Attribute\AttributeArray as AttributeAttributeArray;
   

   I think it might be nicer to prefix these with Component instead of Attribute. Similar to how this was done:

   use Drupal\Component\Discovery\YamlDirectoryDiscovery as ComponentYamlDirectoryDiscovery;

2. +++ b/core/lib/Drupal/Core/Template/AttributeArray.php
   @@ -7,100 +7,10 @@
   + * @deprecated in Drupal 8.1.0, will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Component\Attribute\AttributeArray
   

   There needs to be a period at the end of the "Use [class]" part in these deprecation docs per https://www.drupal.org/node/1354#drupal (proper sentences), and looking at other examples I found in core.

3. It's too late to do this in 8.1.x per https://www.drupal.org/core/d8-allowed-changes#minor (new deprecations) so let's update the deprecation messages to say 8.2.0 please :)

This is looking good.

However, given the efforts over in #2572635: Fix 'Drupal.Commenting.DocComment.LongFullStop' coding standard (and more generally #2571965: [meta] Fix PHP coding standards in core, stage 1, it would be nice for this change to go in without introducing new PHPCS issues. For instance:

$> phpcs --standard=Drupal ./Core/Template/Attribute.php

FILE: ...butions/cores/drupal/core/lib/Drupal/Core/Template/Attribute.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 12 | ERROR | [ ] Missing short description in doc comment
 16 | ERROR | [x] There should be no white space after an opening "{"
 16 | ERROR | [x] There should be no white space before a closing "}"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 72ms; Memory: 6Mb

To run PHPCS only on the changed/staged files:

git diff --name-only --cached | grep -v "core/tests/Drupal/Tests/Core/Template/AttributeTest.php" | xargs phpcs --standard=Drupal

(Note the weird grep is to remove the old/renamed file--this method is modified from #1266444: DrupalCI should return failed for poor code style - php)

I don't think they all need to be fixed, but the ones regarding the empty doc comments should be fixed here while the context is still relevant, rather in the huge issue attempting to add doc comments where they are missing.

That sounds reasonable to me, and I think we have time to fix those CS issues and get this into 8.2.x. Thanks @jhedstrom!

Thanks @aleksip! I think all of the above concerns and issues have been addressed.

Missing parameter name
for unlimited unnamed parameters I don't know what to do about.

I also do not know, but it probably need not be addressed here. My main concern was that this patch not add new issues that would need to be dealt with later.

Adding parent.

Also all the @file blocks need to be removed.

Reroll of #23

@kostyashupenko
#26 needs to be addressed.

Please use the git configuration from https://www.drupal.org/documentation/git/configure, see

[diff]
  renames = copies

. This allows you to create much smaller patches when files needs to be "just" moved

+++ /dev/null
@@ -1,454 +0,0 @@
diff --git a/core/themes/seven/css/components/user.icons.admin.css b/core/themes/seven/css/components/user.icons.admin.css

diff --git a/core/themes/seven/css/components/user.icons.admin.css b/core/themes/seven/css/components/user.icons.admin.css
new file mode 100644

new file mode 100644
index 0000000..7df6ba2

index 0000000..7df6ba2
--- /dev/null

--- /dev/null
+++ b/core/themes/seven/css/components/user.icons.admin.css

+++ b/core/themes/seven/css/components/user.icons.admin.css
+++ b/core/themes/seven/css/components/user.icons.admin.css
@@ -0,0 +1,15 @@

@@ -0,0 +1,15 @@
+/**
+ * @file
+ * Styling for the user module icons.
+ */
+
+/**
+ * Toolbar tab icon.
+ */
+.toolbar-bar .toolbar-icon-user:before {
+  background-image: url(../../../../misc/icons/bebebe/person.svg);
+}
+.toolbar-bar .toolbar-icon-user:active:before,
+.toolbar-bar .toolbar-icon-user.is-active:before {
+  background-image: url(../../../../misc/icons/ffffff/person.svg);
+}

These are unrelated changes, probably caused by a bad reroll

Oh, i am so sorry, my bad. There is really unrelated changes from another issue :) i've added too much unnecessary files

New reroll of #23

@kostyashupenko
Please read me full comment (hint, git config)

I successfully applied #30 to 89712cf26fe49e446c49eba912d9961744243b9c (based on git log -- core/lib/Drupal/Core/Template/)
Here's the diff with renames = copies. Perhaps I'll do a re-roll later if you guys don't want to pursue this anymore.

And is moving decoupled stuff from core to component really a Feature request? How about a Task?

Rerolled #33.

Addressed #26.

This is just a quick driveby review.

+++ b/core/tests/Drupal/Tests/Component/Attribute/AttributeTest.php
@@ -1,18 +1,23 @@
 
@@ -74,6 +79,7 @@ public function testRemove() {

@@ -74,6 +79,7 @@ public function testRemove() {
 
   /**
    * Tests setting attributes.
+   *
    * @covers ::setAttribute
    */
   public function testSetAttribute() {
@@ -102,6 +108,7 @@ public function testSetAttribute() {

@@ -102,6 +108,7 @@ public function testSetAttribute() {
 
   /**
    * Tests removing attributes.
+   *
    * @covers ::removeAttribute
    */
   public function testRemoveAttribute() {
@@ -141,6 +148,7 @@ public function testRemoveAttribute() {

@@ -141,6 +148,7 @@ public function testRemoveAttribute() {
 
   /**
    * Tests adding class attributes with the AttributeArray helper method.
+   *
    * @covers ::addClass
    */
   public function testAddClasses() {
@@ -184,20 +192,30 @@ public function testAddClasses() {

@@ -184,20 +192,30 @@ public function testAddClasses() {
 
     // Add an array of classes.
     $attribute->addClass(array('red', 'green', 'blue'));
-    $this->assertArrayEquals(array('banana', 'aa', 'xx', 'yy', 'red', 'green', 'blue'), $attribute['class']->value());
+    $this->assertArrayEquals(
+      array('banana', 'aa', 'xx', 'yy', 'red', 'green', 'blue'),
+      $attribute['class']->value()
+    );
 
     // Add an array of duplicate classes.
-    $attribute->addClass(array('red', 'green', 'blue'), array('aa', 'aa', 'banana'), 'yy');
+    $attribute->addClass(
+      array('red', 'green', 'blue'),
+      array('aa', 'aa', 'banana'),
+      'yy'
+    );
     $this->assertEquals('banana aa xx yy red green blue', (string) $attribute['class']);
   }
 
   /**
    * Tests removing class attributes with the AttributeArray helper method.
+   *
    * @covers ::removeClass
    */
   public function testRemoveClasses() {
     // Add duplicate class to ensure that both duplicates are removed.
-    $classes = array('example-class', 'aa', 'xx', 'yy', 'red', 'green', 'blue', 'red');
+    $classes = array(
+      'example-class', 'aa', 'xx', 'yy', 'red', 'green', 'blue', 'red',
+    );
     $attribute = new Attribute(array('class' => $classes));
 
     // Remove one class.
@@ -224,6 +242,7 @@ public function testRemoveClasses() {

@@ -224,6 +242,7 @@ public function testRemoveClasses() {
 
   /**
    * Tests checking for class names with the Attribute method.
+   *
    * @covers ::hasClass
    */
   public function testHasClass() {
@@ -239,6 +258,7 @@ public function testHasClass() {

@@ -239,6 +258,7 @@ public function testHasClass() {
 
   /**
    * Tests removing class attributes with the Attribute helper methods.
+   *
    * @covers ::removeClass
    * @covers ::addClass
    */
@@ -251,12 +271,15 @@ public function testChainAddRemoveClasses() {

@@ -251,12 +271,15 @@ public function testChainAddRemoveClasses() {
       ->removeClass(array('red', 'green', 'pink'))
       ->addClass(array('apple', 'lime', 'grapefruit'))
       ->addClass(array('banana'));
-    $expected = array('example-class', 'blue', 'apple', 'lime', 'grapefruit', 'banana');
+    $expected = array(
+      'example-class', 'blue', 'apple', 'lime', 'grapefruit', 'banana',
+    );
     $this->assertArrayEquals($expected, $attribute['class']->value(), 'Attributes chained');
   }
 
   /**
    * Tests the twig calls to the Attribute.
+   *
    * @dataProvider providerTestAttributeClassHelpers
    *
    * @covers ::removeClass
@@ -271,7 +294,7 @@ public function testTwigAddRemoveClasses($template, $expected, $seed_attributes

@@ -271,7 +294,7 @@ public function testTwigAddRemoveClasses($template, $expected, $seed_attributes
   }
 
   /**
-   * Provides tests data for testEscaping
+   * Provides tests data for testEscaping.
    *
    * @return array
    *   An array of test data each containing of a twig template string,
@@ -337,35 +360,48 @@ public function testIterate() {

@@ -337,35 +360,48 @@ public function testIterate() {
    * Tests printing of an attribute.
    */
   public function testPrint() {
-    $attribute = new Attribute(array('class' => array('example-class'), 'id' => 'example-id', 'enabled' => TRUE));
+    $attribute = new Attribute(array(
+      'class' => array('example-class'),
+      'id' => 'example-id',
+      'enabled' => TRUE,
+    ));
 
     $content = $this->randomMachineName();
     $html = '<div' . (string) $attribute . '>' . $content . '</div>';
     $this->assertClass('example-class', $html);
     $this->assertNoClass('example-class2', $html);
 
-    $this->assertID('example-id', $html);
-    $this->assertNoID('example-id2', $html);
+    $this->assertId('example-id', $html);
+    $this->assertNoId('example-id2', $html);
 
     $this->assertTrue(strpos($html, 'enabled') !== FALSE);
   }
 
   /**
+   * Tests attribute values.
+   *
    * @covers ::createAttributeValue
+   *
    * @dataProvider providerTestAttributeValues
    */
   public function testAttributeValues(array $attributes, $expected) {
     $this->assertEquals($expected, (new Attribute($attributes))->__toString());
   }
 
+  /**
+   * Provides test data for testAttributeValues.
+   *
+   * @return array
+   *   An array of test data.
+   */
   public function providerTestAttributeValues() {
     $data = [];
 
     $string = '"> <script>alert(123)</script>"';
-    $data['safe-object-xss1'] = [['title' => Markup::create($string)], ' title="&quot;&gt; alert(123)&quot;"'];
+    $data['safe-object-xss1'] = [['title' => TestMarkup::create($string)], ' title="&quot;&gt; alert(123)&quot;"'];
     $data['non-safe-object-xss1'] = [['title' => $string], ' title="' . Html::escape($string) . '"'];
     $string = '&quot;><script>alert(123)</script>';
-    $data['safe-object-xss2'] = [['title' => Markup::create($string)], ' title="&quot;&gt;alert(123)"'];
+    $data['safe-object-xss2'] = [['title' => TestMarkup::create($string)], ' title="&quot;&gt;alert(123)"'];
     $data['non-safe-object-xss2'] = [['title' => $string], ' title="' . Html::escape($string) . '"'];
 
     return $data;
@@ -381,7 +417,7 @@ public function providerTestAttributeValues() {

@@ -381,7 +417,7 @@ public function providerTestAttributeValues() {
    */
   protected function assertClass($class, $html) {
     $xpath = "//*[@class='$class']";
-    self::assertTrue((bool) $this->getXPathResultCount($xpath, $html));
+    self::assertTrue((bool) $this->getXpathResultCount($xpath, $html));
   }
 
   /**
@@ -394,7 +430,7 @@ protected function assertClass($class, $html) {

@@ -394,7 +430,7 @@ protected function assertClass($class, $html) {
    */
   protected function assertNoClass($class, $html) {
     $xpath = "//*[@class='$class']";
-    self::assertFalse((bool) $this->getXPathResultCount($xpath, $html));
+    self::assertFalse((bool) $this->getXpathResultCount($xpath, $html));
   }
 
   /**
@@ -405,9 +441,9 @@ protected function assertNoClass($class, $html) {

@@ -405,9 +441,9 @@ protected function assertNoClass($class, $html) {
    * @param string $html
    *   The HTML snippet to check.
    */
-  protected function assertID($id, $html) {
+  protected function assertId($id, $html) {
     $xpath = "//*[@id='$id']";
-    self::assertTrue((bool) $this->getXPathResultCount($xpath, $html));
+    self::assertTrue((bool) $this->getXpathResultCount($xpath, $html));
   }
 
   /**
@@ -418,9 +454,9 @@ protected function assertID($id, $html) {

@@ -418,9 +454,9 @@ protected function assertID($id, $html) {
    * @param string $html
    *   The HTML snippet to check.
    */
-  protected function assertNoID($id, $html) {
+  protected function assertNoId($id, $html) {
     $xpath = "//*[@id='$id']";
-    self::assertFalse((bool) $this->getXPathResultCount($xpath, $html));
+    self::assertFalse((bool) $this->getXpathResultCount($xpath, $html));
   }
 
   /**
@@ -434,7 +470,7 @@ protected function assertNoID($id, $html) {

@@ -434,7 +470,7 @@ protected function assertNoID($id, $html) {
    * @return int
    *   The number of results that are found.
    */
-  protected function getXPathResultCount($query, $html) {
+  protected function getXpathResultCount($query, $html) {
     $document = new \DOMDocument();
     $document->loadHTML($html);
     $xpath = new \DOMXPath($document);
@@ -452,3 +488,11 @@ public function testStorage() {

@@ -452,3 +488,11 @@ public function testStorage() {
   }
 

None of those changes are needed. Do you mind putting them into its own issue? By doing so, these test changes can land more quickly.

Needs a re-roll.

We'll also need a follow-up to make sure there's a subtree split for this component.

1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -2,344 +2,12 @@
   + * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
   
   +++ b/core/lib/Drupal/Core/Template/AttributeArray.php
   @@ -2,100 +2,12 @@
   + * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
   
   +++ b/core/lib/Drupal/Core/Template/AttributeBoolean.php
   @@ -2,42 +2,12 @@
   + * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
   
   +++ b/core/lib/Drupal/Core/Template/AttributeString.php
   @@ -2,30 +2,12 @@
   + * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
   
   +++ b/core/lib/Drupal/Core/Template/AttributeValueBase.php
   @@ -1,70 +1,13 @@
   + * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
   

   Update the deprecation notice for 8.6.0.

   Add @see for change notice.

   Add a follow-up to add @trigger_error().

2. +++ b/core/tests/Drupal/Tests/Component/Attribute/AttributeTest.php
   @@ -1,18 +1,23 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\Component\Attribute\AttributeTest.
   + */
   

   We don't use @file in tests any more.

3. +++ b/core/tests/Drupal/Tests/Component/Attribute/AttributeTest.php
   @@ -184,20 +192,30 @@ public function testAddClasses() {
   +      array('banana', 'aa', 'xx', 'yy', 'red', 'green', 'blue'),
   

   Use []. There are other places where we have this usage.

4. +++ b/core/tests/Drupal/Tests/Component/Attribute/AttributeTest.php
   @@ -1,18 +1,23 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\Component\Attribute\AttributeTest.
   + */
   

   We don't use @file any more in tests.

So basically: As per #37, do the file moves, add the @deprecation notices, run phpcbf, and that's the patch. Then we can do other cleanups in follow-ups.

re-rolled

Adjusted patch by following #42's comment (1-3 but without adding @see)
Also adjusted composer.json in keeping with https://www.drupal.org/node/2769841

Updated patch with interdiff added.

seems like testTwigAddRemoveClasses() had unexpressed dependencies in it.

$twig = new \Twig_Environment($loader); is assuming that everything in the Drupal\Core\Template namespace is available, including Twig_Environment.

It seems to me like that test is just misplaced in the attribute class, and could live in core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php and could reference the attribute component.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -2,358 +2,12 @@
+ * @deprecated in Drupal 8.8.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Component\Attribute\Attribute.

+++ b/core/lib/Drupal/Core/Template/AttributeArray.php
@@ -2,100 +2,12 @@
+ * @deprecated in Drupal 8.8.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Component\Attribute\AttributeArray.

+++ b/core/lib/Drupal/Core/Template/AttributeBoolean.php
@@ -2,42 +2,12 @@
+ * @deprecated in Drupal 8.8.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Component\Attribute\AttributeBoolean.

+++ b/core/lib/Drupal/Core/Template/AttributeString.php
@@ -2,30 +2,12 @@
+ * @deprecated in Drupal 8.8.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Component\Attribute\AttributeString.

+++ b/core/lib/Drupal/Core/Template/AttributeValueBase.php
@@ -2,70 +2,12 @@
+ * @deprecated in Drupal 8.8.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Component\Attribute\AttributeValueBase.

I know I said we should follow-up to add @trigger_error() in #42, but let's just add it here. So that means adding @trigger_error() like in the docs: https://www.drupal.org/core/deprecation#how-class

Yes, we have to have a CR with a deprecation.

@aleksip yes, no deprecated usages should be found

I wonder how widespread useage of Drupal\Core\Template\Attribute is in contrib. I.e. how much of an impact is it going to be to update their modules. (some maintainers are complaining about the increased frequency of deprecations as we're getting closer to 8.8.0)

@aleksip there's unofficial but working tool http://grep.xnddx.ru/

On the topic of determining impact in the contrib space, some of the D9 readiness tools might help here.

With this patch in place it should be possible to run drupal-check for deprecations and find references to Attribute in the results. If a few of us ran this on some relevant projects locally, I think we could come up with a decent representation of the most common projects that use it. We could even submit some issues proactively to notify the projects and/or provide patches.

Regarding deprecations, this seems like the time to do it rather than wait for Drupal 10?

Here are the usages mentioned above. Note that many of these are not the use statement that would require updating, so less than 484 actual edits. Many of them are @see comments.

There are also some bugs in that search. For instance, it returns core file module files for the portfolio_theme project...

All this to say I think we should move forward with this, as module maintainers are going to need to do some work for Drupal 9 whether this goes in or not, and this seems like a pretty minor impact.

@aleksip thanks for your work on this. I'm thrilled with this change, and agree that it will be a valuable component to give back. We have a project that will use it immediately. Also, I agree that now is the time to do it and also that the impact will be minimal on contrib. @brianperry's point that getting the patch to land so that it will get reported by drupal-check seems key as well.

I reviewed the code to get my head wrapped around the changes, and it looks good to me. What is needed to get this to RTBC?

Marking this as RTBC after following the patch review instructions. The patch applied cleanly against 8.8.x, commit 39bbdf40ac3599bf61db6ba392dafb9e3fdb11ef. Local testing of the site before and after the patch worked flawlessly.

I agree that this move is probably beneficial. I think it might be a little late to try to deprecate the old classes for removal in D9.

Statistics on contrib use are one thing, but the attribute system is a primary way for preprocess functions to interact with templates, and as such is like to be extensively used in custom themes. I spot checked several of the projects I maintain, and each one of them reference this class in the custom theme's .theme file. While the required update is minor, it's very possible that it affects almost all medium to complex custom themes out there, which could be a very large number of sites. Likewise (and again, just my thoughts, with no real empirical evidence) I would think a common scenario would be a website with no real custom modules or code, but with a custom theme using this class. Such a site owner might not be looking at tests, and might think that as soon as their custom modules are D9 ready, then they can safely update.

We have already started discussing requiring all deprecations from here on out be scheduled for removal in Drupal 10 instead of 9, and I think this one would be particularly disruptive given the fact that no contrib will be able to update for it until Drupal 8.7 support drops, and the unknown usages in Custom themes (and again, I fully support the idea).

I think at minimum at this point we should change the scheduled removal to Drupal 10, and we should seriously consider postponing until the D9 branch opens so that we keep the current namespaces with no triggered errors in 8.9LTS

That all being said, I'm not a Release Manager, so I will leave RTBC, but I am tagging for RM review to make a final decision.

We could deprecate now (in 8.8.0) for Drupal 10 without making a @trigger_error(), with a follow-up targeting D9.0.x to add the @trigger_error().

That way we get both the component now and the deprecation path later.

Hey look what just got filed: #3073936: [policy, no patch] Policy for Drupal 10 deprecations

Right, I dont think anything prevents this from getting into 8.8.0, except just deciding whether its deprecated for 10 vs 9, and if 10, how.

re #81 funny you should say that. We were just discussing how to handle something like that, in a general way:

https://www.drupal.org/project/drupal/issues/3073936

Regardless of when it is decided that this will land, I'd be happy to help with patches to contrib modules if dividing and conquering would be useful.

I can see the benefit of people being able to use this outside Drupal.
However, I don't see the benefit in deprecating the core classes.
If we go down that route, the patch is missing deprecation tests (tests that ensure the deprecation errors are triggered).
But I don't see the point - we can keep the classes in core that extend, they're a useful extension point if we need to provide Drupal-centric implementations again at some point. E.g. we have a PluginBase in component with a Drupal-centric one in core. I can't see any discussion above about why we added the deprecation - am I missing something?

This is good work, but I think we should work out if we need the deprecation at all, I'd vote we don't

Needs work for some nits, and either removing the deprecation or adding deprecation tests.

1. +++ b/core/includes/common.inc
   @@ -205,10 +205,10 @@ function valid_email_address($mail) {
   + *   value within a $attributes array passed to Drupal\Component\Attribute\Attribute,
   

   phpcs issue here: > 80

2. +++ b/core/lib/Drupal/Core/Render/Element/Date.php
   @@ -81,7 +81,7 @@ public static function processDate(&$element, FormStateInterface $form_state, &$
   +   *   initializing Drupal\Component\Attribute\Attribute with all the attributes.
   
   +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -70,7 +70,7 @@ public function getInfo() {
   +   *     tag. The attributes are escaped, see \Drupal\Component\Attribute\Attribute.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -44,7 +44,7 @@ public function getInfo() {
   +   *   currently done by initializing Drupal\Component\Attribute\Attribute with all
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -68,7 +68,7 @@ public static function processRadios(&$element, FormStateInterface $form_state,
   +          // The key is sanitized in Drupal\Component\Attribute\Attribute during output
   

   same here

3. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -46,7 +46,7 @@ public function __construct() {
   +      'Drupal\Component\Attribute\Attribute',
   

   while we're touching this, let's use the class constant

Another reason against deprecation - it would be terrible to deprecate it and make people do all the work to remove it and then find we need an extension point and have to add it back

Re: #88: It would be our component, too, so we could modify it there.

Well, I think he meant if we needed an extension with some non component dependency

Well, I think he meant if we needed an extension with some non component dependency

Yeah, this is what I meant, sorry should have been clearer

Right so the plan would be to have non-deprecated empty subclasses that extend the component classes, to reserve a class namespace in core in case we need to inject services or something?

I'm plus one on this approach, but we also need a release manager review, I will ping them

1. +++ b/core/lib/Drupal/Component/Attribute/composer.json
   @@ -0,0 +1,17 @@
   +    "name": "drupal/core-attribute",
   

   do we need an entry for this in core/composer.json too?

2. +++ b/core/tests/Drupal/Tests/Component/Attribute/AttributeTest.php
   @@ -1,21 +1,25 @@
   +/**
   + * Contains \Drupal\Tests\Component\Attribute\AttributeTest.
   + */
   

   we don't do this anymore

I agree with not deprecating the core copy prior to D9; it's constructed directly all over theme-layer code and would be a big disruption as deprecations go. I do think we should file a followup to decide whether to retain or deprecate the core wrapper. If we don't find a reason to have Drupal-specific implementation details, we should still consider deprecating it before D10. The docblock for the core wrapper could also link this followup in a @todo.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -2,358 +2,11 @@
+ * @see https://www.drupal.org/node/3070485

This was anappropriate documentation addition when we were deprecating the core component, but I think we should actually put the explanation of why we have this core subclass in the docblock. We don't normally substitute change records for API documentation.

Thanks everyone!

@aleksip it looks good!
After this patch becomes part of core. would it make #2623960: Improve the merge() method on Attribute objects easier to implement?

I don't have much to contribute other than from the testing/CI site of things, and ensuring that it gets subtree split out properly when it goes in. Seems to me that now that this does not introduce any new deprecations, that there aren't any real barriers to making this a component. That being said, I personally unfamiliar with Twig, or how this component would be used, so Im not a great resource to move it forward, but Im definitely +1 on making functionality like this available to the wider community.

Looks like the followup was filed as well, so Im removing the @deprecated and Needs Followup tags.

Since this is not altering any functionality, I suppose my lack of Twig knowledge shouldnt matter. I'll go ahead and RTBC this because it still has the "Needs release manager review" tag, and they generally focus on RTBC stuff first, IIRC.

Looks like xjm has already properly reviewed this (note, I haven't yet) and feedback was addressed, removing the tag.

One small nit/request that has bugged me for years:

Rename the wrapper class Attribute to Attributes (plural) since it is really a collection of attributes, not a singular standalone attribute.

Having recently started a small personal SF4.3 project that requires a few templates, I now realize that I am quite strongly in favor of this, much more than I had realized, and am even wondering if there would be interest in packaging attributes up and getting it included as part of symfony/twig...

Thanks for the updated patch! Just another bit we need to do for the docs:

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -2,358 +2,12 @@
  * Collects, sanitizes, and renders HTML attributes.
  *
- * To use, optionally pass in an associative array of defined attributes, or
- * add attributes using array syntax. For example:
- * @code
- *  $attributes = new Attribute(array('id' => 'socks'));
- *  $attributes['class'] = array('black-cat', 'white-cat');
- *  $attributes['class'][] = 'black-white-cat';
- *  echo '<cat' . $attributes . '>';
- *  // Produces <cat id="socks" class="black-cat white-cat black-white-cat">
- * @endcode
- *
- * $attributes always prints out all the attributes. For example:
- * @code
- *  $attributes = new Attribute(array('id' => 'socks'));
- *  $attributes['class'] = array('black-cat', 'white-cat');
- *  $attributes['class'][] = 'black-white-cat';
- *  echo '<cat class="cat ' . $attributes['class'] . '"' . $attributes . '>';
- *  // Produces <cat class="cat black-cat white-cat black-white-cat" id="socks" class="cat black-cat white-cat black-white-cat">
- * @endcode
- *
- * When printing out individual attributes to customize them within a Twig
- * template, use the "without" filter to prevent attributes that have already
- * been printed from being printed again. For example:
- * @code
- *  <cat class="{{ attributes.class }} my-custom-class"{{ attributes|without('class') }}>
- *  {# Produces <cat class="cat black-cat white-cat black-white-cat my-custom-class" id="socks"> #}
- * @endcode
- *
- * The attribute keys and values are automatically escaped for output with
- * Html::escape(). No protocol filtering is applied, so when using user-entered
- * input as a value for an attribute that expects an URI (href, src, ...),
- * UrlHelper::stripDangerousProtocols() should be used to ensure dangerous
- * protocols (such as 'javascript:') are removed. For example:
- * @code
- *  $path = 'javascript:alert("xss");';
- *  $path = UrlHelper::stripDangerousProtocols($path);
- *  $attributes = new Attribute(array('href' => $path));
- *  echo '<a' . $attributes . '>';
- *  // Produces <a href="alert(&quot;xss&quot;);">
- * @endcode
- *
- * The attribute values are considered plain text and are treated as such. If a
- * safe HTML string is detected, it is converted to plain text with
- * PlainTextOutput::renderFromHtml() before being escaped. For example:
- * @code
- *   $value = t('Highlight the @tag tag', ['@tag' => '<em>']);
- *   $attributes = new Attribute(['value' => $value]);
- *   echo '<input' . $attributes . '>';
- *   // Produces <input value="Highlight the &lt;em&gt; tag">
- * @endcode
- *
- * @see \Drupal\Component\Utility\Html::escape()
- * @see \Drupal\Component\Render\PlainTextOutput::renderFromHtml()
- * @see \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols()
+ * @todo Consider deprecating this class in
+ *   https://www.drupal.org/project/drupal/issues/3080648.
  */
-class Attribute implements \ArrayAccess, \IteratorAggregate, MarkupInterface {

We still need documentation explaining what it is, why it exists, etc. This doesn't pass the core documentation gate: https://www.drupal.org/core/gates#documentation

Normally we'd use {@inheritdoc} here, but that doesn't work when we have to add additional information. (Ref: #1994890: Allow {@inheritdoc} and additional documentation) And in this case I think we need, at a minimum, explaining why this exists and how it is different from the Drupal\Component version. We can then add "See Drupal\Component\Attribute\Whatever for full documentation of this class." or such.

I'm not super thrilled about the notion of making the new component Attributes instead of Attribute; that seems like an upgrade path WTF and annoyance waiting to happen.

An interdiff would help me evaluate the changes and re-RTBC quickly. I'd make one, but I won't be able to until probably tomorrow.

fwiw I agree with duplicating and not deprecating for the moment.

I am not 100% convinced about leaving the Drupal\Core version in permanently 'just in case', we might never need to change it. So it seems worth opening a follow-up, against 9.x, to consider deprecating it for 10.x (or even later).

There are some core issues which may involve changes to the Attribute class such as #2567743: Add protocol filtering to the core Attribute component, #2531824: Attribute class to check safe strings before escaping (has tests), and a whole range of related issues like #2544110: XSS attribute filtering is inconsistent and strips valid attributes and others. These could (probably) be done in the Component, and not all of the issues touch Attribute itself anyway, but worth being aware of.

If we're not deprecating, we need some way to encourage people who are working with Drupal core not to use the Component version of the class, in case we ever do make a change to the Core version, because then they wouldn't get the bug fix. Should this just be docs in the Component referencing the Core version? Could we add a DrupalCS rule that looks for the use statement and tells you to use core instead?

Thanks for that follow-up and apologies for missing it had already been created - following the issue now.

This is comment is similar to catch's (comes about after a discussion) but with a slightly different pov and a suggestion about how to resolve it.

One thing that confuses me about this change is that once it goes in what are we expecting new code to use? The component version - or the core version? Atm they are functionally equivalent but #87 down suggest that we might need to make changes to the core versions due to things like:

• #2544110: XSS attribute filtering is inconsistent and strips valid attributes
• #2944173: CKEditor mangles tokens in URLs, due to bug in Xss::attributes()
• #2567743: Add protocol filtering to the core Attribute component
• #2531824: Attribute class to check safe strings before escaping (has tests)

I've reviewed all of those and fortunately I think all of these can be fixed in the component version.

I've been pondering if an alternative would be to use class_alias() to alias the new component classes to the old core namespace. That way we wouldn't have additional classes lying around and we wouldn't have additional class inheritance. The question would then come how can we deprecate usage of the core namespace in future. For that I have two thoughts:
1. We just don't have too - a class_alias() can remain and it doesn't matter.
2. If we want to we could use static analysis ie PHPCS or PHPStan to do the deprecation.

So where would we add the class_alias() code? For me the best place to do this is in the autoloader. We can add a files section to core/composer.json - something like

                "files": [
                    "aliases.php"
                ]

And add core/aliases.php with something like

/**
 * @file
 * Contains class aliases needed for Drupal core.
 */

class_alias('\Drupal\Component\Attribute\Attribute', 'Drupal\Core\Template\Attribute');

In order to get composer to rebuild the autoloader and .lock file correctly you'll need to composer update drupal/core from the cli. And then we can remove all the core classes in favour of the component classes.

I see three potential future scenarios:

1. We maintain two identical copies of the classes forever, one in core, and one in component. We don't care which one people use.

   This seems a bit sloppy, but is the most compelling argument for using class_alias. Given than this is a class that is highly likely to be used in custom themes, which is a large majority of sites, I would assume, and unlikely to have the same type of testing that catches deprecation errors, it might be worth avoiding the disruption of moving this class.

2. We have a core extension to the component classes, which implements some additional drupal specific features with dependency requirements that prevent the changes from being made in component.

   For this, obviously we can't use class_alias, although we could possibly go the class_alias route until such time as this potential situation actually occurs. The problem there is that if we say we don't care which one people use and then add something to core, then we are in trouble, since some people were using component. So I would say in this case it would have been better to start with separate class files that could have separate documentation pointing people to use core from åœw

3. We deprecate the core classes in Drupal 9.

   In this case, we could do it with class_alias and some static analysis or classloader trickery, but it seems far easier to just do the extension and
   document the docblock and trigger an error like normal.

@aleksip PHPStorm supports class_aliases - we them already in the testing system.

Looks like the current feedback has been addressed, so I'll RTBC

Here's another approach to the class "copying" that doesn't rely on inheritance and ensure that the core and component classes stay the same without any effort because there are no core classes anymore. Also when we want to deprecate we can add @trigger_error to the core files and be done.

I think we should only consider deprecating the core attribute classes in Drupal 9 for Drupal 10.

There's no magic component composer.json updating

+++ b/core/lib/Drupal/Component/Attribute/composer.json
@@ -5,9 +5,9 @@
-        "php": ">=5.5.9",
-        "drupal/core-render": "^8.8",
-        "drupal/core-utility": "^8.8"
+        "php": ">=7.0.8",
+        "drupal/core-render": "^8.9",
+        "drupal/core-utility": "^8.9"

I think we need to target drupal 9 first. So "php": ">=7.2.3". One thing that is interesting is the other components all have drupal stuff at ^8.8.

@aleksip let's focus on D9 for now as this is a new component and I agree that in ^9.0 make sense in that case. We should open an issue to bump the rest of the components.

I don't think there is any benefit in this landing in 8.9 as we're not going to deprecate the core classes in Drupal 8 anyway - so this would only happen in D9 and probably after sometime.

In #3090145-19: Ensure that mixing array and Attribute objects in theme rendering is managed properly @mondrake correctly points out that Attribute is badly named. It should be something like AttributeCollection or Attributes. Using a better name for the component version should be in-scope here.

More of a task than a feature, really.

Rerolled and moved to MR workflow.

Attempted renaming as per #138.

Rebased on 9.3.x

Added typehints to new code where possible, and deprecation errors to the legacy code.

I think deprecating here is out of scope? We can still move to the core classes extending component (with no additional code), but #87 is a core committer giving a -1 to deprecation and #3080648: Consider deprecating core Attribute classes exists for further discussion of this.

One additional comment about dropping the single use of random in a test, I don't see why we need that.

Fixed the use of random.

Re #148 and #87 I learned we do not leave stuff in if it's only potential use. We implement when the need comes. So here keeping the deprecations and adding a silencer.

IMHO, either we move and deprecate, or we leave stuff as is and won't fix this.

Other than the deprecation decision this is ready to go, marking RTBC to get another round of core committer feedback. Noting that @catch was also -1 to deprecating in #116 but then @alexpott had other thoughts in #120 that I'm not sure we've fully explored, let's see what they think now.

Still something to do. Apart from fails, deprecation tests.

Apart from the changes needed to composer.lock, this is for review now.

@mondrake to fix the composer issue just run composer update --lock from the top level directory and commit the change which should be

diff --git a/composer.lock b/composer.lock
index 73e59bab83..e8166bd3c4 100644
--- a/composer.lock
+++ b/composer.lock
@@ -534,7 +534,7 @@
             "dist": {
                 "type": "path",
                 "url": "core",
-                "reference": "538af067d3843f8b7394e98991fcec7c2d45213d"
+                "reference": "ed274f30a52f737954af30c846c39014ef911668"
             },
             "require": {
                 "asm89/stack-cors": "^1.1",
@@ -609,6 +609,7 @@
                 "drupal/contextual": "self.version",
                 "drupal/core-annotation": "self.version",
                 "drupal/core-assertion": "self.version",
+                "drupal/core-attribute": "self.version",
                 "drupal/core-bridge": "self.version",
                 "drupal/core-class-finder": "self.version",
                 "drupal/core-datetime": "self.version",

Thanks @longwave! One day I will learn this :)

Two questions about backward compatibility, and still the problem of whether we deprecate or not to answer, but core committers can have the final say on that one.

For signature typehint BC, I think we must come to a decision on how to add typehints to existing code, #3050720: [Meta] Implement strict typing in existing code. An example could be #3217961: Prepare for Connection::select() signature to be fully typehinted in D10.

Fixed part of #155.

Thanks - but do we need a deprecation test for that Settings change?

Added deprecation tests for:

• deprecation of the abstract class AttributeBaseValue (tricky because we need to avoid autoloading of the class before the test starts)
• deprecation of the usage of the Attribute class in $settings['twig_sandbox_allowed_classes'] (extremely tricky because this was read in a constructor, and with PHPUnit mocking this is a disaster, had to refactor some parts of TwigSandboxPolicy to allow mocking Settings::get)

Also, deprecation of the usage of the Attribute class in $settings['twig_sandbox_allowed_classes'] is probably not enough, that's OK for developers but sites may have this set in their settings.php, so we probably need a warning in the status report on top? Maybe I see now reasons for reluctance in deprecating these classes.

Rerolled.

Rerolled.

I guess this is D10 material now, where in any case return typehints would have to be solved for classes in HEAD.

Rerolled.

rerolled

Testbot not happy.

The deprecation window for D9.3 has passed.

Applied the changes that should be needed to target 10.x

CI failed somehow

This is an issue where #3180042: Add baseline for deprecation testing would help a lot.

Rebased onto 10.1.x, will work on renaming classes to HtmlAttribute* as suggested in #3252386-148: Use PHP attributes instead of doctrine annotations.

This patch is raising the new component's PHPStan compliance level to 9. This requires excluding PHPCS checks on it because they are incompatible to most PHPStan L6+ fixes. Keeping the patch separate from the MR since I am aware this is too far fatching ATM.

Spell check fixes.

Sorry.

Some improvements in scope/visibility of properties and constructors, driven by PHPStan.

Apparently, using phpstan prefixed annotations for params and returns, we can get code compliant with PHPStan-9 AND pass current Drupal PHPCS checks.

So I did that in the MR.

rebased

This issue is already compatible with #3344303: [policy, no patch] Decide on a namespace for PHP Attribute classes but just noting the decision over here.

Removing the needs follow-up tag since that already exists per #100.

Wasn't entirely sure how to best test this one.

I applied the MR and did some basic features like creating content, media, using the workflow, etc.

Nothing seemed to break but if there's a more targeted way let me know!

+1 from me but with a large feature maybe should get a few more (and real testing)

I think the issue here is that we need some specialist information, such as whether the infrastructure is set up to add another packagist listing and whether the subtree split will work.

Back in the day, @Mixologic had that set up to happen smoothly, but since he's not at the DA any more, it unclear who to ask. Or at least I don't know who. :-)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Maybe after commit of #3358739: Update coder to 8.3.18 we can now go back to use standard @param and @return annotations.

Think this would be good to get in early for 10.2

Tried rebasing to see if the tests would run but seems to be an issue.