WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 9 months ago

#44883 closed enhancement (fixed)

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

Reported by: westonruter Owned by: laurelfulford
Milestone: 5.0.3 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:

Description (last modified by 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.

Attachments (3)

44883.0.diff (2.2 KB) - added by westonruter 13 months ago.
44883.1.diff (4.3 KB) - added by westonruter 13 months ago.
44883.2.patch (1.1 KB) - added by laurelfulford 9 months ago.

Download all attachments as: .zip

Change History (27)

#1 @westonruter
13 months ago

  • Description modified (diff)
  • Keywords has-patch added
  • Owner set to laurelfulford
  • Status changed from new to reviewing

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?

#2 @laurelfulford
13 months ago

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

#3 follow-up: @dlh
13 months ago

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?

#4 in reply to: ↑ 3 ; follow-up: @westonruter
13 months ago

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.

Last edited 13 months ago by westonruter (previous) (diff)

#5 in reply to: ↑ 4 ; follow-up: @dlh
13 months ago

Replying to westonruter:

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

Yep, exactly that.

#6 in reply to: ↑ 5 @westonruter
13 months ago

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.

#7 @dlh
13 months ago

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.

#8 @laurelfulford
13 months ago

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

#9 @westonruter
13 months ago

  • Keywords commit added

Would you like to commit?

#10 @laurelfulford
13 months ago

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.)

This ticket was mentioned in Slack in #core-committers by laurelfulford. View the logs.


13 months ago

#12 @SergeyBiryukov
12 months ago

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.

#13 @SergeyBiryukov
12 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#14 @SergeyBiryukov
12 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.9 consideration.

#15 @pento
12 months ago

  • Keywords fixed-major removed
  • Milestone changed from 4.9.9 to 5.0.1

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

#16 @pento
9 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#17 @pento
9 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#18 @laurelfulford
9 months ago

  • Milestone changed from 5.0.3 to 5.1

#19 @laurelfulford
9 months ago

  • Milestone changed from 5.1 to 5.0.3

#20 @laurelfulford
9 months ago

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!

#21 @desrosj
9 months ago

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.

#22 @desrosj
9 months ago

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.

#23 @desrosj
9 months ago

In 44408:

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

Previously introduced in [43659].

See #44883.

#24 @desrosj
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.