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.
Comment | File | Size | Author |
---|---|---|---|
#6 | 2079019.patch | 12.91 KB | damiankloip |
Comments
Comment #1
catchThis is fine but feels like it could go in as an API addition any time.
Comment #2
BerdirTagging.
Comment #3
tstoecklerNote: 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.
Comment #4
plachMakes sense
Comment #5
plachComment #6
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #7
damiankloip CreditAttribution: damiankloip commentedComment #8
chx CreditAttribution: chx commentedThis 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?
Comment #9
chx CreditAttribution: chx commentedBut, I left the issue at needs review because perhaps others will find the warnings strong enough.
Comment #10
yched CreditAttribution: yched commentedNot 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.
Comment #11
plach@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:
Yes, this is exactly what I had in mind when I first proposed it.
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 :)
Comment #12
plachSorry, 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.
Comment #13
damiankloip CreditAttribution: damiankloip commentedThat 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?
Comment #14
plachI'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?
Comment #15
fagoAs 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.
Comment #16
andypostMaybe better to postpone this on #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions
Comment #19
plachComment #20
attiks CreditAttribution: attiks at Attiks commentedWe'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?
Comment #21
BerdirNo. 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.
Comment #22
attiks CreditAttribution: attiks at Attiks commented#21 Is there an issue already for entity query integration?
Comment #23
chx CreditAttribution: chx commented@attiks we do not have an issue we have a branch http://cgit.drupalcode.org/efq_views/log/?h=8.x-1.x
Comment #24
attiks CreditAttribution: attiks at Attiks commented@chx thanks, will have a look.
Comment #25
colan@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. ;)
Comment #26
attiks CreditAttribution: attiks at Attiks commented#25 I uploaded our code to the project page, https://www.drupal.org/project/external_entities it works best with clean rest endpoints
Comment #33
Hanno CreditAttribution: Hanno commentedMoving to 9.x as the chance we will implement this in 8 is small.. Feel free to move back to 8 if not.
Comment #35
xjmThis is a minor-only addition. Since 8.9.x is now in beta, I'm moving this to 9.1.x. Thanks!
Comment #38
jsibley CreditAttribution: jsibley commentedSince it has been 2 years since any update, should we assume that this will not be changed?
Thank you.
Comment #40
lukusJust checking to see if there's an update on this issue.
Comment #41
daffie CreditAttribution: daffie commented@lukus: If this issue is important to you, then maybe you can create the patch/MR to make it happen. I will help you by reviewing your work.
Comment #44
janes_p CreditAttribution: janes_p as a volunteer commented#38 +1
Are there any activities / plans for this issue?