Move drupal_html_id() and drupal_html_class() to Drupal\Component\Utility

1. +++ b/core/lib/Drupal/Component/Utility/Markup.php
   @@ -0,0 +1,178 @@
   +
   +  /**
   +   * @var array
   +   */
   +  protected static $classes = array();
   +  protected static $seenIdsInit;
   +  protected static $seenIds;
   +
   

   Needs some docs ... is there any way to get rid of the statics, I fear not.

2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   index 4e3c0db..d61975b 100644
   --- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   
   --- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   
   +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -7,6 +7,7 @@
   
   @@ -7,6 +7,7 @@
    
    namespace Drupal\Tests\Core\Form {
    
   +use Drupal\Component\Utility\Markup;
    use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    use Drupal\Core\Form\FormBuilder;
    use Drupal\Core\Form\FormInterface;
   @@ -108,6 +109,8 @@ public function setUp() {
   
   @@ -108,6 +109,8 @@ public function setUp() {
        $this->account = $this->getMock('Drupal\Core\Session\AccountInterface');
        $this->formBuilder->setCurrentUser($this->account);
    
   +    // Clear out the seen IDs for Markup::htmlId().
   +    Markup::resetSeenIds();
      }
    
   

   Is there a reason this is needed in here?

3. +++ b/core/tests/Drupal/Tests/Core/Utility/HtmlIdentifierTest.php
   @@ -0,0 +1,80 @@
   +  public function testDrupalCleanCSSIdentifier() {
   ...
   +  public function testDrupalHTMLId() {
   

   We could and should use data providers here.

1. +++ b/core/tests/Drupal/Tests/Core/Utility/HtmlIdentifierTest.php
   @@ -0,0 +1,122 @@
   +class HtmlIdentifierTest extends UnitTestCase {
   

   shouldnt this be just HtmlTest.
   Also an @group Drupal :)

2. +++ b/core/tests/Drupal/Tests/Core/Utility/HtmlIdentifierTest.php
   @@ -0,0 +1,122 @@
   +   * Tests that drupal_html_class() cleans the class name properly.
   

   shouldnt we say Html::getClass here? :)

3. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -0,0 +1,193 @@
   +      if (empty($_POST['ajax_html_ids'])) {
   

   this is a red flag for a component..Maybe move this to the DIC and make it request aware

this is a red flag for a component..Maybe move this to the DIC and make it request aware

The dx of that would though dramatically degresse. I wonder whether we can have an event listener which sets the current active ajax html IDs onto a static property

Here is a current rerole after some test additions that landed.

In case we want to have the service, we might should just move the html IDs into a service but keep the other function in a component?

The dx of that would though dramatically degresse. I wonder whether we can have an event listener which sets the current active ajax html IDs onto a static property

This sounds pretty good i guess. Add a public static function setAjaxHtmlIds(array $ids) then call it from an event subscriber:)
Which means the component knows nothing of the Request

I would like to work on this. I accidentally created dublicate issue, where you can see some of my ideas. https://drupal.org/node/2167261

Some thoughts:

1. dawehner suggested to create a service (at least for ids), should we?
2. I agree that the basic identifier cleanup could be done by a component, but should it also be a service? It would allow to replace the identifier syntax.
3. For simplicity I would argue that we need only one service (e.g. html.identifier) with two methods (e.g. validate and validateUnique). See my reasons below. If the ajax stuff is too tightly coupled with Drupal it would make this a core service.

First, I think the following function calls would be easy to read and understand. They are also abstract enough to use in different situations and not restricted to only on classes and ids. I'm not sure if the word validate can be used here though. It could be seen as just checking if the string is valid. What would be a better word?

\Drupal::service('html.identifier')->validate($string)
\Drupal::service('html.identifier')->validateUnique($string)

I see no reason to not cache already validated identifiers. So we can combine drupal_html_class and drupal_clean_css_identifier. At the moment drupal_html_id dublicates code from drupal_clean_css_identifier. My plan would be to call validate() inside validateUnique().

After fast review of the provided patches I could also go for this naming convention.

\Drupal::service('html.identifier')->get($string)
\Drupal::service('html.identifier')->getUnique($string)

@jiv_e: CSS classes and HTML IDs are not the same. Let's not mix them (if I remember correctly, we used to in previous versions of Drupal).

Also, it's not about validating (which imply a true / false result), but about (1) generating for HTML ids, (2) cleaning up for CSS identifiers.

If we want to have both CSS and IDs in the same component (I could go either way), let's just keep the method names that @dawehner had in #7.

After considering what Damien said I also think that we should keep the ID and class in the function names e.g. like this.

\Drupal::service('html')->getId($string)
\Drupal::service('html')->getClass($string)

I'm not sure if 'html' as service name is too generic. It could be misleading.

But I'd argue that cleanCssIdentifier could be private method and called by both public methods above to avoid code duplication.

At the moment drupal_clean_css_identifier is called 23 times in core excluding tests. The frequencies seem to be:
Getting ID: 2
Getting class: 20
Getting views template path: 1

After thinking about it I can't see any other correct use case for this function than getting class or ID. Also getting ID with this function seems wrong, because the uniqueness is not ensured.

@jiv_e: HTML5 dropped all the requirements on IDs, so it makes sense to use the same requirements as CSS class names so that we guarantee that there is no need to escape characters in CSS and Javascript selectors. So ok to make cleanCssIdentifier protected and have both getId and getClass leverage it.

I wonder if we shouldn't drop the caching in getClass, or at least move it to cleanCssIdentifier and move the drupal_strtolower there too.

Rerolled the patch.

Wrong patch file.

This fixes it.

Last patch was outdated so I recreated it. Basically the tests were changed a little bit. As we went through them with @ohthehugemanatee. They seemed friendly.

I had some issues with interdiff though. I got this error:

1 out of 3 hunks FAILED -- saving rejects to file /var/folders/89/2835sf6d5ks41zf57m867fsw0000gn/T//interdiff-1.QQvB64.rej
interdiff: Error applying patch1 to reconstructed file

The mentioned rejection file contained this:

***************
*** 286,318 ****
    /**
     * {@inheritdoc}
     */
-   protected function drupalHtmlClass($class) {
-     return $class;
-   }
- 
-   /**
-    * {@inheritdoc}
-    */
-   protected function drupalHtmlId($id) {
-     if (isset(static::$seenIds[$id])) {
-       $id = $id . '--' . ++static::$seenIds[$id];
-     }
-     else {
-       static::$seenIds[$id] = 1;
-     }
-     return $id;
-   }
- 
-   /**
-    * {@inheritdoc}
-    */
-   public function drupalStaticReset($name = NULL) {
-     static::$seenIds = array();
-   }
- 
-   /**
-    * {@inheritdoc}
-    */
    protected function &batchGet() {
      $batch = array();
      return $batch;
--- 287,292 ----
    /**
     * {@inheritdoc}
     */
    protected function &batchGet() {
      $batch = array();
      return $batch;

I think this won't matter because these functions are removed in both patches (28 and 29).

Another re-roll.

I finally managed to debug the tests with PHPStorm and Xdebug. I found out why there were two tests failing in the last patch. During the test run forms with same ids are created multiple times. For some reason those are cached and id-numbers added. Even if I think they shouldn't they don't represent the case of rendering multiple forms on the same page. I don't yet know why the behaviour is changed.

public function testBuildFormWithClassString() {
...
  $this->assertSame('test-form', $form['#id']);

Fails because
Expected: test-form
Actual: test-form--2

public function testUniqueHtmlId() {
...
  $this->assertSame('test-form-id', $form['#id']);

Fails because
Expected: test-form-id
Actual: test-form-id--3

Dawehner, have you intentionally replaced the more general cache clearing function with the more spesific one?

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockViewBuilderTest.php
   @@ -103,7 +104,7 @@ public function testBasicRendering() {
        // Reset the HTML IDs so that the next render is not affected.
   -    drupal_static_reset('drupal_html_id');
   +    Html::resetSeenIds();
   

   This seems to be ok even though I can't see how the cache name 'drupal_html_id' corresponds to static::$seenIds.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -555,7 +556,7 @@ public function processForm($form_id, &$form, &$form_state) {
   -        $this->drupalStaticReset('drupal_html_id');
   +        Html::resetSeenIds();
   

   Same here.

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormTestBase.php
   @@ -178,7 +179,7 @@ public function setUp() {
      protected function tearDown() {
   -    $this->formBuilder->drupalStaticReset();
   +    Html::resetSeenIds();
      }
   

   But here drupalStaticReset is meant to empty the whole static cache. How can this be replaced with only Html::resetSeenIds() ?

This seems to be ok even though if I can't see how the cache name 'drupal_html_id' corresponds to static::$seenIds.

Previously drupal_html_id() used __FUNCTION__ for its static cache key.

muh

meh.

Just nitpicks.

Cool, thank you for the review.

Thanks for taking this forward! I got too busy.

I was a little turned around with why we touched AjaxRequestSubscriber in this patch. Tim explained on IRC that a) there's only one Ajax subscriber, so just call it what it is, b) the current code for determining HTML IDs has to go through backflips to see what HTML IDs might've been used on an Ajax request, the new version of the code just announces it. Seems fair.

Committed and pushed to 8.x. Thanks!

Cleaning up drupalcampfi tags.