Change placeholder token generation from using sha1 to crc32b

There are also quite a lot of Crypt:: placeholders floating around.

Updated with Crypt::hashBase64(serialize($placeholder_render_array));

Needs tests also.

Updated with Crypt::hashBase64(serialize($placeholder_render_array));

IMHO we better want to switch to a different way than crypt in order to make it clear, that we don't deal about something security related here.

#6 Looks good. manually reviewed the patch.

+++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -61,7 +61,7 @@ public function providerPlaceholders() {
+      $token = crc32(serialize($token_render_array));

Verified - Crc32 is being used correctly. making it RTBC.

Alright, we we don't want to convert Crypt::hashBase64 over here, does someone mind opening up a follow up for that?

We should use hash('crc32b') instead, see #2546680: Use hash('crc32b') not crc32().

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -669,7 +669,7 @@
-    $attributes['token'] = crc32(serialize($placeholder_render_array));
+    $attributes['token'] = hash('sha1', serialize($placeholder_render_array));

+++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -61,14 +61,14 @@
-      $token = crc32(serialize($token_render_array));
+      $token = hash('sha1', serialize($token_render_array));

@@ -275,7 +275,7 @@
-    $token = crc32(serialize($expected_placeholder_render_array));
+    $token = hash('sha1', serialize($expected_placeholder_render_array));

Oops — these re-introduce SHA1. :)

This is a case where we might want to be concerned about hash collisions. I'm not sure if that's a real issue, but compared to any other usage it's more likely to be a problem.

+++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -68,7 +68,7 @@ public function providerPlaceholders() {
-      $token = hash('sha1', serialize($token_render_array));
+      $token = hash('crc32', serialize($token_render_array));

Issue title says to switch from sha1 to crc32b, not crc32.

#18: Would md5 or another algorithm be better here?

This is a case where we might want to be concerned about hash collisions. I'm not sure if that's a real issue, but compared to any other usage it's more likely to be a problem.

Awesome point!

I think that if the placeholders were using solely the CRC32 check, that we could indeed have a problem. There are 2^32 (>4 billion) possible hashes. But, more importantly, the hash here is only a subset of the placeholder. We always also list the callback and arguments. This was originally done not to avoid collisions, but to improve debuggability. But now, it effectively helps to avoid collisions.

#20: indeed. I missed that.

This is going to conflict with #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags, but that's easy enough to resolve.

So do we need to force a cache clear or something because placeholders are changing - is there an upgrade path implication here?

No, because:

• any placeholder works, even foobar can be used as a placeholder.
• the placeholders in #markup have the associated #attached[placeholders] already, so there is always guaranteed to be a 1:1 mapping

In other words: any cached render array is fully internally consistent. It doesn't matter if the outside world changes.

I don't think so since update.php does a drupal_flush_all_caches() whether or not there are updates to run:

https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Contro...

https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Contro...

Rewrote IS and added beta evaluation.

Catch is certainly right here, I ran into that, while working on #2540416: Move update.php back to a front controller

Committed 9f12768 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.