Spinning this out of #1302378: Use multiple specialized entity controllers.

The base EntityController class currently handles keeping it's own 'static' cache internal to the class, or always fetching from the database all the time.

In Drupal 7, this means that http://drupal.org/project/entitycache needs to provide it's own controller class to provide persistent caching for entities.

We can make this a lot more flexible by just replacing the custom caching within the Entity controller class, with a configurable cache controller key instead of the current 'static cache' key. There's no need for entity-specific cache controllers, since any class implementing CacheBackendInterface will do fine.

To allow for 'static' caching, I've added a new ArrayBackend which simply stores cache items in a php array. Apart from expires which didn't seem worth working on until #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support is in, this (untested) backend supports everything the database backend does, including tags etc., meaning it should be useful for unit testing eventually.

For no static caching at all, the NullBackend works fine.

To actually implement entitycache in D8 (whether core or contrib), you'd need a backend that proxies the MemoryBackend + whichever one is implemented for the cache_entity bin, since we're placing responsibility for both in a single class doing things this way. There's some inconsistencies with resetCache() etc. that would need to be sorted out for that.

Completely untested patch that will likely blow up, but just to show what it looks like.

One feature gets dropped here, which is fetching from cache when $conditions is passed to entity_load(). That's already a bit wonky as it is, and the $conditions parameter is already on its way out and documented as deprecated in 7.x per #1184272: Remove deprecated $conditions support from entity controller.

Files: 

Comments

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
11.72 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Without the syntax error..

Status: Needs review » Needs work

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

pounard’s picture

Sounds like a good improvement, the ArrayCache implementation is a good idea!

class DatabaseBackend implements CacheBackendInterface { Wrong name, right? Should be ArrayBackend ?

'cache controller' => 'Drupal\Core\Cache\MemoryBackend', Sounds wrong too, is it ArrayBackend or MemoryBackend? I'm fine with both, but you have to agree with yourself about this one!

pounard’s picture

If you don't react before I'm getting back home, I'll try to fix your patch.

sun’s picture

I think this needs more architectural planning.

The 'static cache' flag in the entity (crud) controller is vastly more intelligent than a completely separate memory cache backend.

It is true that we could attempt to split and separate that backend, but we don't want to lose the static caching within the crud controller entirely.

This patch adds "cache controller", but actually means "static cache controller".

Let's also bear in mind that there's a cost of instantiating one more object (including config + DI) for every single entity type.

pounard’s picture

We can chain backends if we want both static and a real cache, sounds like an easy win: we could get rid of this awful loading logic that do both, and rely on a unique object with a clean interface that would do both at the same time. It's a pattern we could also re-use at various places in Drupal too.

Instanciating a single object, you won't notice it on your runtime, it's not significant, even with 10 or 20 different entity controllers, this won't hurt a bit your execution time, it's really nothing compared to what does a single Views, yet most people continue to put up to 10 Views on the same page.

In Drupal 7 the static cache is quite dumb, all you have is a cacheGet() and cacheSet() internal methods, which acts like a basic key/value store (if we omit the deprecated conditions array). If it is smarter than this, please enlighten me because I probably missed it.

catch’s picture

Hmm I thought I'd fixed the silly naming issues already but apparently only in my head rather than the actual patch. I think we should go for ArrayBackend since then there's no chance of confusing it with an actual static, or APC or something, MemoryBackend sounds a bit more clever than it is.

@sun: the intention here is to allow the cache controller to do either no caching, static caching, persistent caching, or static + persistent caching (via chaining the two together), and also to start the process of pushing small bits of the entity code back into core/lib where they're generically useful. I'm not sure how viable that will actually be - often static and persistent caches need to be treated differently, but when I looked at offering the potential for both, at least within the controllers the access points are the same.

pounard’s picture

Some other ideas arround the idea:
- We could, in the future, share cache chains to entity controllers that needs the same chain (we should then prepare some kind of mass deregistration mecanism in the array cache for static entries to act upon controller destructor)
- This pattern, with a clean cache chain definition mecanism, could be used in a lot of other places where the code actually emulate this behavior, for example a lot of info hooks

Using it massively would remove a lot of duplicated code everywhere, it might even be useful in conjunction with the actual DrupalCacheArray implementation that could evolve to use this base.

Using the chain of command (for saving), chain of responsability (for loading) and proxy patterns (basically using a chain towards the objects behind) for this seems really clean in term of architecture because the final object only rely on the cache interface, without worrying about what's behind, it's actually maintainer's even somehow.

fago’s picture

Status: Needs work » Needs review

We can chain backends if we want both static and a real cache, sounds like an easy win: we could get rid of this awful loading logic that do both, and rely on a unique object with a clean interface that would do both at the same time. It's a pattern we could also re-use at various places in Drupal too.

I like the idea of having a dedicated cache system, which has a single consistent interface to developers. Having a NullCacheBackend + a static cache backend that proxies another permanent backend makes much sense too me too.

I don't like having the EntityStorageController directly talking to the cache system though. Imho, we loose some flexibility here.

I'd prefer the approach taken by #1302378: Use multiple specialized entity controllers: Have a dedicated entity cache controller class, which is invoked by entity_load() and handles everything related to caches - see #1302378-4: Use multiple specialized entity controllers (comment #4).

Reasons:

