Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #N
Problem/Motivation
ConfigEntityBase calls $storage_controller->getQuery()
, but without checking for ConfigStorageControllerInterface first. This prevents any other entity storage from being used.
Additionally, the other storage controllers bypass the getQueryServicename() method, and just use \Drupal::entityQuery() directly.
Proposed resolution
Move getQuery() to EntityStorageControllerInterface, and use it.
Remaining tasks
N/A
User interface changes
N/A
API changes
API change: All entity storage controllers must have a getQuery() method. This is mitigated by the default implementation on EntityStorageControllerBase.
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 4.2 KB | dawehner |
#14 | entity_get_query-1862300-14.patch | 24.38 KB | dawehner |
Comments
Comment #1
gddComment #2
tim.plunkettComment #3
tim.plunkettReopening. Because of some discussion on twitter, I tried to write a key-value entity storage controller, and I almost succeeded too.
Except ConfigEntityBase calls getQuery(), which is only on ConfigStorageControllerInterface.
Yet that is a concept that is part of every entity. getQueryServicename() is on the interface, and must be implemented by every storage controller.
Comment #5
tim.plunkettThat, but working.
Comment #6
tim.plunkettThis is needed by the KV backend I wrote today https://drupal.org/sandbox/timplunkett/2208541
Comment #7
tim.plunkettComment #8
YesCT CreditAttribution: YesCT commentedreviewing this.
Comment #9
tim.plunkettResponding to some point YesCT made on IRC.
Comment #10
YesCT CreditAttribution: YesCT commentedI dont totally get it all yet, but the issue summary makes more sense and the patch looks good afaik.
Comment #12
tim.plunkettNeeded to update the unit test. And it turns out that $$entity_manager doesn't work so well.
Comment #13
YesCT CreditAttribution: YesCT commentedReviewed this. (well discussed it in irc some more and tried to understand it.)
ConfigStorageController still has a getQuery method, but its on EntityStorageControllerBase instead of having its own implementation.
EntityStorageControllerInterface says that things need to implement getQuery
ConfigStorageControllerInterface extends EntityStorageControllerInterface
so, the getQuery for config things, is using the generic getQuery defined on EntityStorageControllerBase
and it gets its special config stuff because
EntityStorageControllerInterface already required getQueryServicename()
ConfigStorageController implements getQueryServicename() { return 'entity.query.config'; }
and other things that extend EntityStorageControllerInterface have their own getQueryServicename, like
DatabaseStorageController implements getQueryServiceName() { return 'entity.query.sql'; }
-----------
Since BlockFormController already has a storage controller, might as well use it. It was changed in the patch to use getQuery() instead of a Query\QueryFactory.
MenuForm also.
So,
I tried to look to see if there were any other places like CommentAdminOverview..
Not sure if this was the best way, but did
And then opened each class.
Ones that do not have a storage controller already, are not worth injecting one, so left them just using the QueryFactory directly. (like FilterFormatFormControllerBase)
But did find two that already had a storage controller, so might as well use getQuery with that, and then we dont need the QueryFactory (it wasn't being used for anything else besides getting the query).
OpmlFeedAdd
DateFormatFormBase
There didn't seem to be any phpunit tests to update.
Running
./vendor/bin/phpunit --strict
before and after the changes yeilded the same results:
OK (4578 tests, 8197 assertions)
Comment #14
dawehnerThe most important point: The implementation should really not bypass the QueryFactory, as this is the place which controls the logic.
On top of it the getQuery method on the storage controller is just a shortcut in case you already have the storage controller available. Based on that I reverted the change in the menu controller,
as it did not needed the menu storage controller before
Comment #15
tim.plunkettThanks @dawehner, I wasn't really sure about that one, I agree with reverting it.
I'd RTBC if I hadn't written the rest of it :)
Comment #16
jibranNice clean up let's fix this.
Comment #18
tim.plunkettThat was just a random segfault, still RTBC
Comment #19
tim.plunkett14: entity_get_query-1862300-14.patch queued for re-testing.
Comment #21
tim.plunkett14: entity_get_query-1862300-14.patch queued for re-testing.
Comment #22
tim.plunkettOkay, two random segfaults later, this is green again.
Comment #23
webchick¡Ay, caramba!
Committed and pushed to 8.x. Thanks!
Comment #24
tim.plunkettThanks!