I was looking at the number of abstract classes that are suffixed with Base (like PluginBase, ConfigEntityBase, AttributeValueBase) and I noticed that there is only one Drupal abstract class that is prefixed with Abstract.

Then I thought, maybe it's a coincidence, and the name actually means something. But there is no class docblock, so it's NOT clear why it's named that.

Furthermore, DatabaseStorage extends it, but MemoryStorage does not, due to "performance reasons".

So, let's document the class and consider renaming it to StorageBase to match the rest of core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
2.16 KB

I spoke with chx about this in IRC, he confirmed that the 'Abstract' in 'AbstractStorage' bore no significance.

Added the missing docblock and renamed the file.

tim.plunkett’s picture

FileSize
2.17 KB

After uploading that, I noticed that elsewhere it's "key/value" not "key-value".

chx’s picture

Status: Needs review » Reviewed & tested by the community

Sure.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/KeyValueStore/MemoryStorage.phpundefined
@@ -10,7 +10,7 @@
  *
- * For performance reasons, this implementation is not based on AbstractStorage.
+ * For performance reasons, this implementation is not based on StorageBase.

Shouldn't this be the fully namespaced name here like all other class references?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.2 KB

Fair enough.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Good, RTBC again.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Seems sensible to me.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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