  • I like being able to talk directly to the storage controller, bypassing any caches without resetting them. E.g. this is what entity_load_unchanged() does now....
  • Then, the entity cache controller could be used by the entity providing module to easily implement custom caching extensions. E.g. in profile2 I'm statically caching profile ids per user. But by having this cache outside of the controller you'll have to re-implement all the cache reseting logic. Compared to that, you'd get the reset-logic basically for free when the custom cache is implemented in the cache-controller, as the cache-controller gets already reset when necessary.
  • We'll probably want some entity-type specific cache controller configuration, e.g. the cache bin to use. The entity cache controller would be the obvious place to take care of that.
  • Then, I'm not so sure about loosing the possibility to fetch from cache when loading by $conditions. Yep, $condition is deprecated, but we have not yet built the easy to use replacement aka entity_load_by_properties() or so. I could see having the helper using the same pattern as entity_load(), what would make it possible to specifically add at least static caching for frequently used queries.
fago’s picture

Status: Needs review » Needs work
catch’s picture

The patch already has a dedicated entity cache controller, it's just it only requires that to match the core cache interface. Custom entity-type-specific controllers would still be possible of course. For a controller that does persistent caching that's the most likely to need something custom. I'm not sure from your post what you think that's needed that's not in the cache interface already. Apart from the conditions stuff but I'd prefer to press on with the issue that removes it. Almost did that before starting this one but wanted to get this out for discussion.

pounard’s picture

Then, I'm not so sure about loosing the possibility to fetch from cache when loading by $conditions. Yep, $condition is deprecated, but we have not yet built the easy to use replacement aka entity_load_by_properties() or so. I could see having the helper using the same pattern as entity_load(), what would make it possible to specifically add at least static caching for frequently used queries.

I think that the conditions (or properties later) loading wouldn't be affected that much by replacing the internal cacheGet() with a backend directly: you still control what happens in your load() function and you are not forced to cache entities by id, you can do something else instead. You can still add a layer for frequently used queries in specific controllers.
But, to be honest, I think that trying to implement such layer would be more than difficult, the load() method may still be optimized later for very specific cases if needed. I even have a hunch that if the controller has direct access to its own cache backend, it would be easier to implement such frequently used queries caching: the controller knows its own schema and fields.

I'd prefer the approach taken by #1302378: Use multiple specialized entity controllers: Have a dedicated entity cache controller class, which is invoked by entity_load() and handles everything related to caches - see #1302378-4: Use multiple specialized entity controllers (comment #4).

The approach in #1302378: Use multiple specialized entity controllers suppose that there is an upper layer that needs to combine itself the cache and "main" controller, which means that the entry point of the API is in another piece of code (in the exemple in procedural code), I think this would be prone to errors: some developers will continue to hit the controllers directly (which makes a lot of sense) instead of calling entity_load(). I don't think this a good idea until there is a front controller for all entity operations.
I think that in the end, both provide the same feature and generalize caching method, but I prefer catch's method: it allows the entity "main" controller to implement its own load/save etc.. methods and have a finer control over its own cache if needed, while most of the others would probably continue to use the generic one.

We'll probably want some entity-type specific cache controller configuration, e.g. the cache bin to use. The entity cache controller would be the obvious place to take care of that.

Regarding specific entity type cache configuration, weither it's a cache controller or a chain set inside the main controller, I think both solutions are equivalent in term of configuration, especially if we can create the cache chain outside and inject it when building the controller instance: instead of creating a cache controller from configuration, we create a cache chain from configuration, doesn't seem too different in the end, it just moves some responsabilities.

And, latest note, I think that even if the cache controller is the way to go, this isntance can still benefit from using the cache chain catch is proposing, it's just a matter of moving it into another objet.

fago’s picture

I think that in the end, both provide the same feature and generalize caching method, but I prefer catch's method: it allows the entity "main" controller to implement its own load/save etc.. methods and have a finer control over its own cache if needed, while most of the others would probably continue to use the generic one.

That gets the point. I've aimed for moving that cache-control into a separate class, such that it stays separated from the storage class.
However, I just realized this thinking is deprecated, as we won't have "pluggable" storage controller anyways (sry, I'm still used to that thinking). As discussed in the entity-interface introduction issue, they already implement the per entity-type storage logic, so any simple storage-pluggability (=without repeating entity-type specific customizations) would have to happen on a lower level. In that light, having the cache-control in the storage-class makes sense to me too. :)

