Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
748 bytes

Let's see what breaks.

yched’s picture

I'd be in favor of this, if we can prove that performance is not affected.

yched’s picture

Patch uses varchar(128) instead of the varchar(255) mentioned in the OP, though :-)

yched’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

yeah... I realized that we constrain UUIDs to 128 characters, too, and the only reason for why we're using VARCHAR for UUIDs is that we want to support "arbitrary strings as UUIDs" right now. (although that might change: #1805576: Add a 'uuid' database schema type)

Thus, I thought what's good enough for UUIDs must be good enough for entity IDs. Updated the summary accordingly.

sun’s picture

Apparently, the patch in #1 passed all tests. :)

So what's left? Upgrade path + Profit? :)

yched’s picture

Benchmark ? :-)

fago’s picture

If we already change it to varchar, why not to varchar(255)? Another nice use-case would be to directly use URIs as IDs, as I implemented here: http://drupal.org/sandbox/fago/1493180

sun’s picture

I don't know how to benchmark this. Field data is cached anyway, so the consequences of this change are barely noticeable.

chx’s picture

Status: Needs review » Needs work

http://drupal.org/node/555536 you must cast to string when querying if you want to this.

sun’s picture

Not sure I understand the data type problem. I think we're querying string columns with integer values elsewhere in core already (e.g., text format IDs).

If I get it right, then all you want is to add a type-cast to (string) for the entity ID in field_sql_storage, so all queries on field data tables use a string and not an integer for the entity ID?

