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.

Comments

heyrocker’s picture

Component: entity system » configuration entity system
Issue tags: +Configuration system
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Postponed » Closed (won't fix)
tim.plunkett’s picture

Issue summary: View changes
Status: Closed (won't fix) » Needs review
Issue tags: +beta target
FileSize
11.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php. View

Reopening. 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.

Status: Needs review » Needs work

The last submitted patch, 3: entity-get-query-1862300-3.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,834 pass(es). View
1.36 KB

That, but working.

tim.plunkett’s picture

This is needed by the KV backend I wrote today https://drupal.org/sandbox/timplunkett/2208541

tim.plunkett’s picture

Issue tags: +KV Entity Storage
YesCT’s picture

reviewing this.

tim.plunkett’s picture

Title: Decouple ConfigEntityBase and ConfigStorageController » Move ConfigStorageController::getQuery() to EntityStorageControllerInterface
Issue summary: View changes
FileSize
20.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,335 pass(es), 81 fail(s), and 34 exception(s). View
9.67 KB

Responding to some point YesCT made on IRC.

YesCT’s picture

I dont totally get it all yet, but the issue summary makes more sense and the patch looks good afaik.

Status: Needs review » Needs work

The last submitted patch, 9: entity-get-query-1862300-6.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
FileSize
3.19 KB
23.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,101 pass(es). View

Needed to update the unit test. And it turns out that $$entity_manager doesn't work so well.

YesCT’s picture

FileSize
27.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,064 pass(es). View
4.92 KB

Reviewed 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

$ ag "__construct\(" core/* | grep QueryFactory

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)

dawehner’s picture

FileSize
24.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,358 pass(es). View
4.2 KB

The 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

tim.plunkett’s picture

Thanks @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 :)

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Nice clean up let's fix this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: entity_get_query-1862300-14.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

That was just a random segfault, still RTBC

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: entity_get_query-1862300-14.patch, failed testing.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Okay, two random segfaults later, this is green again.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

¡Ay, caramba!

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Thanks!

Status: Fixed » Closed (fixed)

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