Apart from the conditions stuff but I'd prefer to press on with the issue that removes it. Almost did that before starting this one but wanted to get this out for discussion.

Great, I'm about to look at that as well :)

@patch: Having a permanent cache renders hook_entity_load() useless. If possible I'd prefer to drop it in the long road anyways, an entity really shouldn't be altered by page request. If it needs to be changed, re-save it with new property values.

pounard’s picture

Having a permanent cache renders hook_entity_load() useless. If possible I'd prefer to drop it in the long road anyways, an entity really shouldn't be altered by page request. If it needs to be changed, re-save it with new property values.

There's a point I agree with is that we shouldn't alter this kind of stuff on every load, it can get quite CPU intensive if you have a lot of modules playing with this. But, in the other end, having the hook_entity_load() can be useful for some modules to set business data which are not properties, but that needs to be on the entity and cached along with it. Concrete use case: I could load data from business tables and set them on a node, but I don't want to store this data in actual node properties because I don't want to duplicate it and handle the full synchronization between my source business data and node properties. If I can still attach arbitrary data on the object I can then just handle nice synchronisation by clearing a few cache on my business table updates. Right now, I actually achieve this using the virtual_field module, that provides a null field storage backend, but defining some virtual fields allows me to actually store precomputed data in field cache along with my commerce products, line items and stuff, without worrying about a complete field storage layer.

EDIT: Sorry, a bit off topic, and that's an akward and quite ugly way of doing this, but in my case, it's mainly for denormalizing some data for performances. I don't know where the property system is going to right and if it's going to help me with those kind of use cases.

What matters in here, is that by caching entities, we cache the hook_entity_load() result with it, and that's why I really like the idea, because if we do this I can then remove my need for hacks such as virtual field etc.

fago’s picture

What matters in here, is that by caching entities, we cache the hook_entity_load() result with it, and that's why I really like the idea, because if we do this I can then remove my need for hacks such as virtual field etc.

Yeah, if there remains a hook_entity_load() it has to be cacheable. Altering stuff or changing stuff on a per request base there is a bad idea, and we should stop supporting that.

The discussion of removing it probably belongs more into #1497374: Switch from Field-based storage to Entity-based storage though, as it depends on whether we want to support a "modular entity storage". However, even with a centralized entity storage I agree that it might be worthwhile to keep it for easily adding non-queryable stuff as you've depict in your use-case.

pounard’s picture

Yes, I honestly think those kind of non queryable stuff are really ugly, but sometime you just don't really have other straightforward choices. I don't know what will be final API for properties, but properties could solve that somehow.

fubhy’s picture

The ArrayCache implementation is a great idea indeed. However, I want to propose some changes and ideas that came up today while we were discussing the Entity Property API and waiting for fago to write down the initial interface definitions :).

The CacheChainInterface

Since we have multiple caching layers we might end up with various combinations of them (static cache only, static + persistent cache, persistent cache only, etc.). We could prevent that from happening if we had a way to chain cache backends in some way. This could be implemented in a generic fashion as a CacheChainInterface that would extend the CacheBackendInterface. This interface would extend the constructor definition for the CacheBackendInterface with an additional argument: An array of cache backends that the should be queried sequentially in order to retrieve an object from the cache.