(I can't see why this would have to be a CAST() in SQL.)

That said, all entity IDs that have been queried from entity base tables should be strings already, since you get a string '123' instead of int 123 as query result.

chx’s picture

> I think we're querying string columns with integer values elsewhere in core already (e.g., text format IDs).

It might need a review of what's happening there -- although if they are text solely then why would there be numbers?

> If I get it right, then all you want is to add a type-cast to (string) for the entity ID in field_sql_storage, so all queries on field data tables use a string and not an integer for the entity ID?

That's correct.

sun’s picture

The upgrade path from D6 to D7 converts existing text format serial IDs simply into strings. This means that upgraded text formats have IDs of "1", "2", "3", and so on, and only newly created formats on D7 have 'real' machine names as IDs (though there's technically nothing wrong with an all-numeric machine name). To my knowledge, Filter module does not type-cast the format IDs in any way before querying the database, and I'm not aware of a critical upgrade path bug or database engine incompatibility with those.

In turn, I still don't really understand the request for type-casting the entity IDs here. However, we can add the necessary tweaks to field_sql_storage module, if that is what it takes to get this issue done. ;)

pounard’s picture

I'm really worried about changing the entity id to a varchar, the field API already is really slow, it will make it even slower, especially for INSERT/UPDATE, at least while using MySQL.

Keeping an internal entity id as a int32 integer for database stored entities sound wiser than making the API more flexible. We can still keep an index table somewhere else to store a potential varchar id if necessary, and the UUID of course.

Making the SQL field storage a bit more complex but allowing it to keep transparently entity id as numeric values would be a better choice, harder but better. Drupal is not using the database right, switching yet another primary field to varchar in those table will be hell. In fact, those tables should have only numeric index/primary keys because the way it exists actually is already slow with a few thousands of entities while it could be raging fast with millions of them using only integer indexes.

EDIT: Here is actually a proof of concept that it is not *that* complex to achieve (except for EFQ): https://drupal.org/sandbox/pounard/1760694

Another worrying thing is not only the performances, but both the index memory consumption and the on-disk table size.

Danny_Joris’s picture

I work on a project called Islandora. Islandora is a set of drupal modules which provides a management and display environment for Fedora Commons. We've been pushing hard for tighter Drupal integration and serving Fedora Commons objects as entities is our next goal. One of our biggest hurdles currently is that the Fedora objects we're serving all have id's that are structured like this: "namespace:1234".
That's just our use case. I'm concerned about performance too. I'm not sure how much this would affect that.

Danny_Joris’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs work » Needs review
FileSize
692 bytes

Just a plain re-roll against HEAD for now...

moshe weitzman’s picture

IMO, allowing for non-integer IDs is an architectural improvement.

Status: Needs review » Needs work

The last submitted patch, drupal8.field-entity-id.16.patch, failed testing.

Jānis Bebrītis’s picture

I agree with this, we have limited time left to push this major change into core to get drupal8 UUID rolling.
recreated patch for 8.x branch

pounard’s picture

This will deteriorate performance with the database and fields, this is wrong to do this without refactoring the whole schema first. Field API is a bottleneck and will be even more.

Pancho’s picture

Marked #1932612: PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer (which tries to go the other route and contains a working D7 patch) a duplicate of this.
We'd need to settle which way to go and then enforce it through all subsystems.

Pancho’s picture

Status: Needs work » Needs review

Let the testbot do its job on #19.

Status: Needs review » Needs work

The last submitted patch, drupal8.field_.entity-1823494-id.19.patch, failed testing.

chx’s picture

We should look at the field and the instance and the entity controller of the instance and add a method to the entity storage controller which returns a type, for core that's sql or config and based on that just decide. Oh and throw an exception if you try to instantiate a field on an entity type where the backend type is not the same as an existing one. Most of this (especially the exception code) is in the entitystorage branch of my sandbox already http://drupal.org/node/1857558 just needs to be separated out.

chx’s picture

Issue summary: View changes

Updated issue summary.

steinmb’s picture

yched’s picture

Now that field storage is per entity type, I guess it would be relatively easy to generate the table schema for the entity_id column as either int or varchar based on some entity type metadata - but that info would need to be available somewhere...

@fago, @Berdir: maybe this could be done by looking at the definition of the 'id' field ?

Berdir’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
3.09 KB

Something like this maybe?

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -1288,6 +1288,50 @@ public static function _fieldSqlSchema(FieldInterface $field, array $schema = NU
    +    if ($id_definition->getType() == 'integer') {
    

    Shouldn't we check on IntegerInterface instead?

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -1288,6 +1288,50 @@ public static function _fieldSqlSchema(FieldInterface $field, array $schema = NU
    +    if (!$revision_id_definition || $revision_id_definition->getType() == 'integer') {
    

    Are we sure we want to support primary ids and revision ids of different types?

Berdir’s picture

Only an actual field item value property instance would be of type IntegerInterface, not the definition...

I'm fine with only supporting the same type for id and revision...

sun’s picture

Wow, that looks surprisingly simple :)

I'd generally agree with @amateescu that the revision_id data type should follow the entity_id data type, but OTOH, that assumption is not even true if you look at 'nid' and 'vid'.

If you consider that you'd replace the entity ID key definition for Nodes to point to the UUID instead of the nid, then the vid may or may not remain to be a custom/serial ID. In case the vid remains as-is, $node->id() would be a string, and $node->revisionId() would still be an integer.

Please disregard the concrete example, I hope you get the point.

Consequently, separately checking for the definitions does appear to make sense.

What else is required for this patch? :)

Status: Needs review » Needs work

The last submitted patch, 27: entity-id-schema-1823494-27.patch, failed testing.

amateescu’s picture

Only an actual field item value property instance would be of type IntegerInterface, not the definition...

Right, my bad :/

Regarding supporting different types for entity_id and revision_id, I guess the use case from #30 is somewhat valid so we shouldn't disallow it :)

What else is required for this patch? :)

I'd like to see @yched's opinion here..

Berdir’s picture

Huh, interesting fails. contact doesn't have an entity id...

Any interesting ideas? :)

We could easily add the same check as for the revision id, but in a way, that is reallly weird :) Should we check that before and don't even create those tables? We could have a null storage now, that just skip this...

yched’s picture

The code looks good to me.

"contact doesn't have an entity id"
Right... Generally speaking, that sounds like a very weird thing that would break a lot of assumptions in many places...
As to our issue here, isn't it wrong for contact entities to use FieldableDatabaseStorageController as a storage controller if they don't intend to be stored in the first place ?

Berdir’s picture

Yes it is, and it's probably going to break horribly if you try to save it :) Agree that the cleanest solution is to have a FieldableNullStorageController or something like that, that doesn't do anything.

fago’s picture

>FieldableNullStorageController
Agreed, but how is the null storage fieldable? I'd assume it's just a NullStorageController ?

>Wow, that looks surprisingly simple :)
Indeed!

