Remove usage of uniqid

Useful information here: https://git.drupalcode.org/project/drupal/-/merge_requests/4801#note_208955

tldr I found that Crypt::randomBytesBase64(16) is 3x faster than uniqid(), because uniqid() sleeps for a full microsecond on many platforms.

But if Crypt::randomBytesBase64() is considered too much of a CPU hog, PHP 7.3+ function hrtime() could be used to build a much faster time-based "uniqid" (to which extra entropy could be added).

Opened https://github.com/mglaman/phpstan-drupal/issues/604 for adding a rule to prevent usage of uniqid(), so that if we do this then we’re not regressing later.

I do not think this code should go into phpstan-drupal. It seems like a candidate for a third-party library like https://github.com/ekino/phpstan-banned-code which allows banning particular functions and other code to be used.

Drupal core does not need to add that library, though. It could provide the rule itself and register it within the phpstan.neon

So we could have \Drupal\Core\Phpstan\Rules\NoUniqidRule that checks FuncCall for that function.

Then the phpstan.neon just needs the following added:

rules:
	- Drupal\Core\Phpstan\Rules\NoUniqidRule

Well if a PHPStan extension covers the use case already, then why reinvent the wheel? Will open a spin-off to experiment.

Filed #3390368: Adopt ekino/phpstan-banned-code and use it to prevent further usage of uniqid(), md5(), sha1(). Thanks @mglaman for the pointer!!

then why reinvent the wheel

The suggestion was to avoid adding a dependency for a pretty simplistic rule.

And the package has other things in it not quite needed. And it makes assumptions:

Major - Should not use node with type "Expr_Exit", please change the code.
in core/assets/scaffold/files/ht.router.php:30

#20 yes hit that, see the child issue.

Still I think if we do sth Drupal, we should still make it configurable so that one rule class, many blocked functions, as opposed to as many rule classes as the possible functions we will ever want to block.

We could potentially use the cspell banned word list for this, see #3274896: Add some words we'd prefer to avoid to Drupal core's flagWords for a different use case.

It would mean you can't explain anywhere why you're not using uniqid() (at least not without using cspell:ignore) but no dependency and no code involved to use that.

I could see us using this this md5() and sha1() once #3307718: Implement xxHash for non-cryptographic use-cases is in too.

Mmm IMHO mixing spell checking with static analysis is no good. And I'm no purist :)

Just adding a single ignoreErrors entry could make the library do what we need ATM, see https://www.drupal.org/pift-ci-job/2774106 which also bans md5() and sha1().

The good things - 1) it explains it's the use of the function that is no good (vs spellcheck); 2) we can baseline the errors now, like anything PHPStan, stop the bleeding, and fix the existing usage at different pace.

In case anyone was curious, I did some benchmarking of various unique ID methods, although just on one Linux system so this is very anecdotal.

Crypt::randomBytesBase64(16) was faster than symfony/uid package's Uuid::v4() (which outputs basically the same thing in a different format).

My homemade function hrtime(TRUE) . '-' . mt_rand() is faster still. The hrtime() counter wraps around when the system reboots.

Method:                        Time:       Sample:
hrtime(TRUE) . '-' . mt_rand()   93.606364 84465946443474-977886000
Crypt::randomBytesBase64(16)    464.163955 JE9NYPHi5GnozpnSY2D5ew
(string) Uuid::v4()             696.964993 56e1832f-50f7-4fe8-8f0e-d2a1457c35a4
uniqid('', TRUE)               1000.533745 652496918083f3.85240928

(Edited to remove some wrong info re: cross-platform behavior, that was just due to the systems rebooting.)

In #3408045: [PP-1] Replace using mt_rand() to generate a unique file upload progress ID with bin2hex(random_bytes(16) we a proposing to use bin2hex(random_bytes(16) I would be interested to see how that compares to the other options.

In my testing, bin2hex(random_bytes(16)) was about 22% faster than Crypt::randomBytesBase64(16), so it would be in second place in the chart above. of course it generates a 45% longer string (32 characters instead of 22 characters).

Ah good to know. I had to look up the reason we had Crypt::randomBytesBase64() and makes sense if you need it to be URL-safe.

#3581442: Replace usage of uniqid() in the Database system is in and replaced uniqid with bin2hex(random_bytes(12)).