The CacheChain class would iterate over the CacheBackend implementations that it has attached to it and forward the get / getMultiple queries to them. Once we have a full result for a query we exit the loop and, if there are any cache backends that could not return a result for the query before, we use the set() method on them to fill them with everything that they don't have cached yet. The next time the same query hits the CacheChain the first backend in the chain will now have element(s) that are subject to the query and return them right away.

This way we would be able to easily stack caching layers on top of each other and also replace single layers independently.

catch’s picture

@fubhy that sounds just like http://drupal.org/sandbox/catch/1153584 which I've never got around to implementing. I agree that'd be a good idea but I think we should add a separate issue for it. I also want to break the ArrayCache backend into a separate patch probably since that's useful for unit testing (and we also need to look at re-organising the cache tests so they're more re-usable with alternate cache backends).

fubhy’s picture

Oh great. I will take a look at your sandbox then. I think this can work really well with the PropertyContainerInterface that is part of the 3rd iteration of the Entity Property API for defining the tag-properties of the cache items (Note: the PropertyContainerInterface is generic and Entity agnostic so it would be completely generic).

Anyways... I agree this should be in a separate issue. I will create one later today and maybe already provide some code.

Sorry for off-topic :)

pounard’s picture

Not that off-topic, the cache chain interface is the idea behing this issue, and IMHO it should be done before we can actually continue this issue. It is a good reusable pattern that could be used at many places.

catch’s picture

@fubhy, the sandbox is empty, I really didn't do anything with it.

I'll try to make the array backend issue soon and upload a separate patch, new issue for cache chain sounds good too, then marking this postponed on them also sounds good :)

catch’s picture

pounard’s picture

Opened and implemented and unit tested #1651206: Added a cache chain implementation.

Alan D.’s picture

A small changes here could have significant ramifications (for better or worse), so I am adding a tag as a reminder to check performance.

I nearly committed a very basic EntityCache controller for the Countries module before doing some simple tests to discover that this caused a 20 fold decrease in speed. On the plus side of this testing, reading in an array of Country entities from the database was 3 times faster than including iso.inc and doing a single drupal_alter() on the countries list. So performance gains could be significant here :)

catch’s picture

fubhy’s picture

Awesome. Now we can get this rolling again. I worked on this a bit during DCon Munich already but was not really satisfied with the outcome because it was still hard-wired with the storage controller. My wish would be to make the caching layer independent of the storage system. I am not entirely sure how we would approach that yet, though. I do have a crazy suggestion though which I have been playing around with in my head lately but it would be a bit drastic to say the least. Will post more about that soon.

moshe weitzman’s picture

Anyone up for reviving this? Next steps?

podarok’s picture

#3 woops

root@pubuntu:/var/www/d8/drupal# git apply --check entitycache_0.patch
error: patch failed: core/modules/comment/comment.module:116
error: core/modules/comment/comment.module: patch does not apply
error: patch failed: core/modules/entity/entity.module:69
error: core/modules/entity/entity.module: patch does not apply
error: core/modules/entity/lib/Drupal/entity/EntityController.php: No such file or directory
error: patch failed: core/modules/system/system.module:273
error: core/modules/system/system.module: patch does not apply

a fast look to all the changes already in repository says this implementation very different against repo one so not possible for a fast reroll

fago’s picture

xjm’s picture

Issue tags: +Needs profiling
Berdir’s picture

Tagging.

Berdir’s picture

Issue tags: +Needs profiling

Weird, I could swear the the tags list was empty when I added those tags...

fago’s picture

effulgentsia’s picture

Priority: Normal » Major

Both of those are major, so raising this one to major as well.

fubhy’s picture

How about adding the caching layer as a decorator around the storage controller? We would then basically have a CacheController (which wraps the storage controller) and implements the StorageControllerInterface. Said CacheController would get a cache backend injected and try to retrieve entries from it and upon cache miss forward to the decorated storage controller.

Edit: This would help us to decouple storage from caching and and thereby remove the need for every storage controller to implement caching separately.

fago’s picture

Edit: This would help us to decouple storage from caching and and thereby remove the need for every storage controller to implement caching separately.

Indeed, this sounds clean and is straight-forward!

damiankloip’s picture

Assigned: Unassigned » damiankloip

Going to work on this.

damiankloip’s picture