+ $id_definition = $definitions[$info['entity_keys']['id']];
+ if ($id_definition->getType() == 'integer') {

That's not very solid, but getting more metadata is quite cumbersome right now, we'd have to instantiate empty field item objects, derive the main property name + check the property -- uh.
It becomes a bit easier with #2002134: Move TypedData metadata introspection from data objects to definition objects so we can save the empty object. Anyway, I think it's totally fine to get started like that and improving it with a better check later on.

sun’s picture

how is the null storage fieldable? I'd assume it's just a NullStorageController ?

Contact Message entities are not saved to the database (by core; contrib might add a storage), their entity forms are submitted only. One major design goal was to allow Message entity bundles ("contact categories") to be fieldable, so you can have multiple contact forms with different fields.

On the assumption that FieldableFooStorageController implies built-in Field API integration (as opposed to FooStorageController), the suggested FieldableNullStorageController appears to make sense?

yched’s picture

yes, i might be overlooking smthg, but not sure why FieldableNull instead of Null. When it comes to storage, "not stored" is "not stored", whether runtime entities have fields or not ?

The ViewBuilder and FormControllers do need to be aware of fields, but storage ?
(validation needs to run on form submits too, but that's taken care of by the from controller, right ?)

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
5.12 KB

Let's try this. The Null implementation actually needs a create() implementation, but we can probably move that into the base class, I don't want to touch that just yet, though.

yched’s picture

+++ b/core/lib/Drupal/Core/Entity/FieldableNullStorageController.php
@@ -0,0 +1,190 @@
+  public function create(array $values) {
+    $entity_class = $this->entityInfo['class'];
+    $entity_class::preCreate($this, $values);
+

That's 40 lines strictly copy/pasted from FieldableDatabaseStorageController, it's a bit sad, plus could easily get out of sync.

Currently there's no existing class higher up where we could share this code.
I'm a little lost in the various issues that reshuffle our storage class hierarchy, but does one of them introduce such a base class ?
ContentEntityStorageController or something ?

yched’s picture

Er, actually, why can't this create() implementation be moved up to FieldableEntityStorageControllerBase ?

Berdir’s picture

Er, actually, why can't this create() implementation be moved up to FieldableEntityStorageControllerBase ?

From #39:

Let's try this. The Null implementation actually needs a create() implementation, but we can probably move that into the base class, I don't want to touch that just yet, though.

Yes it can, but I just created the Null implementation by copying the database implementation and dropping everything that was not necessary, didn't want to bother with changing the base/parent class yet. Will do so on the next update.

fago’s picture

On the assumption that FieldableFooStorageController implies built-in Field API integration (as opposed to FooStorageController), the suggested FieldableNullStorageController appears to make sense?

hm, true. I was confused by fieldable vs extensible here, but I see that the NULL-storage should act as extensible null storage - thus, yeah it makes sense.

yched’s picture

@Berdir: doh, sorry, I looked at patch #39 but skipped the attached comment. Self wrist-slap.

No biggie, but I still don't fully understand why we we need a "null but with fileds" flavor of "null". No storage means no storage, whatever is in the thing to "not store" ?

Is it because storage controllers currently have the create() method, which needs to do stuff on fields ? If so, isn't there an existing issue that tries to refactor that, and if so should we include a @todo on that issue to turn FieldableNull into Null ?

Berdir’s picture

create() is one reason yes, but not just that. It's not temporary, Null will continue to be FieldableNull.

contact_message is a fieldable/extensible entity so it has to have a fieldable/extensible storage, even if it's a null implementation.

Both conceptually and due to technical necessity. Field::preSaveNew() calls $entity_manager->getStorageController($this->entity_type)->onFieldCreate($this);, the same goes for all those other methods, bundles are created and so on. That explodes if the storage controller for contact_message doesn't provide the corresponding methods.

chx’s picture

+  public function getQueryServiceName() {
+    // @todo: valid?
+    return NULL;

This is a problem; this is not valid. I would very strongly recommend throwing an exception that querying these are not valid otherwise you need to write a null query object implementing the querry interface, doing nothing and register it as a service and I doubt we want that.

yched’s picture

@berdir: of course, makes sense. Thanks.

Berdir’s picture

Made the null storage controller nuller and added the exception.

This should be ready apart from tests. Wondering if it's enough to just have some unit tests for the schema generation code and if it's possible.

yched’s picture

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
@@ -11,11 +11,90 @@
+  public function create(array $values) {

Yay. Then it can be removed from the FieldableDatabaseStorageController subclass ? :-)

Tests: per se, I guess unit tests should be enough here.
Then again, all our current test coverage for FieldableDatabaseStorageController is inDrupal\system\Tests\Entity\FieldSqlStorageTest, which is a DUTB

Berdir’s picture

Right, but we don't have an entity with string identifier, so we'd have to define that. I think that's easier to provide in a unit test.

The attached patch does that, just adds test coverage for this specific change, but I think it would be nice to convert more of our test coverage to unit tests later.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    --- a/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
    +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
    
    +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
    @@ -11,11 +11,90 @@
    +   * Constructs a DatabaseStorageController object.
    

    Let's not lie here.

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
    @@ -11,11 +11,90 @@
    +        throw new EntityStorageException(format_string('Missing bundle for entity type @type', array('@type' => $this->entityType)));
    

    We could use String::format here.

  3. +++ b/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
    @@ -0,0 +1,97 @@
    +  public static function getInfo() {
    

    We use @inheritdoc on getInfo as well now.

  4. +++ b/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
    @@ -0,0 +1,97 @@
    + *
    + * @see \Drupal\Core\Entity\FieldableDatabaseStorageController
    + */
    

    Let's add @group Drupal here

  5. +++ b/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
    @@ -0,0 +1,97 @@
    +  public function testView() {
    

    this function name feels wrong.

  6. +++ b/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
    @@ -0,0 +1,97 @@
    +
    +    // Make sure that the entity_id schema field if of type varchar.
    +    $this->assertEquals($schema['test_entity__test_field']['fields']['entity_id']['type'], 'varchar');
    +    $this->assertEquals($schema['test_entity__test_field']['fields']['revision_id']['type'], 'varchar');
    +  }
    

    Should we also test the other case of using a string?

Berdir’s picture

Removed the create() method in the fieldable database storage controller.

@dawehner: Thanks for the review!

Fixed 1-5

6: We have a lot of test coverage in DUBT and functional tests, I'd prefer to limit unit test coverage here to that specific use case. Otherwise we'll never stop.

Anything left to do here? :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Other than the extraneous empty line at the beginning of FieldableDatabaseStorageControllerTest::testFieldSqlSchemaForEntityWithStringIdentifier() ;-), this looks ready if green.

xjm’s picture

Berdir’s picture

The last submitted patch, 52: entity-id-schema-1823494-52.patch, failed testing.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.73 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 57: entity-id-schema-1823494-57.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
14.73 KB
699 bytes

Another try.

The last submitted patch, 59: entity-id-schema-1823494-59.patch, failed testing.

Berdir’s picture

This fixes the unit test and the incorrect revision_id check.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2a839b7 and pushed to 8.x. Thanks!

Minor fix during commit :)

diff --git a/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php b/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
index da51589..81dd807 100644
--- a/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/FieldableDatabaseStorageControllerTest.php
@@ -28,7 +28,7 @@ class FieldableDatabaseStorageControllerTest extends UnitTestCase {
    */
   public static function getInfo() {
     return array(
-      'name' => 'Fieldable database storag controller',
+      'name' => 'Fieldable database storage controller',
       'description' => 'Tests the fieldable database storage enhancer for entities.',
       'group' => 'Entity'
     );
gaele’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

According to #21

David_Rothstein’s picture

For Drupal 7, is there anything that can be done here that isn't handled by the backport of #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID instead?

Berdir’s picture

No, those two issues are not really related. That one is about not having a database exception on PostgreSQL, when you e.g. go to node/bla.

This is about actually having entity types that do not have integer as their ID (but e.g. a UUID) and making them fieldable. Which is currently not possible, because field storage tables are always created with entity_id as integer.

David_Rothstein’s picture

Right, but is it possible to safely backport that? (The suggested "backport" for this issue in #21 is basically a version of #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID instead.)

Berdir’s picture

That doesn't seem like a fix for this issue, just enforcing that IDs must be numeric, in which case this would no longer be a problem in the first place :)

An actual fix would need to be something like my patches in e.g. #27. Except we don't have that field definitions to know if the id /revision id fields are integers or not. I guess we could work around that by introducing a new flag in hook_entity_info(), something like 'entity_key_type' => array('id => 'string/integer').

I guess that would be safe to backport, because nothing would change unless you have an entity type that explicitly specifies that? If someone actually wants to work on it seems another question, I am not interested, really :)

That said, such a flag might also be a way for a 7.x fix for #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID, although *that* would break existing types that are strings :(

  • alexpott committed 2a839b7 on 8.3.x
    Issue #1823494 by Berdir, sun, Jānis Bebrītis: Field API assumes serial/...

  • alexpott committed 2a839b7 on 8.3.x
    Issue #1823494 by Berdir, sun, Jānis Bebrītis: Field API assumes serial/...
steinmb’s picture

  • alexpott committed 2a839b7 on 8.4.x
    Issue #1823494 by Berdir, sun, Jānis Bebrītis: Field API assumes serial/...

  • alexpott committed 2a839b7 on 8.4.x
    Issue #1823494 by Berdir, sun, Jānis Bebrītis: Field API assumes serial/...
plach’s picture

Version: 7.x-dev » 8.4.x-dev
Status: Patch (to be ported) » Fixed

A D7 issue was opened to take care of the port (#3212398: [D7] Field API assumes serial/integer entity IDs, but the entity system does not ), I think we can close this.

Status: Fixed » Closed (fixed)

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