PrivateTempStore::set() will fail if $key is too long

Please add steps to reproduce so i can verify the fix as per the expectations.

Call PrivateTempStore::set() with a long $key param, such as greater than 128 characters. (It can fail with shorter ones too, but that length is sure to fail.)

FWIW, I vote for just throwing an exception. 80 characters seems like more than enough. One downside is that someone might test their code using 100 character keys and logged-in users, then when their code is released it blows up when faced with anonymous users, but not everything can be allowed for.

I encountered the problem in vbo_export module because that generates keys from several variables, such as machine names for Views. If they are long, they add up to too much.

I've added an exception for when the key length exceeds 84 characters. Please review this patch.

Will need test coverage.

Also may need a CR as we are throwing a new exception.

After $key = $this->createkey($key); in PrivateTempStore::set, $key will be the full key (like "bfCKE9aB6eKLnKPuyGkXIvL5v3iavdtgw48ilCheSYs:somevalue"). That can be 128 chars. So, I think the number in the patch should be changed. It also might be a good idea to use a

const

to hold that number and include a comment with an explanation.

Adding the updated patch:

1) Addition of constant for string length limit as suggested in #9.
2) Increased length limit to 120 characters as suggested in #9 (Though this limit may be debated and increased or decreased as per consensus arrived at).
3) Added PhpUnit test.

Kindly review and suggest.

forgot top update the issue status.

Rather than throwing an exception we ought to be able to hash the key if it exceeds a certain length.

So, what you mean is if someone tries to save something using a 1000 character key, hash the key before saving? Then, when retrieving that 1000 character key, hash that to get the real key?

Yes that's right.

We have similar logic in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21D...

See also #2872276: Database lock backend should normalize the lock name → for the lock system.

adding the updated patch:
1) Included hashing of key.
2) Updated test case for set and get methods.

Kindly review and suggest.

1. +++ b/core/lib/Drupal/Core/TempStore/PrivateTempStore.php
   @@ -102,7 +113,12 @@ public function __construct(KeyValueStoreExpirableInterface $storage, LockBacken
      public function get($key) {
   +    if (strlen($key) > self::MAX_TEMPSTORE_KEY_NAME_LENGTH) {
   +      // Hash the value.
   +      $key = Crypt::hashBase64($key);
   +    }
   

   I think the main question is whether to keep the first few characters of the key for debugging etc., this is probably a less common thing to debug than cache IDs though.

2. +++ b/core/lib/Drupal/Core/TempStore/PrivateTempStore.php
   @@ -118,6 +134,8 @@ public function get($key) {
       *   Thrown when a lock for the backend storage could not be acquired.
   +   * @throws \InvalidArgumentException
   +   *   Thrown when the provided key's length exceeds the maximum allowed length.
       */
   

   This can be removed.

#16 🐛 PrivateTempStore::set() will fail if $key is too long Needs review .2 - DONE

#16 🐛 PrivateTempStore::set() will fail if $key is too long Needs review .1 -
Here I get the point that we would like to preserve few initial words from the original key... Do not see any harm in doing/implementing that but at the same time I also do not see any value addition in doing that either (kindly help me in case I am missing some thing).

[just thinking aloud here] As in any case the user adding such long tempstore key will generally keep using the same key that is hashed and will always get the desired values. If we attempt to preserve some part from the original key how will that help or what value addition we intend to achieve here!

The value addition is let's say you store "flurb" and "zumbo" (pretend those are longer names). Hashing would make those "kq3w12" and "riqjuwq", keeping the first few letters would be "fluw12" and "zumjuwq". That way you could kinda see what they are in case you need to debug something.

Yes #18 is correct, it's for debugging convenience.

For #16.1

While at it could the issue summary be updated slightly with the proposed solution. Currently there are 3

Thanks for clearing the air, adding updated patch:
1) Updated logic to preserve original key as much possible with hashed suffix.
2) Updated tests.

Kindly review and suggest.

For the IS update.

Updated the patch to limit the code changes to "createKey" method only.
Realised this while updating the IS.

kindly review and suggest.

Remember that the value returned by ::getOwner() can be as much as 44 characters for anonymous users. The patch in #24 only allows for it to be 11 characters.

Thanks Liam Morland → for pointing out the scenario, I have update the patch to take care of the ID value (retured from getOwner) that is prefixed to key.

Kindly review and suggest.

apologies for the noise... kindly ignore patch at #26 🐛 PrivateTempStore::set() will fail if $key is too long Needs review .

Changes introduced:
re-provisioned for the loss of characters owing to 'ascii_general_ci' collation, this error got introduced in patch at 26 so kindly ignore that.
Concern raised in #25 has been addressed.

Kindly review and suggest.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)