Make WordPress Core

Changes between Initial Version and Version 1 of Ticket #44883


Ignore:
Timestamp:
09/03/2018 05:12:29 AM (7 years ago)
Author:
westonruter
Comment:

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?

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #44883

    • Property Keywords has-patch added
    • Property Owner set to laurelfulford
    • Property Status changed from new to reviewing
  • Ticket #44883 – Description

    initial v1  
    1 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.
     1The 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:
    22
    3 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.
     3> 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.
     4
     5In 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.
     6
     7A 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.
    48
    59But all of this could be avoided if Twenty Seventeen just used a more appropriate function for generating unique HTML IDs.
     10
     11Introduced in #38659 and [https://github.com/WordPress/twentyseventeen/commit/2d0d7737061811d357be58ee955eeacfadd42534 before the Twenty Seventeen merge].