Follow-up from #1497374: Switch from Field-based storage to Entity-based storage, by @plach in #248:

There's only one thing that I'd really wish to see changed: currently the views integration code of the Field module treats every class extending the DatabaseStorageController as a SQL implementation. IMHO this is an unneeded limitation, as already pointed out above by @fago: should someone need to write an alternative SQL storage controller, she would be forced to extend
the core one with all its assumptions about the underlying implementation. I'd suggest instead to provide an interface, e.g. SqlStorageControllerInterface and make DatabaseStorageController implement it. The current pseudo-private static methods could be refactored and moved onto that interface, becoming regular methods, and then the views integration code could just check whether the storage controller implements SqlStorageControllerInterface.

I realize that those methods were not made part of the public API intentionally, but I think that since there are legal use cases for needing to retrieve some details about the storage implementation (Views integration being the most obvious), we should provide an actual API that code needing to deal with SQL can properly rely on. Doing the opposite IMHO will just "authorize" people to find the most sneaky ways to circumvent the Entity Storage API. I think a SqlStorageControllerInterface marker interface would be enough for this issue, as I guess that a proper API allowing to describe any SQL table layout would not use the static methods in the current form. Refactoring those would totally be a follow-up.

Creating this issue to discuss and implement that.

Files: 
CommentFileSizeAuthor
#6 2079019.patch12.91 KBdamiankloip
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2079019.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

catch’s picture

Priority:Critical» Major

This is fine but feels like it could go in as an API addition any time.

Berdir’s picture

Issue tags:+Entity Field API

Tagging.

tstoeckler’s picture

Note: I'm adding a SqlStorageControllerInterface in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities (I don't think it makes sense to postpone on this one, as there hasn't been much concrete progress here). IMO we should either close this issue or repurpose it to something like "Make Views use SqlStorageControllerInterface" and mark it postponed.

plach’s picture

Title:Add a SqlStorageControllerInterface to standardize special casing of the database storage controller» Make Views use SqlStorageControllerInterface
Priority:Major» Normal
Status:Active» Postponed

Makes sense

plach’s picture

Status:Postponed» Active
damiankloip’s picture

Title:Make Views use SqlStorageControllerInterface» Make Views use SqlEntityStorageInterface
StatusFileSize
new12.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2079019.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is a go at this, spoke to berdir on IRC. We can use the SqlEntityStorageInterface, which didn't exist when this issue was created.

Also removed static hardcoded calls to ContentEntitytorageController in field.views.inc.

damiankloip’s picture

Status:Active» Needs review
chx’s picture

This is exactly what I didn't want to happen when I worked with on the entity storage issue and this should not happen because this will cause people to code around the controller again. I see the warnings but that won't be enough :( we need to think harder on how to stop this sort of abuse. For example, the existence of https://drupal.org/project/efq_extra_field have driven the design of the new entity query system return value to be an unabusable integer - integer pair. And so should the existence of countless stupid code drive us to make it so there's no clean way to reach the field tables ; warnings or not.

Can we , for example , move some of this stuff under Views somehow?

chx’s picture

But, I left the issue at needs review because perhaps others will find the warnings strong enough.

yched’s picture

Not relevant to @chx's concerns, but :

SqlEntityStorageInterface already has a public getTableMapping() method. I'd think that's the object Views should be taking its informations from ? In other words, IMO TableMappingInterface should be "the official business object to turn to when you need info about the SQL layout of an entity typed stored in an SqlEntityStorageInterface".

That probably requires working a bit on the API and DX of TableMappingInterface.
That might also mean moving the logic in _fieldTableName(), _fieldSqlSchema() & friends out of ContentEntityDatabaseStorage and into a base class for TableMappingInterface.

IMO this intersects with #2274017: Make SqlContentEntityStorage table mapping agnostic - crossposting over there.

plach’s picture

@chx:

I understand and share your concerns, OTOH I think that if we don't provide an API for modules legally needing to work with SQL like Views, people will be "authorized" to find sneaky ways to do that. At least we can provide a controlled path that's part of an API geared towards storage abstraction.

@yched:

SqlEntityStorageInterface already has a public getTableMapping() method. I'd think that's the object Views should be taking its informations from ? In other words, IMO TableMappingInterface should be "the official business object to turn to when you need info about the SQL layout of an entity typed stored in an SqlEntityStorageInterface".

Yes, this is exactly what I had in mind when I first proposed it.

That probably requires working a bit on the API and DX of TableMappingInterface.
That might also mean moving the logic in _fieldTableName(), _fieldSqlSchema() & friends out of ContentEntityDatabaseStorage and into a base class for TableMappingInterface.

And this is exactly what I am doing in #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions :)

plach’s picture

Sorry, I didn't look at the patch previously, now that I did I noticed it's mainly duplicating my work in #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

damiankloip’s picture

That issue is held up though... :( is the a good step to break thing up a bit maybe? As this one has a smaller scope and conversion?

plach’s picture

I'd like to avoid postpone the other one on this. Maybe you can have a look to my branch and extract it (or I can do it :) so whatever is ready first goes in?

fago’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlEntityStorageInterface.php
@@ -23,4 +24,99 @@
+  static public function _fieldTableName(FieldConfigInterface $field);

As chx and yched mentioned those should not go into the interface - we've the table mapping in place already which tells you about
- which tables exist
- which field is stored in there
- what the column names used are and how they map

That should be everything Views needs to work with. If not, we should probable extend/improve the TableMappingInterface directly as yched suggest in #10. I do not think this is postponed on anything anymore, as the interface for getting the table mapping is in place regardless of where the code internally figures out which table is called how.

andypost’s picture

Status:Needs work» Needs review

jibran queued 6: 2079019.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 6: 2079019.patch, failed testing.

plach’s picture

Issue tags:+entity storage
attiks’s picture

We're working on a module to use remote entities directly from Drupal 8, so we implemented a new storageController that handles the calls to the backend system. We want to provide views integration as well, but this isn't possible since EntityViewsData only accepts SqlEntityStorageInterface, should this be changed to EntityStorageInterface?

Berdir’s picture

No. views in core can only deal with entities stored in the database, this issue isn't about changing that.

You need to implement your own views query plugin or wait for entity query integration.

attiks’s picture

#21 Is there an issue already for entity query integration?

chx’s picture

@attiks we do not have an issue we have a branch http://cgit.drupalcode.org/efq_views/log/?h=8.x-1.x

attiks’s picture

@chx thanks, will have a look.

colan’s picture

@attiks: Would you be so kind as to provide a link to your project? Is it a successor to Remote Entity API? If you've got something else or have thoughts, I'd appreciate it if you could comment over at #2538644: Remote entities in Drupal 8. I added remote entity support to EntityFieldQuery Views Backend in D7, and am trying to stay on top of whatever folks are working on in this area for D8. Also, come to my talk tomorrow. ;)

attiks’s picture

#25 I uploaded our code to the project page, https://www.drupal.org/project/external_entities it works best with clean rest endpoints