[wp-trac] [WordPress Trac] #44883: Twenty Seventeen: Use simple counter rather than uniqid() for generating unique IDs for HTML elements

[WordPress Trac]

#44883: Twenty Seventeen: Use simple counter rather than uniqid() for generating
unique IDs for HTML elements
---------------------------+----------------------------
 Reporter:  westonruter    |       Owner:  laurelfulford
     Type:  enhancement    |      Status:  reviewing
 Priority:  normal         |   Milestone:  4.9.9
Component:  Bundled Theme  |     Version:  4.7
 Severity:  normal         |  Resolution:
 Keywords:  has-patch      |     Focuses:
---------------------------+----------------------------
Changes (by westonruter):

 * keywords:   => has-patch
 * owner:  (none) => laurelfulford
 * status:  new => reviewing


Old description:

> 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
> random number to be used (e.g. `search-form-5b8cb8ffd2f29`), whereas 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 and
> Lodash there is a `_.uniqueId()` function which does just this. Not only
> is it less computationally expensive, but it also has a key advantage for
> a caching purpose.
>
> For example, the [https://github.com/Automattic/amp-wp AMP 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
> [https://github.com/Automattic/amp-wp/issues/1239 issue] and
> [https://github.com/Automattic/amp-wp/pull/1325 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.

New description:

 The Twenty Seventeen theme uses [http://php.net/uniqid 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 [https://github.com/Automattic/amp-wp 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
 [https://github.com/Automattic/amp-wp/issues/1239 issue] and
 [https://github.com/Automattic/amp-wp/pull/1325 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
 [https://github.com/WordPress/twentyseventeen/commit/2d0d7737061811d357be58ee955eeacfadd42534
 before the Twenty Seventeen merge].

--

Comment:

 [attachment:"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?

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/44883#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform