Follow-up to #2407361: Move usages of drupal_html_id() to Html::getUniqueId()

ViewPreviewForm contains dead code which was for tracking generated HTML IDs. Remove it:

    // 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();
CommentFileSizeAuthor
#9 viewpreviewform_should-2443847-9.patch786 bytescilefen
#8 viewpreviewform_should-2443847-8.patch959 bytescilefen
#5 viewpreviewform_should-2443847-5.patch827 bytescilefen
#5 interdiff-2-5.txt540 bytescilefen
#2 viewpreviewform_should-2443847-2.patch909 bytescilefen

Comments

cilefen’s picture

cilefen commented
Title: ViewPreviewForm should reset seen HTML IDs with Html::::resetSeenIds() » ViewPreviewForm should reset seen HTML IDs with Html::resetSeenIds()
cilefen’s picture

cilefen commented
Status: Active » Needs review
StatusFileSize
new viewpreviewform_should-2443847-2.patch909 bytes

I removed the inline comment also because the function doc for resetSeenIds is clear enough.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views_ui/src/ViewPreviewForm.php
@@ -25,11 +26,7 @@ public function form(array $form, FormStateInterface $form_state) {
-    // 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.

Why do we loose the documentation here?

cilefen’s picture

cilefen commented

@dawehner I said in #2 it seems redundant. Disagree? I don't feel strongly about it either way though.

cilefen’s picture

cilefen commented
StatusFileSize
new interdiff-2-5.txt540 bytes
new viewpreviewform_should-2443847-5.patch827 bytes

This is with the comment.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you for that!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Task » Bug report
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This is a bug because this code can't be working at this point. Any chance we can add a test for this?

cilefen’s picture

cilefen commented
Status: Needs work » Needs review
StatusFileSize
new viewpreviewform_should-2443847-8.patch959 bytes

I think there is no need to reset the seen IDs here because the ID of preview form wrapper is always views-preview-wrapper. This class never tries to generate an ID.

cilefen’s picture

cilefen commented
StatusFileSize
new viewpreviewform_should-2443847-9.patch786 bytes
dawehner’s picture

dawehner
Primary language German
commented

Are you sure we also use no ID at all for CSS and what not?

cilefen’s picture

cilefen commented

I am not totally sure. But it looks as though the Ajax wrapper IDs are hardcoded in this form so this serves no purpose. This static isn't set anywhere nor is $seen_ids_init even used.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Isn't this simply dead, obsolete code? Grep HEAD for drupal_html_id, this is the only match. So I don't see why we'd need tests for that?

I think there is no need to reset the seen IDs here because the ID of preview form wrapper is always views-preview-wrapper. This class never tries to generate an ID.

Exactly. And on top of that, the Html class is only used to generate a unique ID if #id isn't already set — see this in FormBuilder:

    if (!isset($element['#id'])) {
      $element['#id'] = Html::getUniqueId('edit-' . implode('-', $element['#parents']));
    }
cilefen’s picture

cilefen commented
Title: ViewPreviewForm should reset seen HTML IDs with Html::resetSeenIds() » Remove dead HTML ID-tracking code from ViewPreviewForm
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed e219cb9 and pushed to 8.0.x. Thanks!

  • alexpott committed e219cb9 on 8.0.x 
    Issue #2443847 by cilefen: Remove dead HTML ID-tracking code from...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.