Twenty Seventeen: Use simple counter rather than uniqid() for generating unique IDs for HTML elements

[westonruter]

The Twenty Seventeen theme uses ​uniqid() in twentyseventeen_get_svg() and searchform.php in order to generate a unique IDs for HTML elements. While this works, it is somewhat overkill: the uniqid() generates a string based on the current microsecond (e.g. search-form-5b8cb8ffd2f29). However, it seems plausible that on a fast machine two calls could happen within the same microsecond. There is also a warning on the PHP docs page about this function:

Warning: This function does not guarantee uniqueness of return value. Since most systems adjust system clock by NTP or like, system time is changed constantly. Therefore, it is possible that this function does not return unique ID for the process/thread. Use more_entropy to increase likelihood of uniqueness.

In any case, it seems all this can be avoided because all that Twenty Seventeen needs is a function that returns an integer which is incremented with each call (e.g. search-form-1). In Underscore/Lodash there is a _.uniqueId() function which does just this. It seems plausible that two calls to uniqid(). This will guarantee that each call will return a unique value in a PHP process, whereas it will remain consistent across processes.

A further problem with using a constantly-changing value across processes can be seen in the ​AMP plugin. The plugin has a post-processor which takes the output-buffered template rendered from WordPress and loads it into a DOM document for post-processing to ensure HTML elements are converted into their AMPHTML equivalents, in addition to making sure that there is no markup left in the response that is not valid AMP. To speed up this post-processor step, once it has finished post-processing the plugin computes an MD5 hash or the resulting HTML from the serialized DOM, and stores it in the object cache with the hash as the key. The next time that the post processor is about to run it first checks to see if it has already post-processed the HTML in the current response, and if so, it can short-circuit with the cached output. This is not possible in Twenty Seventeen, however, because the generated HTML is different for every single time that WordPress generates a template. This causes an additional problem for the plugin because it results in a 100% cache miss rate. See GitHub ​issue and ​PR where the plugin now detects for high cache miss rate and turns off the post processor cache when it happens.

But all of this could be avoided if Twenty Seventeen just used a more appropriate function for generating unique HTML IDs.

Introduced in #38659 and ​before the Twenty Seventeen merge.

[westonruter]

44883.0.diff​ introduces twentyseventeen_uniqid() which is pretty much a straight PHP port of the _.uniqueId() function in Underscore.js. This function is then used instead of uniqid() and the values it returns.

@laurelfulford Would you please review?

[laurelfulford]

@westonruter Sure thing! I'll try to get to this tomorrow; if not, by the end of the week.

[dlh]

Having implemented essentially the same thing on more than one project, I personally would value the function in the patch being made available in core generally. What kind of case would need to be made for that to happen?

[westonruter]

Replying to dlh:

Having implemented essentially the same thing on more than one project, I personally would value the function in the patch being made available in core generally. What kind of case would need to be made for that to happen?

Like a wp_unique_id() function added to wp-includes/functions.php? I like that. We would just need to differentiate it from being a universially unique ID, which is what wp_generate_uuid4() is for.

[dlh]

Replying to westonruter:

Like a wp_unique_id() function added to wp-includes/functions.php?

Yep, exactly that.

[westonruter]

Replying to dlh:

Replying to westonruter:

Like a wp_unique_id() function added to wp-includes/functions.php?

Yep, exactly that.

That's a good idea, but I just realized that it wouldn't be suitable in this case for Twenty Seventeen because the theme needs to work backwards to the 4.7 branch.

See 44883.1.diff​ which still retains a twentyseventeen_unique_id() in Twenty Seventeen, but uses a new wp_unique_id() when it is available.

[dlh]

I had forgotten about the compatibility requirement for Twenty Seventeen. Given that the theme needs its own implementation regardless, I'd be happy to open a ticket for the core function if it would be preferable to consider it separately. All that said, the wp_unique_id() in 44883.1.diff​ works for me as advertised.

[laurelfulford]

@westonruter Sorry I got back to this a little later than I'd hoped. 44883.1.diff​ looks good to me as well!

[westonruter]

Would you like to commit?

[laurelfulford]

I can't commit this one since it includes a file outside of the themes directory -- I don't have the power -- but I can ask in the committers Slack channel for another set of eyes.

(Thanks for following up on this! For some reason I didn't think to add that when I commented earlier -- I blame it on Monday.)

[SergeyBiryukov]

In 43658:

General: Introduce wp_unique_id(), a PHP implementation of Underscore's uniqueId method.

A static variable contains an integer that is incremented with each call. This number is returned with the optional prefix.
As such the returned value is not universally unique, but it is unique across the life of the PHP process.

Props westonruter, dlh.
See #44883.

[SergeyBiryukov]

In 43659:

Twenty Seventeen: Use a simple counter incremented with each call instead of uniqid() for generating unique IDs for HTML elements.

Props westonruter.
Fixes #44883.

[SergeyBiryukov]

Reopening for 4.9.9 consideration.

[pento]

Moving to 5.0.1. The version numbers referenced in the previous commits will need to be updated.

[laurelfulford]

Apologies, I punted this on autopilot!

I've added an update in 44883.2.patch​ that changes the WordPress version from 4.9.9 to 5.0.3, and the Twenty Seventeen version from 1.8 to 2.0.

@SergeyBiryukov could you please take a look at this one again, since I can only commit the theme files? I'm also happy to take care of the Twenty Seventeen half of it, I just want to make sure both get in at roughly the same time. Thanks!

[desrosj]

In 44406:

General: Update since annotation for wp_unique_id().

In [43658], wp_unique_id() was introduced. This updates the since annotation to be accurate.

See #44883.

[desrosj]

In 44407:

General: Introduce wp_unique_id(), a PHP implementation of Underscore's uniqueId method.

A static variable contains an integer that is incremented with each call. This number is returned with the optional prefix.
As such the returned value is not universally unique, but it is unique across the life of the PHP process.

Props westonruter, dlh.

Merges [43658] and [44406] to the 5.0 branch.
See #44883.

[desrosj]

In 44408:

Twenty Seventeen: Update since and see annotations for twentyseventeen_unique_id().

Previously introduced in [43659].

See #44883.

[desrosj]

In 44409:

Twenty Seventeen: Use a simple counter incremented with each call instead of uniqid() for generating unique IDs for HTML elements.

Props westonruter, laurelfulford.

Merges [43659] and [44408] to the 5.0 branch.
Fixes #44883.