Problem/Motivation

If the old key and new key are identical, when calling rename() with MemoryStorage the data will be removed.

This is mitigated because MemoryStorage is only used during install (and some tests) and rename() isn't called at all.

Proposed resolution

Don't erase the data.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3031130-rename-2-PASS.patch1.46 KBtim.plunkett
#2 3031130-rename-2-FAIL.patch878 bytestim.plunkett

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new 3031130-rename-2-FAIL.patch878 bytes
new 3031130-rename-2-PASS.patch1.46 KB

In theory, it makes no sense to call rename if you aren't changing the keys. But there's nothing enforcing that... Which is why I think this is correct.

If I were writing this as a new interface I would consider throwing an exception if the keys are identical. But too late for that.

The last submitted patch, 2: 3031130-rename-2-FAIL.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Code looks good and patch still applies.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d70a4752348 to 10.0.x and e1b059c398c to 9.4.x. Thanks!

diff --git a/core/tests/Drupal/KernelTests/Core/KeyValueStore/StorageTestBase.php b/core/tests/Drupal/KernelTests/Core/KeyValueStore/StorageTestBase.php
index a768cfc2cd2..23059b8372b 100644
--- a/core/tests/Drupal/KernelTests/Core/KeyValueStore/StorageTestBase.php
+++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/StorageTestBase.php
@@ -197,9 +197,9 @@ public function testRenameNoChange() {
     $store = $stores[0];
 
     $store->set('old', 'thing');
-    $this->assertIdentical($store->get('old'), 'thing');
+    $this->assertSame($store->get('old'), 'thing');
     $store->rename('old', 'old');
-    $this->assertIdentical($store->get('old'), 'thing');
+    $this->assertSame($store->get('old'), 'thing');
   }
 
   /**

Fixed deprecated code on commit.

  • alexpott committed d70a475 on 10.0.x 
    Issue #3031130 by tim.plunkett: \Drupal\Core\KeyValueStore\MemoryStorage...

  • alexpott committed e1b059c on 9.4.x 
    Issue #3031130 by tim.plunkett: \Drupal\Core\KeyValueStore\MemoryStorage...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.