Assigned: damiankloip » Unassigned

Actually, not right now!

Berdir’s picture

Issue tags: +Entity Field API

Tagging.

jhedstrom’s picture

The issue summary needs updating if there is remaining work here.

Berdir’s picture

Priority: Major » Normal

Just like I said years ago, I don't think it is realistic to replace the special "static" cache, it's more complicated than that. We do have persistent entity caching now, so I don't think this is still major.

andypost’s picture

catch’s picture

#2501117: Add static caching for PermissionsHashGenerator::generate() added a cache.static backend which could be used for this.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
2.95 KB

Things have changed a lot, starting again from scratch.

It's actually very straightforward doing this in EntityStorageBase, here's a proof of concept that doesn't explode on my local.

Status: Needs review » Needs work

The last submitted patch, 47: 1596472.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -135,8 +153,11 @@ public function resetCache(array $ids = NULL) {
+    if ($this->entityType->isStaticallyCacheable()) {
+      foreach ($ids as $id) {
+        if ($cached = $this->cacheBackend->get($this->getCid($id)))
+          $entities[$id] = $cached->data;
+        }
     }

missing closing } ?

catch’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

I think that's just a diff artifact.

But I did forget what I called the method apparently.

catch’s picture

Oh ouch, apparently my eyes are a diff artifact.

The last submitted patch, 50: 1596472.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 51: 1596472.patch, failed testing.

Berdir’s picture

\Drupal\Core\Entity\EntityTypeManager::clearCachedDefinitions():

$this->handlers = [];

And some more cases as well. That invalidates the static cache (and injected definition) of those handlers. That doesn't work anymore now.

catch’s picture

Status: Needs work » Needs review
FileSize
6.52 KB

Yeah had to be too good to be true.

Fixed subclasses of EntityStorageBase.

Implemented __destruct() for #54 which I'm not happy about but see if this gets closer to a green run.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -73,18 +75,28 @@
    */
-  public function __construct(EntityTypeInterface $entity_type) {
+  public function __construct(EntityTypeInterface $entity_type, CacheBackendInterface $cache_backend = NULL) {
     $this->entityTypeId = $entity_type->id();
     $this->entityType = $entity_type;
     $this->idKey = $this->entityType->getKey('id');
     $this->uuidKey = $this->entityType->getKey('uuid');
     $this->langcodeKey = $this->entityType->getKey('langcode');
     $this->entityClass = $this->entityType->getClass();
+    if (!isset($cache_backend)) {
...
+    }
   }

I think this should read:

$this->staticCacheBackend = $cache_backend ?: \Drupal::service('cache.static')

(right now you never assign if the backend is actually injected)

Also, I was just thinking..

a) $cache_backend should probably be $static_cache_backend?
b) I know that default_backend is overridden by setting a global default cache backend. Means you have to explicitly specify fast chained backends when using e.g. redis as default backend. Also means that redis will be used for cache.static in that case, oops... I think default_backend should override the default, need to open an issue about that.

Status: Needs review » Needs work

The last submitted patch, 55: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
7.4 KB

Was going to say it's too early for #56, but with 34 fails that's perfect timing to tidy things up. Addresses both points for now.

Status: Needs review » Needs work

The last submitted patch, 58: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
14.38 KB
7.27 KB

Fixed ConfigStorageBase and sub-classes as well as a couple of unit tests. Uploading to make sure this didn't introduce any silly regressions in web tests. Otherwise it's mostly updating unit tests from here to get to green.

Status: Needs review » Needs work

The last submitted patch, 60: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
15.69 KB
3.39 KB

Status: Needs review » Needs work

The last submitted patch, 62: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
12.91 KB

Rebased, not looking at test failures yet with this one.

Status: Needs review » Needs work

The last submitted patch, 64: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
13.65 KB

That __destruct() doesn't help, trying without.

Status: Needs review » Needs work

The last submitted patch, 66: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
19.84 KB
7.83 KB

Status: Needs review » Needs work

The last submitted patch, 68: 1596472.patch, failed testing.

Berdir’s picture

fail: [Other] Line 178 of core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php:
Drupal\KernelTests\Core\Command\DbDumpTest::testScriptLoad
Drupal\Core\Database\SchemaObjectExistsException: Table cache_static already exists.

