There's no reason we ever need to reset this, it causes 33 calls to drupal_static() on a default install. drupal_static() still comes up as the number two most expensive function in terms of self time when profiling HEAD, with 300 calls to it taking around 3% of the request.

CommentFileSizeAuthor
#4 drupal_html_id_static.patch897 bytescasey
#1 html_id_drupal_static.patch772 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
FileSize
html_id_drupal_static.patch772 bytes

Status: Needs review » Needs work

The last submitted patch, html_id_drupal_static.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

hmm, or perhaps it should...

casey’s picture

casey CreditAttribution: casey commented
Status: Needs work » Needs review
FileSize
drupal_html_id_static.patch897 bytes

drupal_static should be used here but we could use the advanced drupal_static() pattern.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Works for me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

This looks fine, but could you explain why we do need drupal_static() here at all, since it's just purely a wrapper helper function around some str_replace() stuff? This feels like maybe a discussion that happened on IRC and didn't make its way to the issue queue...

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

two things. first, nitpick, no need to escape '-' in the regex, just make it the last character in the set:

  $id = preg_replace('/[^A-Za-z0-9\-_]/', '', $id);

can be written as:

  $id = preg_replace('/[^A-Za-z0-9_-]/', '', $id);

second, why do we need to use the escaped id as the key, and keep the increment scope so narrow? if we use the unescaped id and make the increment scope wider, then we can skip repeated preg_replace() calls, something like:

function drupal_html_id($id) {
  static $increment = 0;
  $seen_ids = &drupal_static(__FUNCTION__, array());
  $id = strtr(drupal_strtolower($id), array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));
  
  if (!isset($seen_ids[$id])) {
    $seen_ids[$id] = preg_replace('/[^A-Za-z0-9_-]/', '', $id);
    return $seen_ids[$id];
  } 
  
  return $seen_ids[$id] . '-' . ++$increment;
}

EDIT: fixed bug in code snippet.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

I didn't look into the failures in depth but it's form rebuilding and multistep. Likely only affects tests but that's why we have drupal_static() in the first place.

casey’s picture

casey CreditAttribution: casey commented

drupal_static is there so you can reset it, right? Theoretically you could call a page twice during one reqest. Their id's shouldn't differ.

Or have I misinterpreted drupal_static?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Here's why the tests fail (edit, without drupal_static()):

// drupal_html_id() maintains a cache of element IDs it has seen,
    // so it can prevent duplicates. We want to be sure we reset that
    // cache when a form is processed, so scenarios that result in
    // the form being built behind the scenes and again for the
    // browser don't increment all the element IDs needlessly.
    drupal_static_reset('drupal_html_id');

From form.inc

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I agree with #9. Resettable static is needed here. #10 is one use-case, and there may be others in contrib. If performance is the issue here, can we rename this issue to something like "Optimize drupal_html_id()"? If that's what this issue becomes, then +1 on #4. -1 for the part of #7 that makes $increment non-resettable. If parts of #7 can be added to #4 and demonstrate useful performance gain, then I'm all for adding them.

Also related is #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page. In AJAX contexts, we need unique ids across page requests, and that is not currently handled by drupal_html_id(). That issue may find a solution that doesn't touch drupal_html_id(), or it may somehow expand the scope of drupal_html_id().

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Shouldn't this effort be rolled into #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page and get done?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: drupal_html_id() should not use drupal_static() » drupal_html_id() should use $drupal_static_fast pattern

Apparently it wasn't. Re-titling. This is still the cause of 80/374 drupal_static() calls on a node/n page, combined calls to drupal_static() take 3% of cpu according to xhprof.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I'm not opposed to drupal_html_id() using $drupal_static_fast, but before we do that, there's two other things that might be worth looking into:

1) Currently, each drupal_html_id() call involves two drupal_static() calls because of the changes from #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page, but looking at that code again, $seen_ids_init is ineffective as a drupal_static() since its result is then used as the 2nd param for the $seen_ids drupal_static(), but the way drupal_static() works, $default_value can never be reset. So we can probably just get rid of $seen_ids_init as a drupal_static() entirely, and save half our calls.

2) As discussed in that other issue, we really shouldn't be invoking drupal_html_id() that often as the requirement for HTML ids to be unique constrains their utility. They can and should be used for attaching Drupal JS settings and behaviors, including enabling AJAX, and perhaps for FAPI input elements to receive LABEL tags, but using ids for CSS is a bad idea (except within page.tpl.php or similar situations where you can assume uniqueness and don't even need to call drupal_html_id() at all), as the id can get a counter appended to it, resulting in the CSS rule not getting applied. For example, see #710172: HTML ids for blocks should not contain underscores. For the node/n page, the vast majority of the calls are for the toolbar, so they only happen for users who have access to the toolbar. Not sure if that alone changes the equation for whether $drupal_static_fast is worthwhile, and also, is there a reason for toolbar items to have ids?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

subscribing

casey’s picture

casey CreditAttribution: casey commented
Status: Needs work » Needs review
Issue tags: -Performance

#4: drupal_html_id_static.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, drupal_html_id_static.patch, failed testing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

This is still a really big issue in 7.x. Even on medium-sized HTML forms, drupal_html_id() can account for 5% or more of the page rendering time. Stupidly, because most of the time we compute IDs that we don't ever display.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Related #1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 8.x-dev » 7.x-dev
Issue summary: View changes

This issue appears to be obsolete for D8; #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset()

In addition, some other issue is trying to remove the unique ID generator in drupal_html_id() entirely.

Thus, moving to D7.