Move usages of drupal_html_id() to Html::getUniqueId()

Sadly, no longer applies.

Rerolling...

RTBC, looks great and will make #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests easier.

Clarifying title.

It helps to pull before rerolling!

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -217,7 +217,7 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
   -        $wrapper_id = drupal_html_id($id_prefix . '-add-more-wrapper');
   +        $wrapper_id = HTML::getUniqueId($id_prefix . '-add-more-wrapper');
   

   HTML -> Html

2. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
   @@ -69,7 +69,7 @@ public static function preRenderLink($element) {
   -        $element['#id'] = $element['#options']['attributes']['id'] = drupal_html_id('ajax-link');
   +        $element['#id'] = $element['#options']['attributes']['id'] = \Drupal\Component\Utility\Html::getUniqueId('ajax-link');
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -65,7 +65,7 @@ public static function processRadios(&$element, FormStateInterface $form_state,
   -          '#id' => drupal_html_id('edit-' . implode('-', $parents_for_id)),
   +          '#id' => \Drupal\Component\Utility\Html::getUniqueId('edit-' . implode('-', $parents_for_id)),
   
   +++ b/core/lib/Drupal/Core/Render/Element/Table.php
   @@ -131,9 +131,10 @@ public static function processTable(&$element, FormStateInterface $form_state, &
   -        $row['#id'] = drupal_html_id('edit-' . implode('-', $element_parents) . '-row');
   +        $row['#id'] = \Drupal\Component\Utility\Html::getUniqueId('edit-' . implode('-', $element_parents) . '-row');
   
   @@ -171,7 +172,7 @@ public static function processTable(&$element, FormStateInterface $form_state, &
   -            '#id' => drupal_html_id('edit-' . implode('-', $element_parents)),
   +            '#id' => \Drupal\Component\Utility\Html::getUniqueId('edit-' . implode('-', $element_parents)),
   
   +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -236,7 +236,7 @@ public static function processTableselect(&$element, FormStateInterface $form_st
   -              '#id' => drupal_html_id('edit-' . implode('-', $parents_for_id)),
   +              '#id' => \Drupal\Component\Utility\Html::getUniqueId('edit-' . implode('-', $parents_for_id)),
   

   Add 'use' at top of file, then just Html::

Did not watch carefully enough the first time.

Can you confirm there are no usages of drupal_html_id() left now?

Besides the function declaration, the only place drupal_html_id is found is in ViewPreviewForm::init():

    // Reset the cache of IDs. Drupal rather aggressively prevents ID
    // duplication but this causes it to remember IDs that are no longer even
    // being used.
    $seen_ids_init = &drupal_static('drupal_html_id:init');
    $seen_ids_init = array();

Yes, that is another bug, but was already present before this, so can be followup IMHO.

Or you could fix it here, should be:

Html::resetIds(); IIRC (or ::resetSeenIds())

Can you open a normal bug report for that?

RTBC

Done #2443847: Remove dead HTML ID-tracking code from ViewPreviewForm. And there are a lot of issues in the core queue referencing drupal_html_id that ought to be evaluated and probably closed.

How does this make #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests easier?

#31: Not that much, but easier to grep for Html::getUniqueId instead of that _and_ drupal_html_id.

I assumed it was converted already, so first got confused on that ...

Hmm OK. When I opened this I thought it'd make that issue harder, so I'd settle for 'not harder'.

No longer applies though.

Back to RTBC if tests pass.

Committed/pushed to 8.0.x, thanks!

Since the introduction of this fix the install screen is broken:

Fatal error: Cannot use Drupal\Component\Utility\Html as Html because the name is already in use in /vagrant_sites/drupal8.local/web/core/lib/Drupal/Core/Render/Element/Link.php on line 11

Will investigate further

Resetting HEAD to 11e02a67 makes the install screen return.

Tried building 8.0.x on simplytest.me and can recreate the issue

Fixes the imports.

I have reverted the commit of the patch in #35. It broke HEAD for anyone not using opcaches.

I think in the short term we should do the use \Drupal\Component\Utility\Html as UtilityHtml;

ircmaxell confirmed this is a bug in all versions of PHP.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_compile.c#7039

I'm guessing the testbots use opcache, because that's the only way they'd pass:

The reason it works with opcache is that opcache removes the symbol table at compile time. So there is never a conflict

ircmaxell said he'd file a PHP7 bug report tomorrow.

Just to clarify:

What happened with the patch that was committed is that it made at least one class in the Drupal/Core/Render/Element area, which has a class called Html already, fail. The Container render element class was given a line saying:

use Drupal\Component\Utility\Html;

And on an ordinary server without opcache, if you tried to install you'd get:

Fatal error: Cannot use Drupal\Component\Utility\Html as Html because the name is already in use in ..../core/lib/Drupal/Core/Render/Element/Container.php on line 10

Two possible fixes are possible:
a) Use aliases, so the use line would say something like "use ... as HtmlComponent".
b) Rename the render element Html class to something else so it doesn't conflict. This has ramifications though.

I ran a test of 8.0.x with the patch from #35 and saw the error message as reported above. Ran another test with the patch from #51 and Drupal installs as expected. I also applied the patch locally and did a full grep of the code base for drupal_html_id. I only saw one remaining case:

+++ b/core/modules/views_ui/src/ViewPreviewForm.php
@@ -21 @@ public function form(array $form, FormStateInterface $form_state)
    $seen_ids_init = &drupal_static('drupal_html_id:init');

I don't think this is relevant but wanted to point it out just in case. Marking RTBC.

@mrjmd We have an issue for that remaining thing. #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests

Nope, this one #2443847: Remove dead HTML ID-tracking code from ViewPreviewForm

If the committers want to credit those who spent time today fixing HEAD and tracking down the reason for the bug, you can add these people to the commit message:

neclimdul, ircmaxell, tim.plunkett, cilefen, nlisgo, Crell

Do we need to create a separate issue to address the fact that the testbot did not pick up on this?

@nlisgo: In general we would add tests directly to this issue so that this issue never crops up again. However, in this particular case it doesn't seem possible to write a test.

Argh sorry about this.

This might have been caught if we'd had #1867192: Testbots need to run on 5.4, 5.5, 5.6 and 7 - at least quickly after commit if not before.

Committed/pushed #52 (with the additional credits) to 8.0.x, thanks!

@tstoeckler: do you mean to say you do not feel it is possible to write a test as part of this issue or at all? I don't know enough about the testbot setup to comment about whether an adjustment could be made to the way the build is done to be sure that this type of issue does not re-occur. Is the build script for testbot available?

Testing on all PHP versions would have caught this - either implicitly because PHP 5.4 wouldn't be running opcache, or if we had added no opcode cache vs. APC vs. opcache as explicit configurations. Probably only after commit though.

If I'd known this would break HEAD I'd have marked it postponed on #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests since in the end this method will be barely used if at all.

Yeah, technically all versions of PHP would not have caught this because

PASSED: [[SimpleTest]]: [PHP 5.4 MySQL]

implies that testbots did test this on php 5.4. So despite not having opcache, they have another opcode cache that must be providing the same behaviour. ircmaxell mentioned doing a writeup today so once he does maybe we'll have a better view into the nitty gritties of the failure and have the details to build an explicit test to catch this.

Yes I think we'd want a PHP environment that explicitly doesn't have an opcode cache configured to ensure any issues like this get caught.

#2446719: Need test runners without bytecode caching

Here's the fix for PHP https://github.com/php/php-src/pull/1149