This is a fun test fail. static isn't supposed to create a table. are kernel tests overriding something somewhere? looking into it.

Berdir’s picture

Ok, that is easy, we just need to override that services as that test defaults to database backend.

Here's why the views test is failing and why this change is a performance problem and behavior change:

MemoryBackend::set():

  /**
   * {@inheritdoc}
   */
  public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array()) {
    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($tags)', 'Cache Tags must be strings.');
    $tags = array_unique($tags);
    // Sort the cache tags so that they are stored consistently in the database.
    sort($tags);
    $this->cache[$cid] = (object) array(
      'cid' => $cid,
      'data' => serialize($data),
      'created' => $this->getRequestTime(),
      'expire' => $expire,
      'tags' => $tags,
    );
  }

The memory backend on purpose serializes the data to behave the same as non-memory backends and break references. That means this is an overhead and it also breaks the current behavior that two load calls for the same entity get the same object. You can argue whether that is good or bad, but we can't just change it. That's going to cause nasty bugs and side effects in code that currently works.

Specifically, what happens here is that we load the same node multiple times, then switch to the requested language. And then we save all of them. But that means that saving the second language will not have the changes of the first x, so saving that overrides the changes that the first language saved.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 72: entity-static-cache-1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
2.79 KB

Not a real patch, but here's a new interface for static caches, and an implementation extending memorybackend without the serialization. It'll need a new service definition then all the code/tests updating to use it, but theoretically should get us to green here after that.

catch’s picture

This will fail but uploading to have a new base to work from.

Status: Needs review » Needs work

The last submitted patch, 75: 1596472-75.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Status: Needs work » Needs review
FileSize
18.85 KB
22.75 KB

Status: Needs review » Needs work

The last submitted patch, 78: 1596472-78.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
22.19 KB
24.48 KB
22.19 KB

interdiff against #74.

Status: Needs review » Needs work

The last submitted patch, 80: 1596472-80.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
24.23 KB
1.14 KB

Status: Needs review » Needs work

The last submitted patch, 82: 1596472-82.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
24.23 KB
1.3 KB
catch’s picture

Sorry for noise, shoulda done a patch testing issue.

The last submitted patch, 84: 1596472.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 85: 1596472.patch, failed testing.

catch’s picture

Status: Needs review » Needs work

The last submitted patch, 88: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
27.42 KB
3.21 KB
27.42 KB

@Berdir pointed out in irc that $this->container is risky in tests since it can get out of sync. Switching to \Drupal fixes it.

Status: Needs review » Needs work

The last submitted patch, 90: 1596472.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
27.42 KB

Not sure what happened with the patch upload.

Mile23’s picture

This issue looks a lot like #2260187: Deprecate drupal_static()/_reset(), add StaticStorage class w/ BC

@Berdir pointed out in irc that $this->container is risky in tests since it can get out of sync. Switching to \Drupal fixes it.

Where are they out of sync? Is it in a test base class or is it in the entity system?

catch’s picture

@Mile23 that issue is trying to replace drupal_static() uses, this issue is a very specific approach for dealing with class properties, it makes most sense when read in conjunction with #1199866: Add an in-memory LRU cache and #375494: Restrict the number of nodes held in the node_load() static cache. It does offer some of the same features (like ability to reset the cache from outside the class, in this case via calls methods on the static cache service), but the use case is very different otherwise, and the node static cache issue predates even the introduction of drupal_static() by several years.

The container goes out of sync in the test - i.e. the return of \Drupal::container() and $this->container are not the same.

Mile23’s picture

Could it be the same problem as this? #2641518-38: Replace deprecated usage of entity_create('config_test') with a direct call to ConfigTest::create()

Your patch modifies EntityReferenceIntegrationTest which uses config_test, and #2641518: Replace deprecated usage of entity_create('config_test') with a direct call to ConfigTest::create() addresses config_test, so maybe config_test needs some love.

ConfigTest::create() ends up being \Drupal::entityManager()->getStorage()->getEntityTypeFromClass(ConfigTest::class). So one of the proscribed ways of instantiating an entity relies on \Drupal and the injected container being in sync, but that behavior doesn't work under some circumstances, which we can't seem to figure out.

So I'll wager that the thing that's out of sync is within the entity API.

Here's a patch that fixes EntityReferenceIntegrationTest by having getTestEntities() use $this->container to build the fixture entities.

