Problem/Motivation

DatabaseStorageControllerNG has diverged from EntityStorageControllerInterface
ConfigStorageController is missing out on recent improvements

Proposed resolution

Add EntityStorageControllerBase
Add new public methods from DatabaseStorageController to the interface

Remaining tasks

Deal with the following:
getFieldDefinitions()
baseFieldDefinitions()
cacheGet()
cacheSet()

Also some of the protected methods
mapToStorageRecord()
mapToDataStorageRecord()
mapFromStorageRecords()
attachPropertyData()

Files: 
CommentFileSizeAuthor
#22 drupal-1978870-22.patch11.25 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,367 pass(es).
[ View ]
#22 interdiff.txt716 bytesdawehner
#18 interdiff.txt1.21 KBandypost
#18 1978870-18.patch10.96 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]
#17 1978870-17.patch10.9 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,661 pass(es).
[ View ]
#9 1978870-9.patch10.48 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,058 pass(es).
[ View ]
#9 interdiff-1978870-9.txt1.04 KBdamiankloip
#7 1978870-7.patch10.67 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,853 pass(es).
[ View ]
#5 1978870-5.patch10.67 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 1978870-4.patch286.91 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 interdiff-1978870-4.txt581 bytesdamiankloip
#2 1978870.patch10.66 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

fago’s picture

Yeah, I agree that would be a good idea.

damiankloip’s picture

Status:Active» Needs review
StatusFileSize
new10.66 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I don't think we should try to do too much here tbh. And Tim, I still think we should not try to make config entities fieldable :) Here is an initial patch moving over some basic properties and cache methods to a base class. Let's get the ball rolling atleast.

I haven't touched any caching logic in the COnfigStorageController class, as it seems we should tackle that in the summary related issue.

Status:Needs review» Needs work

The last submitted patch, 1978870.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new581 bytes
new286.91 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

whooops

damiankloip’s picture

StatusFileSize
new10.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Doh, this is the actual correct patch. sorry for the noise.

Status:Needs review» Needs work

The last submitted patch, 1978870-5.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new10.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,853 pass(es).
[ View ]

Really, don't roll patches when you first wake up!

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -0,0 +1,133 @@
+   * Implements Drupal\Core\Entity\EntityStorageControllerInterface::__construct().

{@inheritdoc}! Otherwise this is great.

damiankloip’s picture

StatusFileSize
new1.04 KB
new10.48 KB
PASSED: [[SimpleTest]]: [MySQL] 57,058 pass(es).
[ View ]

Great, thanks.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -0,0 +1,133 @@
+  public function __construct($entityType) {
+    $this->entityType = $entityType;
+    $this->entityInfo = entity_get_info($entityType);
+    // Check if the entity type supports static caching of loaded entities.
+    $this->cache = !empty($this->entityInfo['static_cache']);
+  }

The parent method has $entityType, so we maybe fix it now, or in a follow up

damiankloip’s picture

fago’s picture

#1893820: Manage entity field definitions in the entity manager takes care of moving the fieldDefinitions part out - so this one would by solved with that then.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

I'm not sure of the validity of this patch given the discussion about config context and entity static caching. Tempted to set as won't fix.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Based on issues like #2004756: Defining default values for config entities and many other divergences the two classes we've seen, we absolutely still need this.

We can choose to remove the shared cache code if we need to later.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Needs re-roll :(

tim.plunkett’s picture

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new10.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,661 pass(es).
[ View ]

Rerolled.

I think we need this either way.

andypost’s picture

Issue tags:+cache
StatusFileSize
new10.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]
new1.21 KB

Suppose caching should be implemented properly

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -136,14 +102,6 @@ public static function createInstance(ContainerInterface $container, $entity_typ
-  public function resetCache(array $ids = NULL) {
-    // The configuration system is fast enough and/or implements its own
-    // (advanced) caching mechanism already.

@@ -183,15 +141,6 @@ public function load(array $ids = NULL) {
-    $this->resetCache(array($id));

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -175,23 +124,6 @@ public function __construct($entity_type, array $entity_info, Connection $databa
-    // Check if the entity type supports static caching of loaded entities.
-    $this->cache = !empty($this->entityInfo['static_cache']);
...
-        unset($this->entityCache[$id]);

@@ -262,15 +194,6 @@ public function load(array $ids = NULL) {
-    $this->resetCache(array($id));

@@ -444,34 +367,6 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
-    if (!empty($this->entityCache)) {
...
-  protected function cacheSet($entities) {
-    $this->entityCache += $entities;

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -0,0 +1,133 @@
+    // Check if the entity type supports static caching of loaded entities.
+    $this->cache = !empty($this->entityInfo['static_cache']);
...
+  public function resetCache(array $ids = NULL) {
+    if (isset($ids)) {
...
+    if (!empty($this->entityCache)) {

Usage of $this->cache was broken for db entities?

Status:Needs review» Needs work
Issue tags:-Entity system, -cache, -Configuration system, -Needs reroll, -Configurables

The last submitted patch, 1978870-18.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+Entity system, +cache, +Configuration system, +Needs reroll, +Configurables

#18: 1978870-18.patch queued for re-testing.

andypost’s picture

needs re-roll

dawehner’s picture

StatusFileSize
new716 bytes
new11.25 KB
PASSED: [[SimpleTest]]: [MySQL] 55,367 pass(es).
[ View ]

Rerole and a minor thing.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Nice!

effulgentsia’s picture

Issue tags:-Needs reroll

Amazingly, #22 still applies despite being 4 days old, so removing the "Needs reroll" tag.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 1745bd2 and pushed to 8.x. Thanks!

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