The Drupal\Core\Template\Attribute class and its helper classes have no Drupal dependencies apart from Drupal Components. There already exists at least one project where Attribute classes are used independently of Drupal, and it would be good if they could be used as a proper Composer package.

Files: 

Comments

aleksip created an issue. See original summary.

aleksip’s picture

I'd be happy to create a patch if this is considered a viable idea.

Cottser’s picture

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.

aleksip’s picture

Here is a patch with wrapper classes.

I found 31 occurrences in core where Attribute is used. Should they be changed too, or are the wrapper classes containing a deprecation notice sufficient?

Cottser’s picture

Status: Active » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 4: move_attribute_classes-2664570-4.patch, failed testing.

dawehner’s picture

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

Cottser’s picture

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’s picture

Status: Needs work » Needs review
FileSize
87.49 KB
35.24 KB

Thanks for the comments! While waiting for #2579691: Remove usages of SafeMarkup::isSafe() here is an updated patch. Will reroll when that issue lands.

Status: Needs review » Needs work

The last submitted patch, 9: move_attribute_classes-2664570-9.patch, failed testing.

aleksip’s picture

Any suggestions on what should be done about \Drupal\Core\Render\Markup in AttributeTest, which causes testNoCoreInComponentTests to fail?

dawehner’s picture

@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;
}
aleksip’s picture

Status: Needs work » Needs review
FileSize
87.59 KB
1.89 KB

@dawehner Thanks!

Here is a new patch with a TestMarkup class.

aleksip’s picture

Change to psr-4 as per #2337283-58: Add a composer.json file to every component. Should this issue be committed first so the Attribute component can be reviewed there?

Cottser’s picture

Status: Needs review » Needs work

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

aleksip’s picture

Status: Needs work » Needs review
FileSize
87.56 KB

Here is the reroll.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Cottser’s picture

Status: Needs review » Needs work

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 :)
aleksip’s picture

New patch with requested changes. Also bumped composer.json requirements to ~8.2.

aleksip’s picture

Status: Needs work » Needs review
jhedstrom’s picture

This is looking good.

However, given the efforts over in #2572635: Fix 'Drupal.Commenting.DocComment' coding standard (and more generally #2571965: [meta] Fix coding standards in core, 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.

Cottser’s picture

Status: Needs review » Needs work

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!

aleksip’s picture

Status: Needs work » Needs review
FileSize
87.94 KB
11.74 KB

Fixed most PHPCS issues. Of the only remaining errors

Tags must be grouped together in a doc comment

is an error itself (#2502837: Multiple code snippets, todos, and links are allowed in file doc blocks) and

Missing parameter name

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

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

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.

Mile23’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Also all the @file blocks need to be removed.

kostyashupenko’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
88.49 KB

Reroll of #23

dawehner’s picture

Status: Needs review » Needs work

@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

kostyashupenko’s picture

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

kostyashupenko’s picture

New reroll of #23

kostyashupenko’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work

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

Eric_A’s picture

Component: theme system » base system
Status: Needs work » Needs review
FileSize
64.97 KB

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?

Status: Needs review » Needs work

The last submitted patch, 33: move_attribute_classes-2664570-33.patch, failed testing.

Eric_A’s picture

Status: Needs work » Needs review
FileSize
64.94 KB

Rerolled #33.

Eric_A’s picture

dawehner’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.