Follow-up: Why does this patch fail if you switch to using the Entity::create() pattern in getTestEntities(), or use \Drupal in the test?

Status: Needs review » Needs work

The last submitted patch, 95: 1596472_95.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
27.42 KB

Re-uploading the passing patch from #92, I don't want this to be blocked on $this->container going out of sync with the tested site.

Mile23’s picture

Ugh. #96 passed locally.

Berdir’s picture

  1. +++ b/core/core.services.yml
    @@ -1639,3 +1639,5 @@ services:
    +  static_cache:
    +    class: Drupal\Core\Cache\StaticCache\StaticCache
    

    should we make this entity specific, so that you can have a LRU implementation for entities but maybe not for other things?

  2. +++ b/core/lib/Drupal/Core/Cache/StaticCache/StaticCache.php
    @@ -0,0 +1,65 @@
    +    // Sort the cache tags so that they are stored consistently in the database.
    +    sort($tags);
    

    what database? :)

    I know, copied, but does make even less sense here than in the parent, should we probably drop teh array nique and sort calls?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
    @@ -115,11 +133,12 @@ public function loadUnchanged($id) {
    -      $this->entities = array();
    +      // Call the backend method directly.
    +      $this->staticCache->invalidateTags(['entity_static_cache']);
    

    and this tag should then be entity type specific?, maybe a property that is defined in __construct(), so you could override it?

  4. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
    @@ -50,6 +51,13 @@ class KeyValueEntityStorage extends EntityStorageBase {
       /**
    +   * The static cache.
    +   *
    +   * @var \Drupal\Core\StaticCache\StaticCacheInterface
    +   */
    +  protected $staticCache;
    

    duplicated?

  5. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
    @@ -60,9 +68,12 @@ class KeyValueEntityStorage extends EntityStorageBase {
    +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
    +    parent::__construct($entity_type, $static_cache);
    
    +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -154,9 +155,12 @@ public function getFieldStorageDefinitions() {
    +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
    +    parent::__construct($entity_type, $entity_manager, $cache, $static_cache);
         $this->database = $database;
    

    we don't need to duplicate the fallback logic here?

catch’s picture

#1. Yes agreed. Nearly did that before. Renamed the service to entity.static_cache

#2: yeah why not, also less to do so will help remove a bit of overhead compared to just sticking stuff in a property.

#3 done.

#4 removed

#5 removed.

Status: Needs review » Needs work

The last submitted patch, 100: 1596472-100.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
26.11 KB

Rebased.

Status: Needs review » Needs work

The last submitted patch, 102: 1596472-102.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
587 bytes
26.11 KB

Missed a service rename.

claudiu.cristea’s picture

Did some cleanup, added some @todos.

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceIntegrationTest.php
@@ -82,9 +82,7 @@ public function testSupportedEntityTypesAndWidgets() {
-      /** @var \Drupal\Core\Entity\EntityStorageInterface $storage */
-      $storage = $this->container->get('entity_type.manager')->getStorage($this->entityType);
-      $entity = current($storage->loadByProperties(['name' => $entity_name]));
+      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

@@ -110,7 +108,7 @@ public function testSupportedEntityTypesAndWidgets() {
-      $entity = current($storage->loadByProperties(['name' => $entity_name]));
+      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

@@ -121,7 +119,7 @@ public function testSupportedEntityTypesAndWidgets() {
-      $entity = current($storage->loadByProperties(['name' => $entity_name]));
+      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

@@ -175,8 +173,7 @@ public function testSupportedEntityTypesAndWidgets() {
-    $entity = current($this->container->get('entity_type.manager')->getStorage(
-    $this->entityType)->loadByProperties(['name' => $entity_name]));
+    $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

However, I'm not too happy with this change, especially, with the fact that $this->container gets out-of-sync. I suspect that the problem might be in this patch. Probably entity cache gets out-of-sync?

Status: Needs review » Needs work

The last submitted patch, 105: 1596472-105.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
26.85 KB
697 bytes

Ouch!

The last submitted patch, 105: 1596472-105.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

EclipseGc’s picture

bah wrong issue... sorry ignore my post.

Berdir’s picture

Reroll, quite a few conflicts but mostly just on array syntax.

Status: Needs review » Needs work

The last submitted patch, 111: 1596472-111.patch, failed testing.