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.

CommentFileSizeAuthor
#6 2079019.patch12.91 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
12.91 KB

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

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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

Hanno’s picture

Version: 8.6.x-dev » 9.x-dev

Moving to 9.x as the chance we will implement this in 8 is small.. Feel free to move back to 8 if not.

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

xjm’s picture

Version: 9.0.x-dev » 9.1.x-dev

This is a minor-only addition. Since 8.9.x is now in beta, I'm moving this to 9.1.x. Thanks!

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jsibley’s picture

Since it has been 2 years since any update, should we assume that this will not be changed?

Thank you.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lukus’s picture

Just checking to see if there's an update on this issue.

daffie’s picture

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

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

janes_p’s picture

#38 +1
Are there any activities / plans for this issue?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.