Updated: Comment #203

This is being coded in the D8MI - Entity Translation sandbox.

Problem/Motivation

  • An entity type's base field definitions are to a large part duplicated in the providing module's hook_schema() implementation which is needed to provide the database schema for the entity type. Especially since field items provide their own schema, this has become completely unnecessary
  • Because the field definitions and the schema are completely separate systems changes to field definitions are not reflected in the schema and thus are only supported if the storage is cared for separately
  • As this has storage implications it is not easily possible to change entity types from translatable to non-translatable or vice-versa and from revisionable to non-revisionable or vice-verca
  • Even in Drupal 7 it is very cumbersome to properly declare the schema for revisionable entities which is why few contrib entity types are properly revisionable. With Drupal 8 and the notion of entity translation in core, the burden for declaring the schema for revisionable and translatable entities is even higher.

Proposed resolution

  • Introduce a SqlStorageInterface for SQL-based entity storages. It contains a getTableMapping() method which returns a TableMappingInterface value object which contains a list of database fields per table for the entity type. It also stores the relationship of the database fields to the entity field definition. Both entity field definitions which provide multiple database fields (i.e. multi-column fields) and database fields that do not correspond to entity field definitions (i.e. denormalized database fields such as 'default_langcode') are supported. Because of this complexity a value object with utility methods is chosen rather than a simple array. As the table mapping is needed during querying and saving the respective code is put directly on the storage class. The table mapping has a switch statement that returns a different table structure, depending on whether the revisionable or not, translatable or not, or any combination of that. This will allow for easier refactoring once we split ContentEntityDatabaseStorage into different classes for the different types.
  • Introduce a ContentEntitySchemaHandlerInterface that uses the entity type, the entity type's field definitions (and their respective schema arrays) and the table mapping (see above) to generate the full schema array for all of the entity type's schema in a getSchema() method. As this code is only used during module install (currently) this code lives in a separate class, the ContentEntitySchemaHandler. To hide this code organization detail from the outside ContentEntityDatabaseStorage implements ContentEntitySchemaHandlerInterface. The getSchema() method of ContentEntityDatabaseStorage simply forwards the call to ContentEntitySchemaHandler, which it lazy-instantiates directly (i.e. using the class name directly). While using direct instantiation in a class is generally discouraged neither of the 3 methods we have for swappability in core are applicable here:
    • We cannot make it a service as the schema handler needs the entity type injected. Therefore we would need per-(content)-entity-type services, but this is not possible as you cannot call EntityManagerInterface::getDefinitions() during container building
    • We could make it a top-level entity controller/handler but the whole concept of schema is bound to the specific implementation of the storage. I.e. there might be storages which do not need a schema at all and, thus, it would be semantically incorrect to expose this as part of the API
    • The entity query factory factory pattern was invented to allow for this sort of easy swapping per entity type. Following that pattern we could introduce a getSchemaHandlerServiceName() method on SqlStorageInterface. Then we could use the storage to figure out which schema handler to use. But since the storage needs the schema handler (constructor) injected there is a chicken and egg problem: Between the storage and the schema handler each one needs the other to be instantiated. This could be solved by using setter injection but we don't really have a way to use setter injection for entity controllers/handlers.

    The fact that we have this separation is quite neat as entity types do not actually have to override the schema handler but can just provide additional schema tables that are not (in part not yet) auto-generated in the storage class.

  • Entity queries are updated to use the table mapping instead of drupal_get_schema()
  • Entity saves and loads are updated to use the table mapping instead of drupal_get_schema() and drupal_write_record()
  • Because entity type tables are no longer registered in hook_schema() DrupalUnitTestBase::installSchema() fails for entity type tables. Therefore a DrupalUnitTestBase::installEntitySchema() is introduced.

Remaining tasks

  • More reviews?

User interface changes

None.

API changes

  • Modules should/can no longer declare their entity type schema in hook_schema(). Instead the schema is generated automatically for them. In case they want to specify additional schema information (denormalization tables, indexes, ...) they need to provide a custom storage class and override getSchema(). If they do want to avoid this behavior for some reason they can provide a storage controller which does not implement ContentEntitySchemaHandlerInterface
  • DrupalUnitTestBase::installSchema() no longer works for entity tables.

API additions

  • Introduce a TableMappingInterface
  • Introduce a SqlStorageInterface
  • Introduce a ContentEntitySchemaHandlerInterface
  • Introduce a DrupalUnitTestBase::installEntitySchema()

Follow-ups

Follow-ups needed if/when this is committed:

Blockers of this issue

-

Spin off to separate issues

Good ideas of separate parts discussed or found during this issue, but not blocking this, and could be done without committing this (not follow-ups)

Files: 
CommentFileSizeAuthor
#415 d8_entity_schema.patch301.55 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,792 pass(es).
[ View ]
#411 d8_entity_schema.interdiff.txt4.25 KBfago
#411 d8_entity_schema.patch301.56 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,786 pass(es).
[ View ]
#409 d8_entity_schema.interdiff.txt1.12 KBfago
#409 d8_entity_schema.patch299.11 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,797 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#402 entity-schema-2183231-402.patch297.99 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,847 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
entity-schema-2183231-400.patch7.62 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#392 entity-schema-auto-2183231-392.patch297.96 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-schema-auto-2183231-392.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#386 dumpfile-PATCH.sql_.txt2.9 MBjessebeach
#386 dumpfile-HEAD.sql_.txt2.91 MBjessebeach
#191 Entity_Schema_getSchema.png116.48 KBjessebeach

Comments

tstoeckler’s picture

Status:Active» Needs work
StatusFileSize
new3.51 KB
new1.64 KB
new27.66 KB
new0 bytes

So I started some work on this recently.

I used the following approach: I added a getSchema() method to FieldableDatabaseStorageController. I started by simply looping over baseFieldDefinitions() of the entity type and putting that together. I then diffed the generated schema with the current schema in hook_schema(). That made me notice a number of missing pieces in the entity field API which is why I opened the following issues:

The current status is a WIP and more of an initial stab at this. @plach's comment in #1498720-34: [meta] Make the entity storage system handle changes in the entity and field schema definitions lead to post this now, though, rather than later in order not to duplicate any efforts.

I'm providing the following files:

  • the *-gets-schema.patch is the actual getSchema() method in FieldableDatabaseStorageController
  • the *-diff-script.php is the script I used to diff the two schemas
  • the *-schema.diff is the output of the above script
  • the *-schema-changes.patch is a bunch of minor changes to the existing hook_schema() implementations to avoid inconsistencies and reduce the size of the diff generated by the script

Note that I'm not at all bent on following this approach any further it just proved useful to me in finding out how our schemas are currently structured.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
new8.4 KB
new13.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es).
[ View ]

Here's a new patch. With this patch I am able to properly generate the entire schema of the various entity_test_* entity types. I chose entity_test specifically so we can proceed quicker on this, as there are some minor issues that remain: So far I have ignored a bunch of non-critical schema keys completely: 'description', 'default', 'not null', 'size', 'unsigned'. The first ones are actually not very hard to fix but the first ones are very inconsistent between our current hook_schema() and our field definitions. So we should figure that out in separate issues, but hopefully proceed here nonetheless.

Again, attaching the (updated) diff script that I used to diff the old and the new schema and the actual diff output.
A couple of notes for that diff output:
1. While a bunch of things are being added in the diff, they all make sense and show incompleteness and deficiencies in the current entity_test schema.

2. In order to get that diff output I had to cheat a bit and apply the following patches:
* #2209071: StringItem should cast the 'length' schema value to (int)
* #2209049: Allow field schemas to contain unique keys

3. Do not turn on entity_reference module when testing this, as that will mess with the schema of entity reference fields. Will open an issue for that as well.

The patch (mostly) contains the FieldableDatabaseStorageController::getSchema() method.

The next step would be actually using that to install the schema. That plays into #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

But this could now seriously use some reviews!

tstoeckler’s picture

StatusFileSize
new42.47 KB
new39.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Opened #2209981: ConfigurableEntityReferenceItem should not mess with the field schema for the problem mentioned above.

New patch, which actually *uses* the automated schema for the entity_test entity types. It contains a couple hacks that we'll eventually want to remove, notably:
* Manually installing the schema in hook_install()
* Manually uninstalling the schema in hook_uninstall()
* Allow to pass in the schema into drupal_write_record().
But this gets us going.

This also reveals a problem, that I've noticed before but ignored so far: Schema defaults. Those are currently almost consistently missing from the field item schemas. Will open an issue to discuss that.

Let's see how this goes.

Because I intended the whole getSchema() function due to the static caching, I used -w for the interdiff, so that won't apply.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new15.16 KB
new37.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

So I apparently my logic which field gets saved into which table was wrong: Only translatable fields end up in the data table(s). I was also trying to save the values of computed fields so far, which wasn't working out very well. The former issue led to rewrite most of the getSchema() method, but that makes it much more readable now. It still needs inline docs (lacking almost entirely now), but it looks OK, to my mind. I can install with this at least.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new895 bytes
new37.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183231-8-entity-schema-auto.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Crossing my fingers that that was the only problem...

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new37.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,909 pass(es), 2,106 fail(s), and 435 exception(s).
[ View ]

Hmm... must have been a context change, no merge conflicts.

xjm’s picture

tstoeckler’s picture

Status:Needs work» Needs review
Issue tags:+drupaldevdays
StatusFileSize
new41.87 KB
new32.88 KB
new57.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

OK, discussed this with @plach, @fago and @Berdir today at Drupal Dev Days. Here's a new patch that implements the EntitySchemaBuilder that @plach proposed in #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions. As discussed I added this as a separate service.

We also discussed making it a top-level controller but decided against this because this would leak the concept of 'schema' into the top-level API, while it is really a very storage-related concepts that might or might not apply depending on the storage controller. The fact that we have to pass around $entity_type, however, into every single method of that class, really feels like we should have per-entity-type instances for this. That would allow to pass in the entity type in the constructor and then just reference it everywhere instead of passing it around. Not sure.

This also includes a temporary shiv to fall back to hook_schema() implementations if they still exist. That's because e.g. Node's schema does not at all comply with its field definitions. That depends at least on #2111887: Regression: Only title (of base fields) on nodes is marked as translatable but there might be similar things for other entity types. Anyway, getting this patch green would allow us to progress quicker here, and then figuring out the rest of the entity types in parallel.

So this patch (just like the previous ones) just removes entity_test_schema(). Let's see. I can save nodes successfully at least...

Providing two (consecutive) interdiffs, but it's pretty much a full rewrite, so it might make more sense to review the patch in its entirety.

tstoeckler’s picture

The reason this fails install is that by ditching drupal_write_record() we lost the auto-serializing support for fields that have 'serialize' => TRUE in their schema. Shortcut entities have the 'route_parameters' field which is an array, so when saving that things blow up. So we somehow need to bring that back.

Also I forgot the actual installation of the dynamic schema.... *slapsforehead* thanks @plach for pointing that out.

plach’s picture

This is already looking very good, +1 for per-entity-type services. Aside from that:

  1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -113,6 +145,11 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
    +    if ($base_table = $this->entityType->getBaseTable()) {

    @@ -120,12 +157,14 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
    +      if ($this->revisionTable) {

    I think we should remove tables from the entity type definition: they don't belong there, they are a detail that matters only for the storage controller. The table name logic should be hard-coded (aside from allowing temporary table names). We may want to introduce a different key to specify the entity is not stored, but I guess it wouldn't specify a storage controller then.

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -120,12 +157,14 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
    +    // @todo Replace this with a check for the 'language' entity key when
    +    //   https://drupal.org/node/2143729 has landed.

    Ok, but see above.

  3. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilder.php
    @@ -0,0 +1,383 @@
    +          if (FALSE !== $key = array_search($field_name, $entity_keys)) {

    Please no, it took me 30 seconds to parse this :)

  4. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilder.php
    @@ -0,0 +1,383 @@
    +  protected function getTables(ContentEntityType $entity_type) {

    As I was saying above I think we should hard-code these.

  5. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilderInterface.php
    @@ -0,0 +1,47 @@
    +/**
    + * Created by PhpStorm.

    +++ b/core/lib/Drupal/Core/Entity/SchemaStorageControllerInterface.php
    @@ -0,0 +1,22 @@
    + * Created by PhpStorm.

    PhpStuff :)

  6. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilderInterface.php
    @@ -0,0 +1,47 @@
    +  public function getAllSchema(ContentEntityType $entity_type, array $field_definitions);
    +
    ...
    +  public function getTableSchema(ContentEntityType $entity_type, array $field_definitions, $table_key);

    +++ b/core/lib/Drupal/Core/Entity/SchemaStorageControllerInterface.php
    @@ -0,0 +1,22 @@
    +  *   A schema array for all of the entity type's tables.

    Since this is a generic API would it make sense to avoid the "table" word? I guess the returned array shouldn't be required to be a valid Schema API array. This would be the case only for controllers implementing the SqlStorageInterface. Maybe just implementing on getSchema() and just returning the full array?

  7. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
    @@ -9,433 +9,34 @@
    -function entity_test_schema() {

    Recreating the schema would help making entity tests pass :)

  8. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
    @@ -134,7 +134,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setSetting('max_length', 32);

    Why is this needed?

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new23.27 KB
new5.24 KB
new75.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Following changes:

- Fixed the serialization problem:
- This forced me to remove one very strange line from Shortcut::getRouteParams(), which I do not understand how it currently works anyway, but...

- Added automatic installation and uninstallation to ModuleHandler::instal()/uninstall()

- In order to fix some of the tests (or attempt that, at least) added a DrupalUnitTestBase::installEntitySchema() as DUTB::installSchema() doesn't work with auto-generated entity tables anymore.

- Fix some weirdnesses in the entity API tests. Most notably they are working with a field that's completely undefined, i.e. no base field definition?! I must be missing something...

- Renamed SchemaStorageControllerInterface to SqlStorageControllerInterface per #2079019: Make Views use SqlEntityStorageInterface. We should postpone and repurpose that issue, I think.

Regarding the review above:
- Fixed 3. I changed it to a in_array() + array_search() combination which is one function call more, but should be more readable I think.
- Fixed 5.
- 7. should be fixed per the above.
- 8. is "needed" to make the auto-generated schema match the current one. We can of course remove that, if wanted.

Still @todo.
- Fix the 1000s of tests that will probably fail. (But hopefully this will install now...)
- Investigate using per entity type services.
- Investigate hardcoding the entity tables in the storage controller. (1., 2., 4., of the review above). I agree with this point but I'm not yet sure how big the scope of that change will be.

plach’s picture

If it turns out that removing table keys from entity type annotation is too big of a change, probably it make sense to defer that to a follow-up.

plach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -305,12 +309,16 @@ protected function attachPropertyData(array &$entities) {
    +        $data_column_names = array_flip(array_diff(array_keys($data_revision_schema['fields']), array_keys($base_schema['fields'])));

    Just a reminder: we should investigate whether we need an actual schema array or a lighter data-structure is enough. Maybe we can add a @todo?

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -442,11 +450,14 @@ protected function buildQuery($ids, $revision_id = FALSE) {
    +    $base_schema = $this->schemaBuilder->getTableSchema($this->entityType, $field_definitions, 'base_table');

    I guess per-entity-type services is the way: we should pass this information just once. @Crell was pointing out we should not require storage controller to have a schem builder definied, though.

  3. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -726,7 +737,12 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_key
    +      $serialize = !empty($definitions[$name]->getColumns()[$definitions[$name]->getMainPropertyName()]['serialize']);

    Should we consider a follow-up to add a serialize() method (actual name TBD) to the field item class? Regular items would just return their value while valuess needing to be serialized would get an automatic serialize() call.

  4. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -662,6 +665,25 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        // Install entity type tables.

    I don't think we should bake this logic into the module handler, it's a new dependency I don't think we *need* to introduce. What about implementing entity_modules_installed()?

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiTest.php
    @@ -25,16 +25,10 @@ public static function getInfo() {
    +    $this->installEntitySchema('entity_test_mul');
    +    $this->installEntitySchema('entity_test_mulrev');

    This is really a cool change :)

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new1.96 KB
new75.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 42,061 pass(es), 620 fail(s), and 531 exception(s).
[ View ]

Ohh, I will (not) miss you drupal_write_record(). You had some nifty features, however...

Let's see if we can get some useful test output this time.

This does not address #21 yet. Thanks for the review, though! Very much appreciated.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new83.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183231-23-entity-schema-auto.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This fixes 1.,2. and 4. from above. This now does not actually use per-entity-type services, as you cannot call EntityManager::getDefinitions() during container building as that will blow up. I then explored implementing a factory-factory logic similar to entity query, so that each storage controller can specify the service name of its schema builder. That however did not work because it introduced a circular dependency issue:
- In order to instantiate the schema builder you need to call the storage to call the getSchemaBuilderServiceName() method.
- In order to instantiate the storage controller, you need the schema builder, as that gets injected.

So this now just introduces a schemaBuilder() method which manually instantiates the schema builder on demand. It's not as nice architecturally, but it's overridable and its lazy-load and it works :-)
Injecting the entity type greatly simplifies the logic in EntitySchemaBuilder. @plach you were right from the beginning that it's better this way, I just needed to see this for myself :-)

I like 3. But let's do that in a follow-up.

Sorry, no interdiff as I fucked my branch up locally. It's pretty much a rewrite anyway, though.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new6.42 KB
new81.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Ahh, the revision issue conflicted with this. HEAD moves fast this week.... :-)

Also removed some bogus field definitions that I had added. @plach said (correctly) that those don't make sense.

And I changed EntitySchemaBuilder to account for $field_definition->hasCustomStorage().

Let's see.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new4.25 KB
new80.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 49,838 pass(es), 1,842 fail(s), and 1,446 exception(s).
[ View ]

So in my many refactorings I seemingly removed a necesseray check which was causing the fatal.

Also removes a bunch of leftover stuff that I saw in self-reviewing the patch above.

*crossingfingers*

jessebeach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilderInterface.php
    @@ -0,0 +1,41 @@
    +/**
    + * Created by PhpStorm.
    + * User: tobias
    + * Date: 24.03.14
    + * Time: 14:23
    + */

    Some boilerplate generation spillover.

  2. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilderInterface.php
    @@ -0,0 +1,41 @@
    +  /**
    +   * Gets the full schema array for a given entity type.
    +   *
    +   * @param \Drupal\Core\Entity\ContentEntityType $entity_type
    +   *   The entity type to return the schema for.
    +   * @param \Drupal\Core\Field\FieldDefinitionInterface[] $field_definitions
    +   *   The field definitions of the entity type, keyed by field name.
    +   *
    +   * @return array
    +   *   A schema array for the entity type's tables.
    +   */
    +  public function getSchema();

    Params need to be included in the function signature and given default values. Objects should be identified by Interface. There's a use statement missing for \Drupal\Core\Field\FieldDefinitionInterface

  3. +++ b/core/lib/Drupal/Core/Entity/SqlStorageControllerInterface.php
    @@ -0,0 +1,15 @@
    +/**
    + * @file
    + * Contains \Drupal\Core\Entity\SchemaStorageInterface.
    + */
    +
    +namespace Drupal\Core\Entity;
    +
    +use Drupal\Core\Entity\Schema\SchemaBuilderInterface;
    +
    +/**
    + * A common interface for SQL-based storage controllers.
    + */
    +interface SqlStorageControllerInterface extends EntityStorageControllerInterface, SchemaBuilderInterface {
    +}

    The @file info is wrong here.

Going to do some manual testing now.

plach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -8,7 +8,10 @@
    +use Drupal\Core\Entity\Schema\SchemaBuilder;
    +use Drupal\Core\Entity\Schema\SchemaBuilderInterface;
    use Drupal\Core\Language\Language;

    Can we rename those ContentEntitySchemaHandler(Interface)? I just realized that schema builder is the name I wanted to give to the service actually triggering (re)builds (see #1498720-48: [meta] Make the entity storage system handle changes in the entity and field schema definitions). The prefix Content is to communicate the fact we are dealing with content entities.

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -247,6 +297,8 @@ protected function mapFromStorageRecords(array $records) {
    +      $table_mapping = $this->getTableMapping();

    @@ -391,6 +444,8 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
    +    $table_mapping = $this->getTableMapping();

    Can we move these close to the lines where we are actually using it?

  3. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -657,41 +723,64 @@ protected function savePropertyData(EntityInterface $entity, $table_key = 'data_
    +        // Since this is not a multi-column field we can assume there is only one

    80 chars

  4. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +

    bogus blank line

  5. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +   * Constructs an SchemaBuilder.

    "a SchemaBuilder"

    I think :)

  6. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +      elseif ($base_schema = drupal_get_schema($tables['base_table'])) {

    Please let's add a comment here: every time I see drupal_get_schema() I think "WTF?!" :)

  7. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +        // @todo This will become unnecessary once https://drupal.org/node/2143729
    ...
    +        // $tables and $entity_keys instead of building the array manually avoids
    ...
    +          // 'default_language' on the data table and the revision data table, if

    80 chars

  8. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +        $other_definitions = array_diff_key($storable_definitions, array_flip($entity_keys) + array('langcode' => TRUE));
    +        $other_fields = array_keys($other_definitions);

    $other_ is a really poor variable name prefix :)
    I had to read the code multiple times to understand what was going on, what about $data_?

  9. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +        $table_mapping = array_fill_keys(array_keys($tables), array_values($entity_keys));

    I don't get what this is doing: shouldn't we use array_combine here?

  10. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +          $table_mapping = array_merge_recursive($table_mapping, array_fill_keys(array_keys($tables), array('language')));

    Actually the base table does not hold the language column.

  11. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +
    ...
    +

    +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilderInterface.php
    @@ -0,0 +1,41 @@
    +use Drupal\Core\Entity\ContentEntityType;
    +

    bogus blank line

  12. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +    $tables = $this->getTables($this->entityType);

    bogus parameter

  13. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +      // @todo Descriptions, unique keys, indexes and foreign keys.

    We need at very least indexes/unique keys before this can be committed. We probably need a spearat method like addTableIndexes() to make it overridable per-entity-type. Or maybe those can go into the processTable* methods?

  14. +++ b/core/modules/entity/entity.module
    @@ -129,16 +130,55 @@ function entity_entity_bundle_delete($entity_type_id, $bundle) {
    +function entity_module_preinstall($module) {

    This looks very neat now :)

  15. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
    @@ -9,433 +9,36 @@
    +  foreach ($entity_type_ids as $entity_type_id) {
    +    $storage_controller = $entity_manager->getStorageController($entity_type_id);
    +
         // Auto-create fields for testing.
    -    entity_create('field_config', array(
    +    $storage_controller->create(array(
           'name' => 'field_test_text',
    -      'entity_type' => $entity_type,
    +      'entity_type' => $entity_type_id,
    +      'user_id' => 1,
           'type' => 'text',
           'cardinality' => 1,
           'translatable' => FALSE,
         ))->save();
    -    entity_create('field_instance_config', array(
    -      'entity_type' => $entity_type,
    +    $storage_controller->create(array(
    +      'entity_type' => $entity_type_id,
    +      'user_id' => 1,
           'field_name' => 'field_test_text',
    -      'bundle' => $entity_type,
    +      'bundle' => $entity_type_id,
           'label' => 'Test text-field',
         ))->save();

    -    entity_get_form_display($entity_type, $entity_type, 'default')
    +    entity_get_form_display($entity_type_id, $entity_type_id, 'default')

    This change really does not look right to me: you are using the test entity storage controller to create field/instance entities...

  16. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
    @@ -94,6 +95,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['field_test_text'] = FieldDefinition::create('text')

    This does not look right either: the definition will be provided by the Field module once we create the related field_config/field_config_instance entities.

plach’s picture

I like 3. But let's do that in a follow-up.

Yep, it's totally follow-up material :)

plach’s picture

Assigned:Unassigned» plach

Working a bit on this.

tstoeckler’s picture

plach’s picture

Assigned:plach» Unassigned
Status:Needs work» Needs review
StatusFileSize
new78.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,520 pass(es), 104 fail(s), and 55 exception(s).
[ View ]

Let's see how many failures we have now...

mauzeh’s picture

Status:Needs work» Needs review
StatusFileSize
new81.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,513 pass(es), 62 fail(s), and 22 exception(s).
[ View ]

Fixed some more tests

mauzeh’s picture

posting interdiff

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new11.5 KB
new92.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,705 pass(es), 23 fail(s), and 8 exception(s).
[ View ]

More fixes from my and @mauzeh's branches.

plach’s picture

@mauzeh:

Reviewing your code, a couple of notes:

  1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/DrupalUnitTestBaseTest.php
    @@ -19,7 +19,7 @@ class DrupalUnitTestBaseTest extends DrupalUnitTestBase {
    +  public static $modules = array('entity', 'entity_test', 'entity_install_schema_test');

    @@ -33,8 +33,8 @@ public static function getInfo() {
    +    $modules = array('entity', 'entity_test', 'entity_install_schema_test');
    +    $tables = array('entity_test', 'entity_install_schema_test');

    @@ -43,7 +43,9 @@ function testSetUp() {
    +    foreach($tables as $table){
    +      $this->assertFalse(db_table_exists($table), "'$table' database table not found.");
    +    }

    Do we need still need entity/entity_test modules here?

  2. +++ b/core/modules/simpletest/tests/modules/entity_install_schema_test/entity_install_schema_test.install
    @@ -0,0 +1,50 @@
    +function entity_install_schema_test_schema(){
    +  $schema['entity_install_schema_test'] = array(
    ...
    +      'entity_install_schema_test_index' => array('sid', 'dummy_integer'),

    I think we should call the module "simpletest_test" or something. It has nothing to do with entities so the current name is a bit misleading. By calling this way we allow it to be used also for other purposes.

  3. +++ b/core/modules/simpletest/tests/modules/entity_install_schema_test/entity_install_schema_test.module
    @@ -0,0 +1,25 @@
    +function entity_install_schema_test_permission() {

    Why do we need these permissions?

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new19.09 KB
new92.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,889 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

This might be green finally.
(crossing-fingers)

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.87 KB
new94.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,818 pass(es).
[ View ]

That was unfair, marvin :(

plach’s picture

StatusFileSize
new32.49 KB
new92.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This refactors the whole stuff a bit: as discussed yesterday with Tobias, we are moving the getTableMapping() method on the storage, so we can properly lazy load the schema builder (which will grow bigger in the follow-ups). Interfaces have been refactored accordingy.

plach’s picture

StatusFileSize
new92.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,976 pass(es).
[ View ]

Sorry, wrong patch. The interdiff in #45 is good.

Berdir’s picture

Quick review...

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -133,6 +220,173 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
    +
    +        $this->getLayoutType();
    +        switch (TRUE) {
    +          // The base layout stores all the base field values in the base table.
    +          case $this->layoutType == static::LAYOUT_BASE:
    +            $table_mapping[$this->baseTable] = $this->processFields(array_merge($key_fields, array_diff($storable_fields, $key_fields)));
    +            break;

    I don't even...

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -582,7 +845,17 @@ public function save(EntityInterface $entity) {
    +        // Even if this is a new entity, the ID key might have been set in which
    +        // case we should not override the provided ID. An empty value for the
    +        // id is interpreted as NULL and thus overriden.
    +        if (empty($record->{$this->idKey})) {
    +          $record->{$this->idKey} = $insert_id;
    +        }
    +        $return = SAVED_NEW;

    There's special cases like uid 0, wondering if that could be a problem. You don't really override that, though.

  3. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
    @@ -0,0 +1,345 @@
    +
    +    // @todo Descriptions, unique keys, indexes and foreign keys.
    +  }

    How are multi-field indexes going to work?

  4. +++ b/core/modules/entity/entity.module
    @@ -99,6 +100,36 @@ function entity_entity_bundle_delete($entity_type_id, $bundle) {
    +function entity_modules_installed($modules) {

    Are you sure this is not too late? modules might already want to create entities in hook_install()?

  5. +++ b/core/modules/serialization/lib/Drupal/serialization/Tests/EntitySerializationTest.php
    @@ -101,6 +95,18 @@ public function testNormalize() {
    +      ),
    +      'revision_uid' => array(
    +        array('target_id' => NULL),
    +      ),
    +      'log' => array(
    +        array('value' => NULL),
    +      ),

    So we add those by default now?

  6. +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.install
    index 0000000..b3d9bbc
    --- /dev/null

    --- /dev/null
    +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.module

    +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.module
    +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.module
    @@ -0,0 +1 @@

    @@ -0,0 +1 @@
    +<?php

    No longer needed, .module files are optional.

  7. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
    @@ -86,7 +86,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
           ->setDescription(t('The bundle of the test entity.'))
    -      ->setRequired(TRUE);
    +      ->setRequired(TRUE)
    +      ->setSetting('max_length', 32);

    FYI: #1709960: declare a maximum length for entity and bundle machine names adds a constant for 32.

  8. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestRev.php
    @@ -60,6 +60,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['revision_uid'] = FieldDefinition::create('entity_reference')
    +      ->setLabel(t('Revision user ID'))
    +      ->setDescription(t('The user ID of the author of the current revision.'))
    +      ->setSettings(array('target_type' => 'user'))
    +      ->setQueryable(FALSE)
    +      ->setRevisionable(TRUE);
    +
    +    $fields['log'] = FieldDefinition::create('string')
    +      ->setLabel(t('Log'))
    +      ->setDescription(t('The log entry explaining the changes in this revision.'))
    +      ->setRevisionable(TRUE)
    +      ->setTranslatable(TRUE);

    Ah, I see, this is manually added now, I guess to test how this will work for nodes?

    Those two fields only exist on the revision tables now, do we keep that? We don't want to see those values if we don't load a revision I think, or we risk duplicating it on save/showing it by default.

    Also, log shouldn't be Translatable, not in EntityTestRev.

plach’s picture

StatusFileSize
new23.74 KB
new100.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,363 pass(es), 60 fail(s), and 7 exception(s).
[ View ]

Sorry, just saw #48, I will address it in the next iteration.

  1. I know, I know :)
  2. Yep, in that case it's not a real override
  3. We are going to provide sensible defaults in the process methods that can be overriden by entity-type-specific schema builders (going to work on this now).
  4. Getting field definitions before installing the schema introduces a nasty bug when enabling the Locale module, which then tries to lookup its tables to translate annotations, but they are not there yet. We will introduce a new hook to let modules act when entity schema is installed.
  5. I would aim to do that, but it needs its own discussion. Now it just defaults to that.
  6. Cool :)
  7. Ok, we'll just skip that then
  8. Yep, but I think it's no longer needed with this patch
  9. I (mostly) agree but I think this needs a separate discussion
plach’s picture

StatusFileSize
new4.28 KB
new96.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,274 pass(es), 65 fail(s), and 10 exception(s).
[ View ]

Reverted some probably unneeded changes after addressing #48.

plach’s picture

The content translation test failures are expected because we are currently disabling translatability for entity types that do have translation support, as their schema has not been converted yet. We probably need some kind of BC layer until we are able to switch table layout, which probably needs to wait for #2068325: [META] Convert entity SQL queries to the Entity Query API.

plach’s picture

StatusFileSize
new11.35 KB
new94.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
3ca5a9b DEV 2183231: Renamed ::schemaBuilder() to ::schemaHandler().
9c1527d DEV 2183231: Restored shortcut translatability.
e531b32 DEV 2183231: Refactored entity save to avoid instantiating the schema handler.
92dfff1 DEV 2183231: Fixed entity serialization test.
969becf DEV 2183231: Reintroduced BC layer to support content translation without the data table.

This might be green again.

plach’s picture

Status:Needs work» Needs review
Related issues:
StatusFileSize
new94.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,028 pass(es), 69 fail(s), and 60 exception(s).
[ View ]
new2.48 KB

This should fix installation.

plach’s picture

Status:Needs work» Needs review
Related issues:
StatusFileSize
new94.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,284 pass(es), 54 fail(s), and 15 exception(s).
[ View ]
new2.15 KB

Let's see whether this is better:

fda182d DEV 2183231: Fixed BC layer comment.
32ad26d DEV 2183231: Fixed entity save.
plach’s picture

Status:Needs work» Needs review
Related issues:
StatusFileSize
new94.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,384 pass(es), 28 fail(s), and 14 exception(s).
[ View ]
new905 bytes

More fixes

plach’s picture

Status:Needs work» Needs review
Related issues:
StatusFileSize
new94.87 KB
new98.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,688 pass(es), 46 fail(s), and 46 exception(s).
[ View ]
new12.92 KB

This includes the previous fixes plus support for index and keys generation. We should be ready to remove the BC layer now.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new765 bytes
new98.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,997 pass(es).
[ View ]

Fixed unique keys generation.

plach’s picture

StatusFileSize
new36.35 KB
new127.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-schema-auto-2183231-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Just a quick and dirty attempt to remove all entity schemas. I was able to complete astandard installation, let's how this performs.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new127.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 58,721 pass(es), 475 fail(s), and 154 exception(s).
[ View ]
plach’s picture

StatusFileSize
new84.79 KB
new168.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 58,839 pass(es), 511 fail(s), and 200 exception(s).
[ View ]

This one should be cleaner:

f634ebb DEV 2183231: Fixed UUID unique keys.
edd5fd6 DEV 2183231: Completed removal of content entity schemas.
c0110bf DEV 2183231: Filtered out multiple base fields from base table mapping.
86b0f73 DEV 2183231: Moved additional entity tables to the related storage classes.
99120f8 DEV 2183231: Fixed some tests.
dfe9810 DEV 2183231: Fixed entity reference hack.
d2a41b9 DEV 2183231: Fixed entity query.

Still missing:

  • some details in the key generation to match exactly the original schema (unsigned int for id/revision_id), int length for id/revision_id (11 vs 10)
  • entity-type specific indexes (to be added in the various buildSchema() methods)
  • removing the drupal_get_schema() BC layer
  • documentation for the new hooks
  • fixing test failures
  • improving test coverage

We should be ready for some serious review though.

tstoeckler’s picture

Fixing some of the test fails now. Starting from the bottom of the list in case anyone is already working from the top.

tstoeckler’s picture

Found two issues:
- As entity query now uses getTableMapping() it fails to find the 'default_langcode' field on data tables, as we currently don't expose that as it's an implementation detail of the storage controller.
- Shortcut currently doesn't have stuff like shortcut_set on the data table denormalized (like node does) which means you need to reinstall after applying this patch, because after the patch it does. Or you can just use minimal profile.

tstoeckler’s picture

Also:
- Forum module is currently broken due to the issue mentioned in #48.4: Forum module tries to save a taxonomy term in forum_install(). This no longer works. :-/

tstoeckler’s picture

Also:
- Node module should have a separate storage controller which adds {node_access} to the schema.

(Sorry for the noise, just going through the tests one by one and don't want to lose this information)

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new18.58 KB
new183.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 61,065 pass(es), 475 fail(s), and 123 exception(s).
[ View ]

One last problem:
- tracker_install() queries {node}

This one fixes all those fails that are related to legacy usage of installSchema() in DUTB tests. Let's see.

tstoeckler’s picture

Some of the aggregator fails are due to #2227367: StringLong's schema is broken, so marking that as critical. Applying that patch doesn't actually fix aggregator because of the entity key defaults stuff. Aggregator items don't have a UUID (see also #2149851: Remove todo about GUID field on the 'aggregator_item' entity), but we're trying to add a unique key for the 'uuid' column nevertheless. I would suggest to simply roll the defaults stuff back. Also #2209049: Allow field schemas to contain unique keys would allow us to remove that hardcoded unique key from the schema handler in the first place.

plach’s picture

@tstockler:

Thanks for keeping the work going!

- As entity query now uses getTableMapping() it fails to find the 'default_langcode' field on data tables, as we currently don't expose that as it's an implementation detail of the storage controller.

I think this is not correct, we should definitely have default_langcode in the table mapping: it's true it's a table to field representation, but I think all actual table columns should be included.

- Forum module is currently broken due to the issue mentioned in #48.4: Forum module tries to save a taxonomy term in forum_install(). This no longer works. :-/

We can move the term installation to hook_entity_schema_installed() (see user.install) to fix that.

- Node module should have a separate storage controller which adds {node_access} to the schema.

There is the node access storage that should be the perfect candidate to do that, but I think this is follow-up material.

- tracker_install() queries {node}

Can we do a quick entity query conversion or it's too much work?

I would suggest to simply roll the defaults stuff back.

I think it's fine: now that stuff is properly refactored to leverage those properties the right way, removing entity key defaults and add some @todo should not be problematic.

Also #2209049: Allow field schemas to contain unique keys would allow us to remove that hardcoded unique key from the schema handler in the first place.

I don't think that's a big deal: if that gets in before this we can adjust our code, otherwise it's a just few lines that will need to be refactored.

YesCT’s picture

Issue summary:View changes
Issue tags:+Needs issue summary update

#20 3. (follow-up needed still?)

Also, tagging for issue summary update as a lot seems to have been sorted out since this issue was created.

Let's get a section for follow-ups and blockers.

tstoeckler’s picture

StatusFileSize
new9.75 KB

Re #82: Thanks for the feedback. Agreed on all accounts.

Re #83: Yes, that follow-up still needs to be created. I will hopefully get to that tonight.

...Along with another re-roll which should then seriously bring down the failure count

If I don't get to that, or life happens..., here's my interdiff for what I have so far. I will assign tonight if I get to it, so as long as this is unassigned, feel free to work on it, but then don't forget to assign yourself.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new29.3 KB
new198.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,097 pass(es), 435 fail(s), and 57 exception(s).
[ View ]

Oops, I did forget to assign in the end. Oh well.

Here we go: This should have a lot less fails. At least all of the problems mentioned above.

The fact that we now have non-entity table fields in getTableMapping() forced me to introduce a weirdness: There really was no sensible array key to use (as the key is the field definition name, but we don't have a field definition here), so I used '' as a key. To clean that up I would like to introduce a small value(ish) object for the table mapping á la Url which has some methods for building and returning the various parts of the current multi-dimensional array. Let's first get this to green, though.

I also introduced a hack in the form of db_table_exists() in tracker_install(). Will have to think more about how to avoid that, couldn't come up with anything so far.

I also removed the drupal_get_schema() backwards-compatibility layer as that should no longer be needed.

I'm pretty tired so won't update the issue summary and open follow-ups just yet, but hopefully tomorrow.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new7 KB
new203.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Here's some more trivial test fixes. The rest are actual failures. I really thought there would be less of those :-( Some of those might be trivial to fix as well, but we'll have to look into them one by one to figure them out. Before working on those I'm going to a) update the issue summary and open a bunch of follow-ups and b) complete the unit test that I have locally, at least with some basic coverage. Maybe that will turn out something.

Maybe I can summon @mauzeh to help with some of the test fails?! (Or anyone else of course!)

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new203.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,403 pass(es), 428 fail(s), and 50 exception(s).
[ View ]

'PDOException' with message 'SQLSTATE[08004] [1040] Too many connections'

Assuming that's a testbot problem. Re-uploading patch.

tstoeckler’s picture

Rewrote the issue summary and opened a bunch of separate issues. There's only one that's truely a follow-up, though. And none of them are critical or major, so don't panic! :-)

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new205.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,229 pass(es), 68 fail(s), and 17 exception(s).
[ View ]
new12.9 KB

This should fix a lot of tests...

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new214.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,441 pass(es), 41 fail(s), and 8 exception(s).
[ View ]
new8.9 KB

More test fixes, done for the moment.

plach’s picture

Priority:Critical» Normal
Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,573 pass(es).
[ View ]

Just a reroll before starting work

plach’s picture

StatusFileSize
new214.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,514 pass(es), 41 fail(s), and 8 exception(s).
[ View ]

Now for reals

plach’s picture

Assigned:plach» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.3 KB
new215.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,491 pass(es), 31 fail(s), and 6 exception(s).
[ View ]

A couple of fixes, I hoped to be able to work on this a bit more...

e70f72c DEV 2183231: Fixed book comment test.
0953b48 DEV 2183231: Fixed revision_uid default.
Berdir’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBookTest.php
    @@ -45,6 +45,7 @@ public function testBookCommentPrint() {
           'title' => 'Book title',
           'body' => 'Book body',
    +      'status' => NODE_PUBLISHED,
         ));
         $book_node->book['bid'] = 'new';

    This seems like the wrong fix? It's the same problem as feed.queued had, which had no default value. Instead, we should set the default_value on the status field...

  2. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -94,6 +94,10 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
    +      // Ensure we have a valid value for the revision author.
    +      if (!isset($record->revision_uid)) {
    +        $record->revision_uid = 0;
    +      }

    Same here I guess?

plach’s picture

Assigned:plach» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.11 KB
new215.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,203 pass(es), 36 fail(s), and 9 exception(s).
[ View ]

Not many fixes again...

tstoeckler’s picture

Assigned:Unassigned» tstoeckler
Status:Needs work» Needs review
StatusFileSize
new6.95 KB
new2.68 KB
new224.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,498 pass(es), 30 fail(s), and 7 exception(s).
[ View ]

OK, I'll take a stab at this.

This one already updates the patch to use the new EntityTypeInterface::BUNDLE_MAX_LENGTH and ContentEntityTypeInterface. Additionally it fixes ViewEntityDependencyTest.

It seems we need to open a follow-up to auto-generate views data from the auto-schema. I.e. that that would no longer be a nice-to-have but at least a major bug. See the @todo in the interdiff. Of course there might be a way to just temporarily fix the problem in some way that I haven't thought of, which would mitigate this.

plach’s picture

@tstoeckler:

It seems we need to open a follow-up to auto-generate views data from the auto-schema. I.e. that that would no longer be a nice-to-have but at least a major bug. See the @todo in the interdiff.

We have #1740492: Implement a default entity views data handler postponed on this work. I already linked it in a @todo or two in a previous patch, I think we can do the same with the @todo you added in the last one.

+++ b/core/lib/Drupal/Core/Entity/SqlStorageInterface.php
@@ -12,6 +12,80 @@
+   * The base table layout: no revision or multilingual support for base fields.
+   *
+   * @var int
+   */
+  const LAYOUT_BASE = 0;
+
+  /**
+   * The revision table layout: provides revision support for base fields.
+   *
+   * @var int
+   */
+  const LAYOUT_REVISION = 1;
+
+  /**
+   * The multilingual table layout: provides multilingual support for base
+   * fields.
+   *
+   * @var int
+   */
+  const LAYOUT_MULTILINGUAL = 2;
+
+  /**
+   * The multilingual revision table layout: provides revision and multilingual
+   * support for base fields.
+   *
+   * @var int
+   */
+  const LAYOUT_MULTILINGUAL_REVISION = 3;

I disagree with this change, sorry: those constants (and the methods below) are all details of our core storage implementation. Other SQL-based implementations might want to store data in a completely different way. Those constants/methods would make no sense in that context. I originally thought about SqlStorageInterface as a marker interface, then I realized that we would need the ::getTableMapping() method to let View generate its data automatically, but I think there's not much more we can abstract. I think it's fair to put ContentEntityDatabaseStorage in our type-hints as in those cases we are explicitly referring to our well-known core implementation.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new31.34 KB
new244.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,513 pass(es), 71 fail(s), and 10 exception(s).
[ View ]

Ok here we go. Will try to tackle some more stuff later today, but unassigning for now.

Re #111: As I really like type-hinting interfaces I did the following as a compromise: I introduced a new ContentEntityDatabaseStorageInterface extends SqlStorageInterface where I moved the constants and the get*Table() methods. I don't feel *super* strongly about this, so if this doesn't work for you @plach, then I'll give up :-) and type-hint ContentEntityDatabaseStorage again.

- This also reverts some of the changes to the node defaults that were problematic:
- The $node->status field default, for instance, depends on the node type configuration. So I added that default in bundleFieldDefinitions(). I also found that the field definition cache is not cleared when a bundle entity is updated, so I fixes that. Should probably be moved to a separate issue.
- The uid and revision_uid should not default to 0, IMO, but uid should be set to the current user by default and revision_uid should be set to the same value as uid if it is not provided. This is not yet implemented, so a couple tests that passed above will probably fail. I got a little confused however, which tests pass and which fail, so uploading this to get a status quo from the bot.

This also includes a baseline unit test. For now only the LAYOUT_BASE schema is tested without a UUID key, so there's a lot of more coverage to provide but it's a start.

tstoeckler’s picture

Hmm... damn. Will look into this later today.

Also forgot to mention: The interdiff in #112 includes the one from #107, at least those parts which I don't change further. I had forgot to apply that to my branch and noticed in the middle of fixing stuff.

Sylvain Lecoy’s picture

You might be interested in looking at the reverse procedure: e.g. building an entity object from Schema API: http://drupalcode.org/project/doctrine.git/blob/48b52566638b3e31e3e0ebff...

If you have any questions i'll be glad to help :)

Sylvain Lecoy’s picture

andypost’s picture

tstoeckler’s picture

Assigned:tstoeckler» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs issue summary update
Related issues:+#1842858: [PP-1] Convert menu links to the new Entity Field API, +#2225739: Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead
StatusFileSize
new21.43 KB
new822 bytes
new263.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,310 pass(es), 66 fail(s), and 46 exception(s).
[ View ]

Here we go.

This fixes all failures from below, except for ConfigImportAllTest, which I did not run locally (yet). The EntityReferenceAdminUiTest depends on #2225739: Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead in order to pass, so this is now blocked on that. Therefore the patch also includes that for now. (I left that out of the interdiff(s), though.)

I also fixed a few things here and there that I noticed reading through the patch, so even though I tried to be very careful I might have broken this or that. I really hope this gets us into the single digits, though, in terms of fails.

Other than that I added the hook documentation for hook_entity_schema_installed() (and for the corresponding hook_entity_schema_uninstalled() that I introduced for consistency). It's not the coolest hook in the world and we might be able to remove it at some point but for now this gets us closer to commit.

Also added test coverage for an entity with a UUID field. Adding test coverage for the other layout types (i.e. the actual nitty gritty) is up next.

Unassigning for now, though. I'll try to work a little bit more on test coverage but that shouldn't block anyone else taking a stab.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new2.09 KB
new264.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,503 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

One last try for today. This should fix all but one of the tests that previously failed. Given how things have gone in this issue I expect a whole bunch of new tests to fail now... Let's see.

martin107’s picture

It looks like there has been a change in policy

from Node.php

    // If no revision author has been set explicitly, make the node owner the
    // revision author.
    if (!$this->getRevisionAuthor()) {
      $this->setRevisionAuthorId($this->getOwnerId());
    }

So anonymous user changes will be marked as owner changes

and from UserCancelTest.php

    // Confirm that user's content has been attributed to anonymous user.
    $test_node = node_load($node->id(), TRUE);
    $this->assertTrue(($test_node->getOwnerId() == 0 && $test_node->isPublished()), 'Node of the user has been attributed to anonymous user.');
    $test_node = node_revision_load($revision, TRUE);
*    $this->assertTrue(($test_node->getRevisionAuthor()->id() == 0 && $test_node->isPublished()), 'Node revision of the user has been attributed to anonymous user.');
    $test_node = node_load($revision_node->id(), TRUE);
    $this->assertTrue(($test_node->getOwnerId() != 0 && $test_node->isPublished()), "Current revision of the user's node was not attributed to anonymous user.");

The test marked with a star (line 294) is expecting attribution by anonymous user.

There is more than this going on as the compiler error is

( ! ) Fatal error: Call to a member function id() on a non-object in XXXXX/sites/drupal/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php on line 294
tstoeckler’s picture

Assigned:Unassigned» tstoeckler

Re #122: Good catch! That check should do a strict check for === FALSE.

Regarding the UserCancelTest failure that's a different problem. I already have that fixed locally, will upload patch tomorrow.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new650 bytes
new264.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,644 pass(es).
[ View ]

Actually, reading #122 again, the conclusion is incorrect. $node->getRevisionAuthor() will actually return the user entity, not the ID, so even for anonymous users the current check is sufficient.

Also, for anyone wondering: In core there is no UI for setting the revision_uid column. The current behavior of always assigning the ID of the current user is not mitigated by the referenced code. I verified that.

Here is the patch with the fix to UserCancelTest. That test actually fails due to a pre-existing bug: When a user account gets cancelled and the nodes get reassigned node.module fails to re-assign the node_revision.revision_uid database field. There is actually test coverage for this (which failed with the above patch), the reason this is not failing in 8.x must be some weirdness in EntityReferenceItem. I will look into that.

Anyway, this one should ***finally*** be green. Let's see.

tstoeckler’s picture

So in terms of bringing this along additional test coverage would be awesome, if anyone wants to work on that.

I also still want to work on converting the table mapping to a value object before this lands, as the current empty string hack is really not so nice. My thoughts on that were something like the following:

<?php
TableMappingInterface
{
 
/**
   * Returns a list of tables.
   */
 
public function getTables();

 
/**
   * Returns a mapping of entity field names to the respective database columns for a specific table.
   */
 
public function getFieldColumns($table)
 
// Or alternatively?!
 
public function getFieldColumns($table, $field_name);

 
/**
   * Adds a list of database columns for a specific entity field.
   */
 
public function addFieldColumns($table, $field_name, $columns);

 
/**
   * Gets a list of database columns that do not belong to entity fields (e.g. for denormalization purposes).
   *
   * Please do find a better name for this method :-)
   */
 
public function getOtherColumns($table);

 
/**
   * Adds a database column that does not belong to an entity field.
   */
 
public function addOtherColumn($table, $column_name);

}
?>

Of course naming, etc. should be improved. I think those methods should cover the current usage of the table mapping, though. I might be wrong, however.

plach’s picture

Awesome work! I will try to review the patch ASAP.

About #125 it might be a good idea but we should try to get feedback from at least @fago and @berdir, before going on with coding.

tstoeckler’s picture

StatusFileSize
new531 bytes
new2.46 KB
new241.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,783 pass(es).
[ View ]

This needed a re-roll after #2225739: Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead.

I removed the Entity Reference hacks, as those should no longer be necessary. Let's see.

I also fixed a small oversight that SqlStorageInterface wasn't extending EntityStorageInterface.

tstoeckler’s picture

StatusFileSize
new240.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
tstoeckler’s picture

StatusFileSize
new836 bytes
new240.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183231-129-entity-schema-auto.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Git doesn't like me today....

Let's try this again. Also with an interdiff, as I removed a check that should not be necessary.

tstoeckler’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new240.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,870 pass(es).
[ View ]
plach’s picture

Status:Needs review» Needs work
StatusFileSize
new241.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,889 pass(es).
[ View ]

Just a reroll to merge/update my dev branch. Tomorrow I will post my review.

fago’s picture

Amazing to see this one being green, great work!

ad #125, TableMappingInterface: Yeah, I really like this idea. It's way cleaner and easier to use than an array with some documented keys. Even further, in future it might be possible to do a DatabaseStorage class that works independently from the table mapping? E.g., it just has save() and saveRevision() methods that work through all respective tables according the mapping. We could prepare for that by making sure TableMappingInterface already provides all the information needed for doing that.

Regarding uid default values, there is #1979260: Automatically populate the author default value with the current user which is working on that already.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -80,12 +121,27 @@ class ContentEntityDatabaseStorage extends ContentEntityStorageBase {
    +   * The entity schema builder.
    +   *
    +   * @var \Drupal\Core\Entity\Schema\ContentEntitySchemaHandlerInterface
    ...
    +  protected $schemaHandler;

    Is it a builder or handler? :-)

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +  public function getLayoutType() {
    +    if (!isset($this->layoutType)) {
    +      $this->layoutType = static::LAYOUT_BASE;

    This and the fuddling with constants does not seem too nice. Also, what does it add compared to checking whether an entity type is revisionable or translatable, or both?

    The check for the data table seems unnecessary for me. If it is translatable it *has* to be there (given this storage class is used), i.e. we can throw an exception e.g. during construct if it's not defined.

    Once that is done, couldn't the schema handler go and do just $entity_type->isTranslatable() / $entity_type->isRevisionable() / $entity_type->isTranslatable() && $entity_type->isRevisionable() to do the checks?

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +  protected function processFields($field_names) {

    getColumnMapping() then maybe?

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -663,53 +942,53 @@ protected function savePropertyData(EntityInterface $entity, $table_key = 'data_
    +          // If we are creating a new entity, we must not populate the record
    +          // with NULL values otherwise defaults would not be applied.

    Imo that's deprecated. We've default values in field definitions which have been applied to $entity - those should be saved, nothing else.

  5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorageInterface.php
    @@ -0,0 +1,103 @@
    +interface ContentEntityDatabaseStorageInterface extends SqlStorageInterface {

    what about ContentEntityDefaultDatabaseStorageInterface ?
    -> this would reflect that any new tandem of storage + schema classes should add their own interface variants.

    Also, documentation should clarify this is the interface of the default database storage, and contrib is free to add/use their own.

  6. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -449,10 +463,30 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    // @todo Fix this!
    +    if (!$node_type) {
    +      return $fields;

    Is there already an issue for that?

  7. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
    @@ -0,0 +1,100 @@
    +    // @todo Theoretically this table belongs to NodeGrantDatabaseStorage
    +    //   instead of NodeStorage. Add a schema creation facility to the former
    +    //   similar to this one.

    So as it doesn't belong in here, shouldn't it better stay in node_schema() for now?

  8. +++ b/core/modules/user/lib/Drupal/user/UserStorage.php
    @@ -154,4 +159,96 @@ public function updateLastLoginTimestamp(UserInterface $account) {
    +    // @todo Theoretically this table belongs to UserData instead of
    +    //   UserStorage. Add a schema creation facility to the former similar to
    +    //   this one.
    +    $schema['users_data'] = array(

    Again, should stay in user_schema() then?

plach’s picture

StatusFileSize
new13.68 KB
new240.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Fixed 1 and 3 from #135 + some other minor clean-up.

@fago:

2. Actually those constants aren't as useful as I had imagined, however we might keep them for now, as for BC reasons we need to keep those checks around and we cannot simply rely on the entity type keys.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +          $table_mapping[$this->dataTable][''] = array('default_langcode');
    ...
    +          $table_mapping[$this->dataTable][''] = array('default_langcode');
    ...
    +          $table_mapping[$this->revisionDataTable][''] = array('default_langcode');

    These don't look very clean, I am wondering whether we can come up with something better.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -663,53 +942,53 @@ protected function savePropertyData(EntityInterface $entity, $table_key = 'data_
    +          // If we are creating a new entity, we must not populate the record
    +          // with NULL values otherwise defaults would not be applied.
    +          if (isset($value) || !$is_new) {

    We can probably remove this check, as we no longer apply defaults at SQL level. This might imply some tricky test failure though, if that's the case we can defer this to a follow-up.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorageInterface.php
    @@ -0,0 +1,103 @@
    +interface ContentEntityDatabaseStorageInterface extends SqlStorageInterface {

    I am really not feeling the need for this interface, I cannot imagine why one would need to provide an alternative implementation of the very same concept without subclassing ContentEntityDatabaseStorage. Anyway, I am not feeling strongly about this either, if we go this way I agree with @fago that we need a more specific name.

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorageInterface.php
    @@ -0,0 +1,103 @@
    +  public function getLayoutType();

    If we don't drop this, I guess getTableLayout() would be a better name, now that I think about it :)

  5. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
    @@ -0,0 +1,464 @@
    +  public function getFieldColumnName(FieldDefinitionInterface $definition, $column) {

    This should move onto the storage class: we are once again instantiating the schema handler at every request atm.

  6. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
    @@ -0,0 +1,464 @@
    +    if ($this->storage->getLayoutType() & ContentEntityDatabaseStorage::LAYOUT_REVISION) {

    This should be referring to the interface (if we don't drop constants).

  7. +++ b/core/lib/Drupal/Core/Entity/SqlStorageInterface.php
    @@ -0,0 +1,24 @@
    +interface SqlStorageInterface extends EntityStorageInterface {

    Nice catch :)

  8. +++ b/core/modules/entity/entity.api.php
    @@ -0,0 +1,49 @@
    +    \Drupal::database()->schema()->dropTable('mymodule_node_data');

    Not sure this is the best example we can provide: we want to encourage people to use fields for this kind of additions :)

  9. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
    @@ -0,0 +1,100 @@
    +  public function getSchema() {
    +    $schema = parent::getSchema();

    These should be buildSchema() otherwise we would bypass the alter hook, but...

  10. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
    @@ -0,0 +1,100 @@
    +    // @todo Theoretically this table belongs to NodeGrantDatabaseStorage
    +    //   instead of NodeStorage. Add a schema creation facility to the former
    +    //   similar to this one.
    +    $schema['node_access'] = array(

    ... I agree with @fago that these shuld be left in the various hook_schema() implementations :)

  11. +++ b/core/modules/node/node.install
    @@ -434,18 +50,25 @@ function node_install() {
    +function node_entity_schema_installed(array $storages) {

    We can get rid of this change then.

  12. +++ b/core/modules/node/node.module
    @@ -836,6 +836,10 @@ function node_user_cancel($edit, $account, $method) {
    +      db_update('node_revision')
    +        ->fields(array('revision_uid' => 0))
    +        ->condition('revision_uid', $account->id())
    +        ->execute();

    Ouch, luckily we are addressing this in #2068333: Convert node SQL queries to the Entity Query API.

  13. +++ b/core/modules/user/user.install
    @@ -5,243 +5,36 @@
    +    db_insert('users')
    ...
    +    db_insert('users')

    We could try and just do a couple of entity saves.

  14. +++ b/core/tests/Drupal/Tests/Core/Entity/Schema/ContentEntitySchemaHandlerTest.php
    @@ -0,0 +1,312 @@
    +  protected $sqlStorage;

    I guess we could call this $dbStorage or just $storage. The current name might be ambigous wrt SqlStorageInterface.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.35 KB
new240.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,809 pass(es).
[ View ]

Better one

plach’s picture

StatusFileSize
new6.11 KB
new244.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 40,814 pass(es), 21,809 fail(s), and 3,642 exception(s).
[ View ]

Fixed aggregator, block and node indexes. I will finish them tomorrow.

@tstoeckler:

Do you want to take care of the reviews above or do you want me to go ahead with them?

plach’s picture

+++ b/core/modules/system/system.module
@@ -1698,6 +1698,8 @@ function system_page_alter(&$page) {
+  $schema = \Drupal::entityManager()->getStorage('custom_block')->getSchema();

Mmmh, I will get rid of this in the next reroll :)

plach’s picture

About #125: what about getExtraColumns() instead of getOtherColumns()?

sun’s picture

Hm. I noticed a level of excitement about this issue/patch slowly getting ready

I want to encourage you to keep on working on this, but I hope that you're aware that there are some critical problems in the currently proposed code...? Sorry for being snarky, but it's my honest first time impression after glancing over this patch :)

LOLWUT @ http://ibin.co/1JAhv20hGyHa

+        case static::LAYOUT_MULTILINGUAL_REVISION:
+          $table_mapping[$this->baseTable] = $this->getColumnMapping(array_diff($key_fields, array($this->langcodeKey)));
+          $data_key_fields = array_diff($key_fields, array($this->uuidKey));
+          $data_fields = array_diff($storable_fields, $key_fields, $revision_metadata_fields);
+          $table_mapping[$this->dataTable] = $this->getColumnMapping(array_merge($data_key_fields, $data_fields));
+          // Add the denormalized 'default_langcode' field to the mapping. As it
+          // does not correspond to a field definition we add it with an empty
+          // key.
+          $table_mapping[$this->dataTable][''] = array('default_langcode');
+          $table_mapping[$this->revisionTable] = $this->getColumnMapping(array_merge(array($this->idKey, $this->revisionKey, $this->langcodeKey), $revision_metadata_fields));
+          $revision_data_key_fields = array_diff($key_fields, array($this->bundleKey, $this->uuidKey));
+          $revisionable_fields = array_keys(array_filter($storable_definitions, $revisionable_filter_callback));
+          $revision_data_fields = array_diff($revisionable_fields, $revision_metadata_fields, $revision_data_key_fields);
+          $table_mapping[$this->revisionDataTable] = $this->getColumnMapping(array_merge($revision_data_key_fields, $revision_data_fields));
+          // See above.
+          $table_mapping[$this->revisionDataTable][''] = array('default_langcode');
+          break;

There are so many array_ functions involved, I... am not even remotely able to decipher what this code tries to do, and thus cannot suggest a more appropriate solution...

However, in case I get the gist right, then that crazy code cries for a dedicated domain data object with proper methods...?

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new400 bytes
new244.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-schema_auto-2183231-144.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hm. I noticed a level of excitement about this issue/patch slowly getting ready

I AM excited, honestly :)

I want to encourage you to keep on working on this, but I hope that you're aware that there are some critical problems in the currently proposed code...? Sorry for being snarky, but it's my honest first time impression after glancing over this patch :)

Well, I guess it depends on the definition of "critical": we are aware there's something we didn't address properly yet. Hopefully we are not missing anything that would require to rewrite the whole thing :)
Did you express all your main concerns in #143 or there's something more you did not have to the time to write down?

LOLWUT @ http://ibin.co/1JAhv20hGyHa

I know I know :)
(see #136.1)

There are so many array_ functions involved, I... am not even remotely able to decipher what this code tries to do, and thus cannot suggest a more appropriate solution...

Basically it's a way to ensure well-known columns come before arbitrary field columns. I guess there are ways to clean-up that code, but I really didn't have the time/motivation to do that yet.

However, in case I get the gist right, then that crazy code cries for a dedicated domain data object with proper methods...?

If I get you right, we were discussing about that in #125 and following.

plach’s picture

StatusFileSize
new244.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,883 pass(es).
[ View ]

Rerolled

plach’s picture

StatusFileSize
new2.97 KB
new240.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,718 pass(es), 34 fail(s), and 7 exception(s).
[ View ]

This fixes a problem in the node indexes and entity schema uninstallation.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.56 KB
new242.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,062 pass(es), 0 fail(s), and 258 exception(s).
[ View ]

Just an attempt to fix schema uninstallation (not really final, sun, all that code should be OOPified ;) + some test fixes. Partial interdiff, sorry I messed it up.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
new242.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,024 pass(es).
[ View ]

This should get rid of notices.

Berdir’s picture

Went through the module/test changes bottom to top.

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorage.php
    @@ -127,4 +131,86 @@ public function getChildCids(array $comments) {
    +
    +    $schema['comment_entity_statistics'] = array(
    +      'description' => 'Maintains statistics of entity and comments posts to show "new" and "updated" flags.',

    Same as file_usage.

  2. +++ b/core/modules/file/lib/Drupal/file/FileStorage.php
    @@ -38,4 +38,60 @@ public function retrieveTemporaryFiles() {
    +
    +    $schema['file_usage'] = array(
    +      'description' => 'Track where a file is used.',

    This needs the same @todo as similar tables, or we'd simply keep it in hook_schema(). Like we for example do for shortcut.module. I don't really understand why we have to do this? And in some cases don't...

  3. +++ b/core/modules/forum/forum.install
    @@ -89,6 +78,22 @@ function forum_install() {
    /**
    + * Implements hook_entity_schema_installed().
    + */
    +function forum_entity_schema_installed(array $storages) {
    +  if (isset($storages['taxonomy_term'])) {
    +    // Create a default forum so forum posts can be created.

    Are we sure this hook is called when forum and taxonomy_term is not enabled at the same time? Again, another problem that we could avoid if we install the schema earlier I think.

  4. +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.install
    @@ -0,0 +1,50 @@
    +function simpletest_test_schema(){
    +  $schema['simpletest_test'] = array(
    +    'description' => 'Stores simpltest_test data.',

    typo.

    Can we maybe use the existing test module that we use for database tests instead of adding a new table for this?

  5. +++ b/core/modules/tracker/tracker.install
    @@ -16,12 +16,18 @@ function tracker_uninstall() {
      */
    function tracker_install() {
    -  $max_nid = db_query('SELECT MAX(nid) FROM {node}')->fetchField();
    -  if ($max_nid != 0) {
    -    \Drupal::state()->set('tracker.index_nid', $max_nid);
    -    // To avoid timing out while attempting to do a complete indexing, we
    -    // simply call our cron job to remove stale records and begin the process.
    -    tracker_cron();
    +  // @todo Is there some way to avoid this check?
    +  if (\Drupal::database()->schema()->tableExists('node')) {

    Can't we install the entity schema before calling hook_install(), like we do normal schema? Yes, it has to be hardcoded in ModuleHandler, but so what? Or we could maybe use hook_preinstall?

  6. +++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
    @@ -366,9 +366,12 @@ public function validate(array $form, array &$form_state) {
    +      // @todo Make the user signature field use a widget to benefit from
    +      //   automatic typed data validation.
    +      $field_definitions = $this->entityManager->getFieldDefinitions('user', $this->getEntity()->bundle());
    +      $max_length = $field_definitions['signature']->getSetting('max_length');
    +      if (drupal_strlen($form_state['values']['signature']) > $max_length) {

    #2227381: Apply formatters and widgets to User base fields 'name' and 'email' is the issue for this.

  7. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -500,8 +500,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setDescription(t('Whether the user is active or blocked.'))
    +      // @todo This should be FALSE.
    +      ->setSetting('default_value', TRUE);

    Why? (@todo should say). Temporary todo or do we need a follow-up for this?

  8. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
    @@ -157,7 +157,7 @@ public function testCreate() {
         $field_info = $this->getMockBuilder('\Drupal\field\FieldInfo')
           ->disableOriginalConstructor()
           ->getMock();
    -    $entity_storage = new ContentEntityDatabaseStorage($entity_type, $connection, $field_info);
    +    $entity_storage = new ContentEntityDatabaseStorage($entity_type, $connection, $entity_manager, $field_info);

    I'm removing $field_info and injecting $entity_manager in #2116363: Unified repository of field definitions (cache + API), if we can get that in first then there's a bunch of stuff that won't be needed anymore.

plach’s picture

StatusFileSize
new243.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,143 pass(es).
[ View ]

Just a reroll. New patch following...

plach’s picture

StatusFileSize
new4.21 KB
new244.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

This completes the entity-type-specifc index definitions. I will start addressing the reviews above soon.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new737 bytes
new244.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-schema_auto-2183231-159.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@Berdir:

We have a tricky issue with the UriItem whose schema defines a text column having 2048 as maxlength, while the current {file}.uri column is a varchar(255). Can you have a look to the interdiff?

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new244.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Rerolled

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new928 bytes
new244.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,166 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Another try...

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new22.8 KB
new229.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,944 pass(es), 3 fail(s), and 22 exception(s).
[ View ]

This reverts the changes to hook_schema() implementations not related to entity storage classes plus some other schema fix:

72b246e DEV 2183231: Fixed comment indexes + reverted unrelated schema changes.
d2bd54e DEV 2183231: Reverted unrelated schema changes.
09748a5 DEV 2183231: Fixed term schema.

Some DUBT test will probably fail due to these changes, but posting anyway to identify them all.

After fixing test failures I will start addressing the reviews above if @tstoeckler does not beat me to it.

jessebeach’s picture

Issue summary:View changes

Massive patch! Awesome work! I'm 50% through reviewing. I think I got through the meat of it and the rest seems to be conversions, but I'll finish up tomorrow to be sure.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +
    +    // @todo Table names do not belong to the entity type definition, they are
    +    //   storage implementation details. Rip them out.
    +    $this->baseTable = $this->entityType->getBaseTable() ?: $this->entityTypeId;
    +

    This @todo needs an isuse logged for it.

    What should get removed from what? The recommendation is ambiguous.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +      if ($this->entityType->hasKey('revision')) {
    +        $this->layoutType |= static::LAYOUT_REVISION;
    +      }

    This could do with a comment to explain what this bitwise operation is doing.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +      $data_table = $this->entityType->getDataTable();
    +      if ($data_table && $this->entityType->isTranslatable()) {
    +        $this->layoutType |= static::LAYOUT_MULTILINGUAL;
    +      }
    +    }

    Again, a comment would be most helpful to explain what this bitwise operation is doing.

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +   */
    +  public function getSchema() {
    +    $schema = $this->buildSchema();
    +    $context = array('entity_type' => $this->entityType);
    +    // TODO Document this hook.
    +    $this->moduleHandler()->alter('entity_schema', $schema, $context);
    +    return $schema;
    +  }

    Secret hooks :)

    I added this to the issue summary.

  5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +  public function getTableMapping() {
    +    if (!isset($this->tableMapping)) {
    +      $table_mapping = array();
    +
    +      $key_fields = array_filter(array($this->idKey, $this->revisionKey, $this->bundleKey, $this->uuidKey, $this->langcodeKey));
    +
    +      // Storable fields are single-value base fields that are not defined as
    +      // computed and that do not specify a custom storage.
    +      // @todo Add support for multiple-value base fields.
    +      $storable_definitions = array_filter($this->fieldDefinitions, function (FieldDefinitionInterface $field_definition) {
    +        return !$field_definition->isComputed() && !$field_definition->hasCustomStorage() && !$field_definition->isMultiple();
    +      });
    +      $storable_fields = array_keys($storable_definitions);

    This @todo needs an issue.

  6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +
    +      // @todo Provide automatic definitions for revision metadata fields.
    +      //   Rename 'log' to 'revision_log'.
    +      $revision_metadata_fields = array_intersect(array('revision_timestamp', 'revision_uid', 'log'), $storable_fields);
    +      $revisionable_filter_callback = function (FieldDefinitionInterface $definition) { return $definition->isRevisionable(); };
    +

    This @todo needs an issue.

  7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +          $table_mapping[$this->dataTable][''] = array('default_langcode');
    +          $table_mapping[$this->revisionTable] = $this->getColumnMapping(array_merge(array($this->idKey, $this->revisionKey, $this->langcodeKey), $revision_metadata_fields));
    +          $revision_data_key_fields = array_diff($key_fields, array($this->bundleKey, $this->uuidKey));
    +          $revisionable_fields = array_keys(array_filter($storable_definitions, $revisionable_filter_callback));
    +          $revision_data_fields = array_diff($revisionable_fields, $revision_metadata_fields, $revision_data_key_fields);
    +          $table_mapping[$this->revisionDataTable] = $this->getColumnMapping(array_merge($revision_data_key_fields, $revision_data_fields));
    +          // See above.
    +          $table_mapping[$this->revisionDataTable][''] = array('default_langcode');
    +          break;
    +      }

    Is there no default? Perhaps an error?

  8. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -742,12 +1013,20 @@ protected function mapToDataStorageRecord(EntityInterface $entity, $table_key =
               ->fields(array($this->revisionKey => $record->{$this->revisionKey}))
    @@ -756,7 +1035,11 @@ protected function saveRevision(EntityInterface $entity) {

    @@ -756,7 +1035,11 @@ protected function saveRevision(EntityInterface $entity) {
           }
         }
         else {
    -      drupal_write_record($this->revisionTable, $record, $this->revisionKey);
    +      $this->database
    +        ->update($this->revisionTable)
    +        ->fields((array) $record)
    +        ->condition($this->revisionKey, $record->{$this->revisionKey})
    +        ->execute();
         }

    Is drupal_write_record removed or deprecated in this patch?

  9. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
    @@ -0,0 +1,456 @@
    +   */
    +  protected function getFieldSchemaData(FieldDefinitionInterface $definition, $key) {
    +    $data = array();
    +    $schema = $definition->getSchema();
    +
    +    foreach ($schema[$key] as $key => $columns) {

    Is $schema just an array at this point?

  10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
    @@ -7,6 +7,7 @@
    use Drupal\Core\Field\FieldItemBase;
    @@ -58,6 +59,10 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel

    @@ -58,6 +59,10 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
         $settings = $field_definition->getSettings();
         $target_type_info = \Drupal::entityManager()->getDefinition($settings['target_type']);

    +    if (!$target_type_info) {
    +      throw new \Exception(String::format('Target type @entity_type does not exist.', array('@target_type' => $settings['target_type'])));
    +    }
    +
         if ($target_type_info->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {

    Perhaps a specific Exception here and not the general one.

plach’s picture

Assigned:tstoeckler» plach

Thanks Jesse! I am going to fix those failure and then I will address your review and the ones above :)

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new7.54 KB
new237.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,211 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

Sorry @everyone, the last week was a bit crazy for me. I really wanted to push this along, but never got around to it. Thanks for keeping this going, especially @plach.

This should fix tests. Will work on addressing the reviews now.

Also noticed this is still assigned to me, sorry for that.

Edit: Crosspost. Damn, hopefully I wasn't to late.

plach’s picture

Assigned:plach» Unassigned

Ouch, I was about to post more or less the same patch :(

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
@@ -20,6 +20,15 @@ public static function getInfo() {
+    $this->installSchema('user', array('users_data'));

I think this can be fixed in the parent class.

Some answers:

@Berdir (#152):

Can't we install the entity schema before calling hook_install(), like we do normal schema? Yes, it has to be hardcoded in ModuleHandler, but so what? Or we could maybe use hook_preinstall?

Honestly I didn't think about harcoding it in the module handler: one reason we might want to avoid that and leave everything under the Entity module's control is that, when switching to dynamic schema handling, we will have a class (probably a service) handling schema installation/uninstallation/reinstallation/manipulation, well, you get me :)

The reason why hook_preinstall() does not work is that it's too early. Originally we were doing it there, but we found a nasty bug: when installing the Locale module, entity type definitions were parsed before Locale schema was installed, thus causing the string translator service, which was already aware of the Locale backend, to break as it tried to query (yet) unexisting tables. We might try to work around this, but actually this is just an example of a situation that theoretically any contrib module might trigger, so I thought it would be better to just define a dedicated hook, also because more of them will be added or renamed when unifying field storage and implementing dynamic schema.

@jessebeach:

I didn't qualify the @todos yet as I wanted to have a final patch, with some +1 before :)

1. The base|data|revision|revision_data table entity type keys
2. Ok, anyway @fago suggested to remove that code so we might go that way instead
7. Not sure what you mean :)
8. Nope, we are just no longer using it in the storage classes
9. Yep, it should always be a Schema API array

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new236.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Let's try that again.

tstoeckler’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new230.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,261 pass(es).
[ View ]

Oops, accidentally included some local, non-comitted changes and also forgot to merge 8.x for the 1000th time...
This should be better. Interdiff was correct.

I agree with putting the schema creation back into ModuleHandler. Once #2206347: Use event system in ModuleHandler is in, we can have the entity system register an event subscriber to clean that up.

plach’s picture

I agree with putting the schema creation back into ModuleHandler. #2206347: Use event system in ModuleHandler is in, we can have the entity system register an event subscriber to clean that up.

I am not clear why the event system should let us fix something hooks are not a good fit for: we'd still have to find the proper point in the execution flow: a new module-system hook or a new event, what would be the actual difference?

tstoeckler’s picture

Re @plach the difference is that the entity *system* - i.e. \Drupal\Core\Entity - cannot implement hooks. Hooks can only implemented by modules. That's why we had to put the stuff in entity.module for now. With the event system we can register arbitrary subscribers in core.services.yml. So the entity system can totally provide an event listener in \Drupal\Core\Entity\EventSubscriber to react to the module handler to install/uninstall the schema.

tstoeckler’s picture

Ahh, sorry I misunderstood your comment. Yes, you are of course correct and I am wrong.

Unlike I tried to allude to in #178 the problem is *not* that the code isn't being called, but the problem is *when* it gets called. Thanks for pointing that out! I will think about this more, maybe we need a new event over there or a new hook without that issue.

Berdir’s picture

The new event will have to be called before hook_install(), so the tables will be available there. The event is not a replacement for hook_install(), it's a replacement for the 10 one-off calls that are currently in ModuleHandler::install() to clear various caches, rebuild stuff and so on.

That's also what we did with the plugin cache clearer service, added a hardcoded call there and will then move to an event in that issue.

plach’s picture

Before hook_install() sounds much like hook_module_preinstall(), which would re-introduce the bug described above. From a theoretical POV #180 works for me, but we will have to be very careful with the actual implementation :)

jessebeach’s picture

+++ b/core/modules/comment/comment.install
@@ -28,157 +28,10 @@ function comment_install() {
-      ),
-      'name' => array(
-        'type' => 'varchar',
-        'length' => 60,
-        'not null' => FALSE,
-        'description' => "The comment author's name. Uses {users}.name if the user is logged in, otherwise uses the value typed into the comment form.",
-      ),
-      'mail' => array(
-        'type' => 'varchar',
-        'length' => 64,
-        'not null' => FALSE,
-        'description' => "The comment author's e-mail address from the comment form, if user is anonymous, and the 'Anonymous users may/must leave their contact information' setting is turned on.",
-      ),
-      'homepage' => array(
-        'type' => 'varchar',
-        'length' => 255,
-        'not null' => FALSE,
-        'description' => "The comment author's home page address from the comment form, if user is anonymous, and the 'Anonymous users may/must leave their contact information' setting is turned on.",
-      ),

Where do these db fields get added back? I'm looking in \Drupal\comment\CommentStorage::buildSchema(), but I don't see them declared.

Berdir’s picture

They are not re-added. They're generated based on Comment::baseFieldDefinitions() and the schema() method of the corresponding field type/field item classes. that's the main thing this issue is doing, generating the schema for the entity tables. buildSchema() overrides are only necessary for things that can't be generated automatically like additional indexes and tables like user roles or taxonomy term hierarchy stuff.

jessebeach’s picture

StatusFileSize
new53.52 KB
new22.28 KB

UML: http://www.lucidchart.com/invitations/accept/5356c34a-9470-474a-8732-696...

The UML is open for editing.

UML diagram of the database storage classes. ContentEntityDatabaseStorage now contains a reference to an instance of ContentEntitySchemaHandler

\Drupal\Core\Entity\Schema\ContentEntitySchemaHandlerInterface is not doing much for us yet.

Also, both \Drupal\Core\Entity\ContentEntityDatabaseStorage and \Drupal\Core\Entity\Schema\ContentEntitySchemaHandler implement this interface, even though ContentEntityDatabaseStorage contains an instance of ContentEntitySchemaHandler. We end up with a circuitious route to getting a schema:

ContentEntityDatabaseStorage->getSchema()

which calls

ContentEntityDatabaseStorage->buildSchema()

which calls

ContentEntityDatabaseStorage->schemaHandler->getSchema()

It seems improper to share an interface like this. Does ContentEntitySchemaHandler really need to implement ContentEntitySchemaHandlerInterface?

All but one method of ContentEntitySchemaHandler is defined outside an interface as well.

This is the current HEAD class/interface UML for reference.

UML diagram of the database storage classes.

jessebeach’s picture

StatusFileSize
new22.28 KB
new53.52 KB

Adding the images with different names. Grumble, process, grumble.

plach’s picture

It seems improper to share an interface like this. Does ContentEntitySchemaHandler really need to implement ContentEntitySchemaHandlerInterface?

I don't see any problem in having an object referencing another object and proxying some calls to it. It's a technique used in several common patterns. For a detailed explanation of why we went this way see the issue summary (bullet 2 of the proposed resolution).

Berdir’s picture

file.uri is special because it's quite a different thing than a usual (http) URI, it's something like public://file_path, and that coudn't be much longer than 255 anyway, so I'd suggest to simply set the max_length of that to 255 instead of doing it on the index only.

+++ b/core/modules/file/lib/Drupal/file/FileStorage.php
@@ -45,6 +45,8 @@ public function retrieveTemporaryFiles() {

+    // @todo There should be a 'binary' field type or setting.
+    $schema['file_managed']['fields']['uri']['binary'] = TRUE;

We do have an issue for this I think.

The reason why hook_preinstall() does not work is that it's too early. Originally we were doing it there, but we found a nasty bug: when installing the Locale module, entity type definitions were parsed before Locale schema was installed, thus causing the string translator service, which was already aware of the Locale backend, to break as it tried to query (yet) unexisting tables. We might try to work around this, but actually this is just an example of a situation that theoretically any contrib module might trigger, so I thought it would be better to just define a dedicated hook, also because more of them will be added or renamed when unifying field storage and implementing dynamic schema

I had the same problem with the plugin cache clearer, the answer was simply that the plugin cache clear needs to happen after the module schema is installed, I think it's the same here, first install tables from hook_schema(), then entity tables, then hook_install(). The order of calls *is* very fragile there, but I think it should be possible to find something that works.

I don't see another way, default stuff needs to work in hook_install(), I'm pretty sure that forum only works in the tests because it's always enabled at the same time as taxonomy.

+++ b/core/modules/comment/comment.install
@@ -28,3 +28,82 @@ function comment_install() {
+      'entity_type' => array(
+        'type' => 'varchar',
+        'not null' => TRUE,
+        'default' => 'node',
+        'length' => 255,
+        'description' => 'The entity_type of the entity to which this comment is a reply.',
+      ),
...
+    'primary key' => array('entity_id', array('entity_type', 32), array('field_id', 32)),

Oh, I missed this in the issue that limited entity_type to 32. Because this is broken, if I would really store a longer entity_type in there, then things could break if the only difference is the longer entity_type. Not related to this issue, but I did notice that you also have a limit on the entity_type index for comment to 32, that should also no longer be necessary because the field length should now already be limited to 32 (if it is not, then we need to adjust it accordingly). We should open a separate issue for this, though.

jessebeach’s picture

StatusFileSize
new55.62 KB

I don't see any problem in having an object referencing another object and proxying some calls to it. It's a technique used in several common patterns. For a detailed explanation of why we went this way see the issue summary (bullet 2 of the proposed resolution).

Fair point. I concede it's not the pattern that concerns me, just some of the details. So if we can work through the details, I'll be assuaged :)

Firstly, if the intent is to make it possible to swap out the schema handling (this is what I understand the intent to be), then we have a method in ContentEntitySchemaHandler that isn't declared in ContentEntitySchemaHandlerInterface, but that ContentEntityDatabaseStorage assumes exists.

public getFieldColumnName()

as in

protected function getColumnMapping($field_names) {
  $mapping = array();
  foreach ($field_names as $field_name) {
    $columns = isset($this->fieldDefinitions[$field_name]) ? array_keys($this->fieldDefinitions[$field_name]->getColumns()) : array();
    foreach ($columns as $index => $column) {
      $mapping[$field_name][] = $this->schemaHandler()->getFieldColumnName($this->fieldDefinitions[$field_name], $column);
    }
  }
  return $mapping;
}

Any schema handler would need to implement this function outside of the ContentEntitySchemaHandlerInterface, no?

Also, in bullet 2 of the issue summary, the following statement is made:

To hide this code organization detail from the outside SqlStorageInterface extends ContentEntitySchemaHandlerInterface.

I hadn't recalled this relationship so I added SqlStorageInterface to my map and discovered that the implementation has deviated from this description.

UML diagram of the Entity Storage and Database storage classes and interfaces.

SqlStorageInterface extends EntityStorageInterface. Maybe we just need to update the description in the issue summary?

And the last observation. The docblock of ContentEntitySchemaHandlerInterface::getSchema() indicates that the method "Gets the full schema array for a given entity type.". This is true for ContentEntitySchemaHandler. For ContentEntityDatabaseStorage, the getSchema method implementation also invokes the entity_schema alter hook. So in this case, the returned array is the full schema plus anything else modules alter into it. Perhaps that should be noted in the docblock for ContentEntityDataStorage::getSchema() implementation in addition to the @inheritdoc statement.

jessebeach’s picture

StatusFileSize
new116.48 KB

So, in the spirit of "putting it all together", I've mapped out the landscape and a little context for the getSchema method in its various incarnations.

In the graph, the blue-background boxes are "field" class or interfaces. The pink boxes are notes indicating what exactly getSchema is doing on this class.

UML diagram of the getSchema method in its various incarnations on interfaces.

For fields, getSchema is defined in \Drupal\Core\Field\FieldStorageDefinitionInterface. Both FieldInstanceConfig and FieldConfig implement this interface. FieldInstanceConfig delegates its getSchema method to its stored instance of FieldConfig.

For entity storage, getSchema is defined in \Drupal\Core\Entity\Schema\ContentEntitySchemaHandler. Both ContentEntityDatabaseStorage and ContentEntitySchemaHandler implement this interface. ContentEntityDatabaseStorage delegates its getSchema method to its stored instance of ContentEntitySchemaHandler.

The getSchema method ContentEntitySchemaHandler loops through its list of FieldItemInterface[] instances and calls their getSchema methods, aggregating the returned arrays by field name.

plach’s picture

StatusFileSize
new232.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here is reroll after #2225955: Improve the DX of writing entity storage classes, I hope I didn't break anything.

I am going to work a bit on making the schema match (almost) exactly the current HEAD one (I'm focusing on FKs now), hopefully my changes won't conflict with Tobias' ones.

@jessebeach:

Firstly, if the intent is to make it possible to swap out the schema handling (this is what I understand the intent to be)

Yep, that's the main goal :)

then we have a method in ContentEntitySchemaHandler that isn't declared in ContentEntitySchemaHandlerInterface, but that ContentEntityDatabaseStorage assumes exists.

Good catch, actually I was planning to move it on the storage class for two reasons:

  • since it is needed to build the table mapping, the dependency flow suggests that it belongs to the storage class and that it should be called by the schema handler;
  • ContentEntitySchemaHandlerInterface is storage-agnostic: it does not know (nor it needs to know) anything about tables and columns, in fact a MongoDatabaseStorage class could use it to return a schema definition array that makes sense for its implementation, possibly not even a Schema API array. By the way this is the reason why we decoupled it from the SqlStorageInterface :)

SqlStorageInterface extends EntityStorageInterface. Maybe we just need to update the description in the issue summary?

Yep, we definitely do. Moreover your nice diagram made me notice that ContentEntityDatabaseStorageInterface (which does not make too much sense to me, btw) should probably extend also FieldableStorageInterface.

[...] So in this case, the returned array is the full schema plus anything else modules alter into it.

I think the description is still valid as what's returned is always the entity schema: it's up to modules altering it in a sensible way. Adding a note the method implementation might make sense, but I think the hook documentation could be enough.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new7.46 KB
new233.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,708 pass(es), 11 fail(s), and 6 exception(s).
[ View ]

This should fix foreign keys.

tstoeckler’s picture

Related issues:+#2249113: Use an entity save instead of db_insert() in user_install()
StatusFileSize
new54.75 KB
new7.08 KB
new237.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,132 pass(es), 51 fail(s), and 31 exception(s).
[ View ]

OK, sorry it took me so long (again...) :-(

This fixes all reviews above, except:

  1. #136.13:
    +++ b/core/modules/user/user.install
    @@ -5,243 +5,36 @@
    +    db_insert('users')
    ...
    +    db_insert('users')

    We could try and just do a couple of entity saves.

    The code there is just being moved, I'd hate to introduce failures or anything due to an unrelated change. Opened #2249113: Use an entity save instead of db_insert() in user_install() for that.

  2. #152.3:
    +++ b/core/modules/forum/forum.install
    @@ -89,6 +78,22 @@ function forum_install() {
    /**
    + * Implements hook_entity_schema_installed().
    + */
    +function forum_entity_schema_installed(array $storages) {
    +  if (isset($storages['taxonomy_term'])) {
    +    // Create a default forum so forum posts can be created.

    Are we sure this hook is called when forum and taxonomy_term is not enabled at the same time? Again, another problem that we could avoid if we install the schema earlier I think.

    No we're not. In fact I'm pretty sure that it won't be. See below for my thoughts on the schema installation

  3. #168.2/3:
    +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +      if ($this->entityType->hasKey('revision')) {
    +        $this->layoutType |= static::LAYOUT_REVISION;
    +      }

    This could do with a comment to explain what this bitwise operation is doing.

    +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +      $data_table = $this->entityType->getDataTable();
    +      if ($data_table && $this->entityType->isTranslatable()) {
    +        $this->layoutType |= static::LAYOUT_MULTILINGUAL;
    +      }
    +    }

    Again, a comment would be most helpful to explain what this bitwise operation is doing.

    I simply forgot those. Although see below, for my thoughts on the layout type.

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -97,38 +153,241 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +   */
    +  public function getSchema() {
    +    $schema = $this->buildSchema();
    +    $context = array('entity_type' => $this->entityType);
    +    // TODO Document this hook.
    +    $this->moduleHandler()->alter('entity_schema', $schema, $context);
    +    return $schema;
    +  }

    Secret hooks :)

    I added this to the issue summary.

    See below for my thoughts on hook_entity_schema_alter()

OK, so there is a couple of things I would like to do:

  1. Regarding schema installation I think Berdir's points above are proof enough that the current approach does not fly. Before hacking this into the ModuleHandler, though, I'd like to try the following: Use the approach used in the cache system of putting all the queries in a try {} block and then trying to create the schema and re-running the query in a catch. If that does work it would solve a lot of our problems magically.
  2. Regarding hook_entity_schema_alter() I feel pretty strongly that we shouldn't introduce such a hook. hook_schema_alter() has always been pretty weird, and here it's just as weird, but IMO also completely unnecessary. You can swap out the storage controller, alter the field definitions, .... There really shouldn't be any need to alter the schema.
  3. Regarding the layout type I agree with @fago (and @plach?) that the constants don't provide much benefit. I'll look into ripping that out, but I don't think it's a priority.
plach’s picture

Thanks! Some answers to #202:

  1. Yep, it seems the current approach for schema installation is borked. As I said above, I didn't think about fiddling with the module handler directly, that's probably our better option. If then we are able to move things back to a hook implementation or an event, that's even better :)
  2. I am bit scared by hook_schema_alter() too, but when talking about this stuff with @Berdir on our way back from Szeged, he pointed out that it's not our job to prevent people from doing stupid things and that an alter hook could be useful. At least this is what I remember of our conversation :)
    I am more than open to reconsidering it, if @Berdir does not feel strongly about it. We could even remove it for now and add it back later if needed, it would just be an API addition. If we go this way we can get rid of the ::buildSchema() methods and just go with the ::getSchema() ones.
  3. As I said, initially I thought they would be way more useful, looking again to them now they look pretty redundant. The real reason beyond them is I never get a chance to use bitwise operators these days :D

Going to review the code now.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new235.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,850 pass(es), 14 fail(s), and 7 exception(s).
[ View ]
new6.05 KB

This should fix many tests.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new7.81 KB
new3.38 KB
new243.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,866 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This should be green again.

Some explanations:
- DrupalUnitTestBaseTest:
- Some assertions are removed, but no actual test coverage is removed: The test previously did the following:
- First test that installing a table from an enabled module works
- Second test that installing a table from a non-enabled module does not work
- Third enable the module from the previous step and test that installing the table now works.
- As you can see, the third step and the first provide identical coverage. That is way some assertions are removed and a little code is moved around.

- MigrateTaxonomyTermTest:
- Taxonomy terms are incredibly weird right now, as:
- They have a 'parent' field item which is defined as a single-value integer field in the field definitions, with a default value of 0.
- It is in fact a multi-value field that is stored in taxonomy_term_hierarchy table. This is fixed in the attached patch. Previously we were generating a schema field for the parent due to this, which this also fixes.
- When it doesn't have an actual parent it gets a single value of 0.
- The test was asserting that the field value is actually NULL, where it is 0 due to the above. I changed it to assert the correct (?) behavior.
- I have no idea why this patch changes that behavior.

Also looked into the taxonomy_term.vid max_length override and - instead of hardcoding the max length in the schema - I added 'max_length' support to EntityReferenceItem. The field definition already declares a max_length, so we just have to use it. Checked that the schema is generated correctly.

tstoeckler’s picture

Assigned:tstoeckler» Unassigned
Status:Needs work» Needs review
StatusFileSize
new748 bytes
new244.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,734 pass(es).
[ View ]

Here we go. Should be green for realz.

Also unassigning. I do plan to work on this and finish off the few remaining todos but don't want to hold anyone back in case I don't get to it.

tstoeckler’s picture

Issue summary:View changes
StatusFileSize
new17.57 KB
new6.09 KB
new338.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,263 pass(es), 1,183 fail(s), and 70 exception(s).
[ View ]

Here we go. This drops hook_entity_schema_alter(). Discussed this briefly in IRC with @Berdir and he didn't have a concrete use-case in mind either. Surely people will find some crazy way of using but I'd rather have them alter the field definitions or swap out the schema handler if they really need it. Since we want to support schema updates when field definitions change having altered schemas is really, really weird. I personally see this similar to an alter hook for config data, which we also don't have.

I also tried to employ the try-catch mechanism for creating schemas that the cache system uses. I literally copied a lot of code from there. Because the storage controllers already have a save->doSave separation I couldn't use that. I am currently using anonymous functions for that but would appreciate better suggestions.

This allowed me to remove hook_entity_schema_(un)installed(). Which should make this patch at least a tad smaller. This also makes #2249113: Use an entity save instead of db_insert() in user_install() not only a random nice-to-have issue but actually a (soft) blocker of this. I put a workaround in user_install() for now.

I suppose we could also remove DrupalUnitTestBase::installEntitySchema() but I didn't do that for now, it should work as is.

This will make #2068325: [META] Convert entity SQL queries to the Entity Query API a hard blocker of this, unfortunately. I hacked around one query in comment_get_thread() so I could at least save and display a node.

I also had to introduce 2 hacks into Views module, which I don't really know what to do with. We're trying very hard to discourage people from querying the DB directly but Views is built on the opposite. At the very least we should have a EntityQuery extends Query (well not *that* EntityQuery, but...) so that we don't have to put the hack in the generic Views Query class, but have a dedicated Entity one. Similarly, we should have a EntityPager class.

Let's see what else breaks :-/

Feel free to suggest to revert the try-catch changes. I too really wanted to avoid the entity query issue as a blocker. I *think* hacking the schema installation into the module handler - for now - would sufficiently solve the problem discussed above as well, but @plach was fairly adamently against it. So not really sure how to proceed.

tstoeckler’s picture

StatusFileSize
new245.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,411 pass(es), 1,020 fail(s), and 63 exception(s).
[ View ]

Doh, forgot to merge 8.x. Sorry for the noise. The interdiffs are correct, though.

tstoeckler’s picture

So this hunk is of course not intentional, will remove on next re-roll:

diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
index d48ae83..a151c9c 100644
--- a/core/includes/install.core.inc
+++ b/core/includes/install.core.inc
@@ -439,7 +439,7 @@ function install_begin_request(&$install_state) {
     $config = BootstrapConfigStorageFactory::get()->listAll();
     if (!empty($config)) {
       $task = NULL;
-      throw new AlreadyInstalledException($container->get('string_translation'));
+      //throw new AlreadyInstalledException($container->get('string_translation'));
     }
   }
plach’s picture

Assigned:Unassigned» tstoeckler
Status:Needs work» Needs review

Feel free to suggest to revert the try-catch changes. I too really wanted to avoid the entity query issue as a blocker. I *think* hacking the schema installation into the module handler - for now - would sufficiently solve the problem discussed above as well, but @plach was fairly adamently against it. So not really sure how to proceed.

I am definitely not against hard-coding the entity schema installation in the module-handler (see #204.1): please (please!), let's revert the try/catch change, it feels very fragile and awkward :(

I reviewed all the lastest interdiffs, the only stuff that is bothering me (aside from the try/catch change) is from #202:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -30,7 +30,7 @@
    +class ContentEntityDatabaseStorage extends ContentEntityStorageBase implements ContentEntityDefaultDatabaseStorageInterface, ContentEntitySchemaHandlerInterface {

    This shows exactly what bothers me of this interface: interfaces are used to hide implementation details, this one mandates or at least strongly suggests an exact implementation. Any storage class implementing it and not respecting some strict assumptions will probably end-up breaking stuff. This interface is a lie IMHO :)

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -525,27 +525,32 @@ public function getFieldStorageDefinitions($entity_type_id, $include_custom_stor
    +   * @param bool $include_custom_storage

    This is overly verbose, would just $custom work?

  3. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -476,7 +476,9 @@ public function getSchema() {
    +    return TRUE;

    I am not sure about this change: this means that the storage class will no longer take care of configurable fields. Even if it's just temporary, this may negatively affect the parent issue, so I'd like to find another solution if possible.

  4. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -361,8 +361,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    -      ->setRevisionable(TRUE);

    Why we removed this? Language is revisionable and storage classes need to now it.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new35.91 KB
new236.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Thanks for the review!

1. I removed ContentEntityDefaultDatabaseStorageInterface now. I did like it, but I'm fine with removing it.

2. We have a few other $include_foo parameters in EntityManagerInterface so I went with $include_custom as a compromise.

3. I reverted this and added a hack with a lengthy comment in ContentEntityDatabaseStorage. Note that currently using hasCustomStorage() would not be a problem but with #2144263: Decouple entity field storage from configurable fields or a follow-up thereof it would eventually become problematic.

4. I personally think isRevisionable() and isTranslatable() does not make sense at all for entity key fields as it's not at all defined what that actually means, but I reverted that change for now.

plach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -156,10 +184,18 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
    +      return !$definition->isMultiple() && !($definition instanceof FieldConfigInterface);

    The idea is that we use "base entity schema" only for base fields, configurable fields should be bundle fields if I am not mistaken.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -345,17 +374,17 @@ public function getTableMapping() {
    +        // data field values for all the reivisionable fields and the

    typo

tstoeckler’s picture

Re #220.1: Well field instances (FieldInstanceConfig) are bundle field definitions, but fields themselves (FieldConfig) are field storage definitions. Which makes sense, that is what they are conceptually. They are added in hook_entity_field_storage_info(). Bundle fields themselves cannot have any storage, as we don't have per bundle storage.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new506 bytes
new236.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,354 pass(es), 80 fail(s), and 49 exception(s).
[ View ]

Sorry, should have tested this. This one installs at least and I can add a node. Let's see if I broken anything inadvertantly.

tstoeckler’s picture

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -156,10 +184,18 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
     $this->storageDefinitions = $entity_manager->getFieldStorageDefinitions($entity_type->id(), FALSE);
-    // @todo Remove this when multiple-value base fields are supported in
+    // @todo Remove the check for FieldDefinitionInterface::isMultiple() when
+    //   multiple-value base fields are supported in
     //   https://drupal.org/node/2248977.
+    // @todo Configurable fields currently have a storage that supports multiple
+    //   values even when their cardinality is set to 1. Therefore the
+    //   FieldDefinitionInterface::isMultiple() check is not sufficient to
+    //   filter them out. From the perspective of the storage
+    //   FieldConfig::isMultiple() should always return TRUE. This cannot be
+    //   implemented that way, however, as the same method is used to decide
+    //   whether or not to display a multiple widget on forms.
     $this->storageDefinitions = array_filter($this->storageDefinitions, function (FieldStorageDefinitionInterface $definition) {
-      return !$definition->isMultiple();
+      return !$definition->isMultiple() && !($definition instanceof FieldConfigInterface);
     });

@221: We are having troubles because we are retrieving field storage definitions, then we need to filter out field config storage definitions obviously. If we just get base field definitions (which ARE field storage definitions), through EntityManagerInterface::getBaseFieldDefinitions(), we don't need such a check. This was one of the main points we discussed in Szeged: we decide whether a field is stored in the base tables or gets CCK schema based on it being "optional", i.e. belonging only to certain entity type bundles.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.34 KB
new236.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,102 pass(es), 33 fail(s), and 2 exception(s).
[ View ]

Let's try this

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
new236.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,242 pass(es), 4 fail(s), and 5,060 exception(s).
[ View ]

Let's see if this is already enough. It fixes VocabularyUnitTest at least and ContentEntitySchemaHandlerTest.

tstoeckler’s picture

Status:Needs work» Needs review

Ahh, so the fails from above were actually fixed, the new fails are due to a bug that was introduced by #2201051: Convert path.module form alters to a field widget. PathItem does not declare any 'columns' in its schema (because the values are stored externally) But getSchema() calls $schema['columns'] unconditionally to see if the field item declares columns with reserved words. It's just that PathItem::getSchema() is never called in 8.x

tstoeckler’s picture

StatusFileSize
new944 bytes
new237.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,445 pass(es).
[ View ]

Oops forgot the patch+interdiff.

tstoeckler’s picture

StatusFileSize
new22.55 KB
new242.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

This introduces TableMappingInterface.

I think I've found an API that is reasonably nice for both using the mapping as well as building it. I'd love your thoughts on it, though!

Let's see what this breaks.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new2.33 KB
new242.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Yeah, late night patch rolling is not always a good idea.

plach’s picture

Status:Needs work» Needs review

Overall, changes look good to me, thanks! I still think in the stroage class we should be using base field definitions instead of storage definitions (see #224). Storage field deifnitions make sense in the table mapping stuff though, as we should probably return also cck tables in the getTables() method (to be done in #2144263: Decouple entity field storage from configurable fields, probably). We can differentate between the base vs bundle fields by checking whether the storage definition implements FieldDefinitionInterface.

  1. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
    @@ -0,0 +1,87 @@
    +   *       'format' => 'description_format',
    ...
    +   * 'description_format' in the example above) depend on the table mapping

    Missing double underscore.

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
    @@ -0,0 +1,87 @@
    +   * The values of the inner array ('title', 'description__value', and

    I think 'title' should be replaced with 'value' if I am understanding this correctly.

  3. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
    @@ -0,0 +1,87 @@
    +   *   An array where the keys are the names of the entity fields and the values
    +   *   are in turn arrays whose keys are the names of the columns as specified
    +   *   in the field item's schema() method and the values are the respective
    +   *   database column names for the respective entity fields.

    I am wondering whether this description is too implementation-specifc. Would it make sense to write something more generic?

plach’s picture

Status:Needs review» Needs work
tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new3.9 KB
new243.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,455 pass(es).
[ View ]

Re 2.: No in fact 'title' is correct there. The point of this paragraph is to try to explain which parts are implementation specific and which parts are generic. In this case naming the database column after the field name (== 'title') is the implementation detail. We could just as well choose to always use the property name as well even for single-column fields - i.e. 'title__value'. Please do provide suggestions on how to document this better.

I tried to expand the documentation a little bit in this patch.

Re 3.: The problem is that for the mapping to be truely generically usable as is, i.e. without further looking into field definitions, etc. We need all three pieces of the following pieces information and we need the relationship between them:
- the field name
- the column name as defined in getSchema()
- the actual db column name
Again, I am more than open to improvement suggestions, but I didn't really see what structure would be better for this.
Hmm... thinking about this I have the following idea: Instead of getFieldColumns() returning the nested structure as above. We have a getFieldNames($table_name) that simply returns a list of field names that are stored per entity type, and then we have a getFieldColumnMapping($field_name) which returns the array('value' => 'title') or array('value' => 'description__value', 'format' => 'description__format') structure, respectively. Thoughts?

According to the previous test result this should be green. Let's see.

Edit: Forgot the most important part: Thanks a lot (!) for the review. This feels close..

tstoeckler’s picture

StatusFileSize
new26.73 KB
new236.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,436 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This implements the idea from #238. It forced me to add a line of code in a few places but it allowed to remove the lengthy documentation on the array structure, which IMO is a sign of a more sane API.

This also changes back to using baseFieldDefinitions() instead of fieldStorageDefinitions(). I still think the latter is more correct, but we can re-hash that debate when we add support for multi-value fields at which point there will actually be a difference. For now it's just a semantic debate.

I will work on adding test coverage next.

Btw: I have one question, that I forgot to ask the whole time. Why are we using 'field__' for field-level unique keys/indexes/foreign keys and not the entity type ID? I mean they are still within the realm of an entity type?!

jessebeach’s picture

The test fail in #239 is due to $entity_manager->getBaseFieldDefinitions($entity_type->id()), where $entity_type->id() equals shortcut, is returning NULL. This is happening in the __construct method of ContentEntityDatabaseStorage.

jessebeach’s picture

The test fail in #239 is due to $entity_manager->getBaseFieldDefinitions($entity_type->id()), where $entity_type->id() equals shortcut, is returning NULL. This is happening in the __construct method of ContentEntityDatabaseStorage.

Derp, my bad. base fields are returning just fine for shortcut. Still looking :)

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new822 bytes
new236.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,338 pass(es).
[ View ]

Sorry, I should really know to run unit tests locally by now, before submitting patches.

Here we go.

Working in better test coverage now.

tstoeckler’s picture

StatusFileSize
new28.66 KB
new250.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,427 pass(es).
[ View ]

Here we go. This brings 100% coverage for DefaultTableMapping and greatly expands the test coverage of ContentEntitySchemaHandler. Still no revisionable / translatable layouts, but full test coverage of index / foreign key generation.

Speaking of which, I think our naming standard for foreign keys is a little bit strange: for single-column fields it is "field__$field_name" and for multi-column fields it is "field__$field_name_$specifier" where $specifier is the original name of the foreign key. I don't really see a need for this differentiation but I'll leave that up to @plach to decide.

Writing the test coverage made be refactor a few internal things minorly. I.e. before we were calling getSchema() and getDescription() multiple times, so I optimized that. I also noticed that for multi-column fields we duplicate the schema description currently, i.e. all columns for a field simply get the field description as schema description. Let's sort that out in #2232471: Discuss and standardize field definition descriptions == schema descriptions, though. I think I might have an idea for that...

yched’s picture

From #239 :

Why are we using 'field__' for field-level unique keys/indexes/foreign keys and not the entity type ID? I mean they are still within the realm of an entity type?!

Yeah, that sounds wrong. Probably an overlook from when we moved storage from per-field to "per [entity type, field name]" in #1497374: Switch from Field-based storage to Entity-based storage ?

think our naming standard for foreign keys is a little bit strange: for single-column fields it is "field__$field_name" and for multi-column fields it is "field__$field_name_$specifier" where $specifier is the original name of the foreign key. I don't really see a need for this differentiation but I'll leave that up to @plach to decide

Agreed that the difference is not ideal. Then again, doesn't it (kind of) reflect the current column names for base fields in base tables ? (just $field_name for single-column field, $field_name__$column for multi column). I guess it's better to keep the two in sync.

plach’s picture

I quickly skimmed through the changes and they (again) look good to me thanks! I will do a deeper review later today. I think we still owe @fago the removal of the layout type constants, but I can try to work on that myself if you wish to work on improving test-coverage further.

Why are we using 'field__' for field-level unique keys/indexes/foreign keys and not the entity type ID? I mean they are still within the realm of an entity type?!

The original idea was reducing the risk of clashes between stuff automatically generated from field definitions and things manually added by the storage class. If not prefixing field stuff with the entity type id feels wrong I think we can safely add such a prefix:

node_field__stuff
node__stuff

I think our naming standard for foreign keys is a little bit strange: for single-column fields it is "field__$field_name" and for multi-column fields it is "field__$field_name_$specifier" where $specifier is the original name of the foreign key.

I don't remember whether this was intentional, but I don't see the need for such a difference either. Probably it's just a failed attempt to implement what @yched says in #245.2 :)

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php
@@ -0,0 +1,267 @@
\ No newline at end of file

Oops :)

plach’s picture

Assigned:tstoeckler» Unassigned
Status:Needs review» Needs work

Since we are getting close to being ready here I performed a diff between the entity schema tables before/after the patch. These are the relevant differences:

  • Dropped SQL defaults / NOT NULL constraints (aside from mandatory field values), since defaults should be provided at data structure level.
  • Added some additional indexes, as specified by field schema or generated schema.
  • In some places we have different int lengths for IDs (serial and entity references) - int(11) vs int(10) unsigned.

These are the table-specifc differences:

  • aggregator_feed: hash - varchar(64) vs varchar(255), image - longtext vs text
  • aggregator_item: guid - text vs longtext
  • comment: status - tinyint(3) unsigned vs tinyint(4), mail - varchar(64) vs varchar(254), homepage - varchar(255) vs text
  • custom_block: info - varchar(128) vs varchar(255)
  • custom_block_revision: info - varchar(128) vs varchar(255), log - longtext vs varchar(255)
  • file_managed: uri - varchar(255) vs text, filesize - bigint(20) unsigned vs int(10) unsigned, status - tinyint(4) vs int(11), created/changed - int(10) unsigned vs int(11)
  • node: type - varchar(32) vs varchar(255)
  • node_field_data: type - varchar(32) vs varchar(255), status/promote/sticky: int(11) vs tinyint(4)
  • node_field_revision: type - varchar(32) vs varchar(255), status/promote/sticky: int(11) vs tinyint(4)
  • node_revision: log - longtext vs varchar(255),
  • shortcut: shortcut_set - varchar(32) vs varchar(255), weight/route_name/route_parameters moved to shortcut_field_data
  • shortcut_field_data: title - varchar(32) vs varchar(255)
  • users: name - varchar(60) vs varchar(255), pass - varchar(128) vs varchar(255)

Some of the above can be fixed by changing field definitions but I think most of them is ok. I will post a detailed plan tomorrow.

plach’s picture

Assigned:Unassigned» tstoeckler
Status:Needs work» Needs review
StatusFileSize
new660 bytes
new246.21 KB

Minor adjustment

tstoeckler’s picture

Assigned:tstoeckler» Unassigned
Issue summary:View changes
Related issues:+#2258347: Consider adding hook_entity_schema_alter()
StatusFileSize
new6.1 KB
new12.96 KB
new660 bytes
new27.76 KB
new261 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Here we go. This fixes #245/#246 and completes the test coverage for ContentEntitySchemaHandler. The only thing left to test is ContentEntityDatabaseStorage::getTableMapping().

Discussed the index and foreign key naming with @plach and we agreed on the following pattern: ENTITY_TYPE_ID_field__FIELD_NAME__KEY where KEY is the key used for the index/unique key/foreign key in the field schema declaration.

This also removes the table layout constants per the above.

I had to produce several interdiffs to due a bunch of merges.

Marking "needs review" for the bot, but this is still needs work per #248.

Edit: Crosspost with #249, but included that as well (including interdiff :-P).

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new4.97 KB
new261.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,544 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

A) I accidentally removed more code that I wanted to.

B) I fixed the NOT NULL => TRUE for entity keys above, but that apparently causes other problems. I will have to investigate that.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new5.85 KB
new261.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,509 pass(es).
[ View ]

tstoeckler--

tstoeckler’s picture

StatusFileSize
new6.76 KB
new261.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,900 pass(es), 24 fail(s), and 66 exception(s).
[ View ]

This should work. Added a lengthy comment on why revision_id cannot be NOT NULL.

tstoeckler’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new1.29 KB
new262.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,354 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Ahh, that 'label' is also an entity key always throws me off. That should not be required either.

Also updating the issue summary a bit.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new263.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,629 pass(es).
[ View ]
new2.54 KB

Crazy stuff, as usual with rest.module ;)

Shortcut is also tricky, the existing code there was broken, WimLeers recently noticed that too in #2241235: Shortcut/ShortcutSet entity types should use cache tags, this is a slightly different fix without the weird isOriginalId() condition. Also added a not-syncing check there.

Based on a discussion with @alexpott, this code should probably live in the UI and not in postSave(), it depends on the user creating the shorcut to check which one is the default, so that's pretty weird to live there.. but that goes beyond what we have to fix here.

fago’s picture

Great to see the TableMapping class + interface, that's much better than the arrays. Even more awesome you've been able to do away with the table layout constants already!

   * Adds field columns for a table to the table mapping.
   *
   * @param string $table_name
   *   The name of the table to add the field column for.
   * @param string[] $field_names
   *   A list of field names to add the columns for.
   *
   * @return $this
   */
  public function addFieldColumns($table_name, array $field_names) {
    $this->fieldNames[$table_name] = $field_names;

I think this should be called setFieldNames as it overwrites existing ones. Maybe a better name would be setTableFields() as it defines which fields go into that table.

  /**
   * Returns a list of all database columns for a given table.
   *
....
   */
  public function getAllColumns($table_name);

I think this can be called just getColumns($table_name) - as there are no different ways between getting all or not-all columns.

  /**
   * {@inheritdoc}
   */
  public function getColumnMapping($field_name) {

I must say this is a bit confusing as it's a mapping part of another mapping (table mapping). Could we just call this getTableColumnNames() maybe, i.e. always differentiate between a "field column" and a "table column" ?

  protected function processIdentifierSchema(&$schema, $key) {
hm, could we solve that by adding a separate "identifier" field type maybe? That said, we should probably mark the "revision id" as revisionable as it values change by revision also.

Awesome to see the table mappings being applied in mapToStorageRecord(), #2232427: Field types cannot control what gets stored is a great follow-up here.

@ContentEntityDataBaseStorage

  /**
   * {@inheritdoc}
   */
  public function getTableMapping() {
    if (!isset($this->tableMapping)) {
      $this->tableMapping = new DefaultTableMapping($this->storageDefinitions);
....

As I'd see it, all that table mapping customizations do not fit in the storage class as the one controls the table mapping should be solely the schema handler. The schema handler tells us how the table mapping looks like, so it should be created/configured there. So could we move that code over there and just forward getTableMapping() to the schema handler?
Ideally, I think the storage class itself would not know about the table mapping at all and just handle all tables in a generic fashion, similar as attachPropertyData() already does. That's something we could look at in a follow-up though.

Found a typo:

@@ -135,7 +135,7 @@ protected function getTables() {
    *   The table schema to add the field schema to, passed by reference.
    * @param string $field_name
    *   The name of the field.
-   * @param string[] $column_mappi
+   * @param string[] $column_mapping
plach’s picture

As I'd see it, all that table mapping customizations do not fit in the storage class as the one controls the table mapping should be solely the schema handler. The schema handler tells us how the table mapping looks like, so it should be created/configured there. So could we move that code over there and just forward getTableMapping() to the schema handler?

This would imply loading the schema handler at each request, which we are trying to avoid as it will grow bigger. I think it's ok for the storage class to know how fields are mapped to tables. Note that this information does not prevent us from cleaning-up all the current assumptions and just relying on the mapping in the load/save code.

plach’s picture

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
@@ -133,6 +133,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+        // It is not possible to set the language to NULL as it is automatically
+        // re-initialized. As it must not be empty, skip it if it is.

I am wondering whther it would make sense to open a follow-up to allow the langcode to be actually NULL. I am not sure why we should special case language, since the other entity keys can be NULL.

Berdir’s picture

The problem with langcode is that we re-initialize it automatically in onChange(), so it's set to an empty value instead of NULL.

Wasn't sure if ContentEntityBase would be able to deal with that being NULL but maybe we can add a check to only update it if it's not NULL? somehow... +1 to try and improve that in a separate issue, but I still think that the crazy stuff that rest.module there is doing is well... crazy. There should be a better way than having to differentiate between setting a list to NULL and array().

tstoeckler’s picture

StatusFileSize
new39.31 KB
new276.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Here we go.

This adds some test coverage for the methods in ContentEntityDatabaseStorage (except for getTableMapping(), didn't get to that one yet) and fixes the above review, except for:

I think this can be called just getColumns($table_name) - as there are no different ways between getting all or not-all columns.

Well, there's still getExtraColumns(), so I think having just getColumns() in addition would be confusing.

As I'd see it, all that table mapping customizations do not fit in the storage class as the one controls the table mapping should be solely the schema handler. The schema handler tells us how the table mapping looks like, so it should be created/configured there. So could we move that code over there and just forward getTableMapping() to the schema handler?

I don't really care at this point, but as @plach above disagrees, I'll let you guys fight this out before changing anything. :-)

protected function processIdentifierSchema(&$schema, $key) {
hm, could we solve that by adding a separate "identifier" field type maybe? That said, we should probably mark the "revision id" as revisionable as it values change by revision also.

Let's please handle those in a follow-up, as both them are non-trivial/controversial IMO.

plach’s picture

If the end goal is having both a storage class and a schema handler class that are decoupled from the actual table layout, what about having a separate factory encapsulating the table-layout logic to instantiate the table mapping? We could exploit per-entity-type services to make the factory swappable.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new21.64 KB
new285.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,642 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

PhpStorm-- I don't know why it didn't rename the actual usages of the method. I should have checked, though, so tstoeckler-- also.

This should fix that.

Also added test coverage for getTableMapping() for simple and revisionable entity types. Translatable and "both" are up next.

I like #268. If noone beats me to it, I will look into that after completing test coverage. However, if I'm not mistaken

We could exploit per-entity-type services to make the factory swappable.

will not work for the same reason it didn't work for the schema handler.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new18.18 KB
new299.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2183231-271-entity-schema-auto.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fix new test introduced in #2257421: Convert CommentDefaultFormatterCacheTagsTest from web test to entity unit test.

Also completes test coverage. We now have complete test coverage for the introduced code.

tstoeckler’s picture

Assigned:tstoeckler» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new299.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,710 pass(es).
[ View ]

The drop is moving fast today... This time it was #2224549: Simplify checking whether an entity type is revisionable. That's a good thing, though. Because we can use the method that that introduced here.

jessebeach’s picture

If the end goal is having both a storage class and a schema handler class that are decoupled from the actual table layout, what about having a separate factory encapsulating the table-layout logic to instantiate the table mapping? We could exploit per-entity-type services to make the factory swappable.

A fine goal. Two questions:

  1. Can we do this as a followup?
  2. This strikes me as non-API breaking, just code refactoring. Is that true?
jessebeach’s picture

Thank you for the test coverage for DefaultTableMappingTest.

This patch has well passed that size where one can comfortably review it in a day :) So, we need to satisfy the following steps, I believe, to feel confident that it's ready.

1. Testbost is green ('natch).
2. New test coverage is sufficient
3. Generated schemas match the schemas in 8.x HEAD

tstoeckler added test coverage for DefaultTableMapping. As far as I can tell the rest of the test coverage looks sufficient.

So, let's establish that the auto-generated schemas are the same as the declared schemas in HEAD and then move on this patch. I'm going to attempt to do this.

jessebeach’s picture

StatusFileSize
new145.08 KB
new143.37 KB

Alight, I've created SQL dumps for an 8.x HEAD install and a site install with this patch. I installed every module and applied config for each core field. If you'd like to install this testing site, I've pushed it up to a sandbox. The dump files are attached.

https://drupal.org/project/bloat/git-instructions

I diffed them and noticed the following inconsistencies:

  1. In comment.install, we had the following entry for the Comment entity's cid field.
    'cid' => array(
      'type' => 'serial',
      'not null' => TRUE,
      'description' => 'Primary Key: Unique comment ID.',
    ),

    which ends up having length 11.

    In the patch, we declare this field as an int, which ends up having length 10. Many incrementing fields were 11 and now are 10 in length.

  2. The 'file_managed' schema has numerous inconsistencies in field types: filesize was a bigint, now it's an int.
  3. The shortcut table is missing the following properties: weight, route_name, route_parameters

    Before

    CREATE TABLE `shortcut` (
      `id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'Primary Key: Unique shortcut ID.',
      `uuid` varchar(128) DEFAULT NULL COMMENT 'Unique Key: Universally unique identifier for this shortcut.',
      `shortcut_set` varchar(32) NOT NULL DEFAULT '' COMMENT 'The bundle of the shortcut.',
      `langcode` varchar(12) NOT NULL DEFAULT '' COMMENT 'The language.langcode of the original variant of this shortcut.',
      `weight` int(11) NOT NULL DEFAULT '0' COMMENT 'Weight among shortcuts in the same shortcut set.',
      `route_name` varchar(255) DEFAULT NULL COMMENT 'The machine name of a defined Symfony Route this menu item represents.',
      `route_parameters` longblob COMMENT 'Serialized array of route parameters of this shortcut.',
      PRIMARY KEY (`id`),
      UNIQUE KEY `uuid` (`uuid`)
    )

    After

    CREATE TABLE `shortcut` (
      `id` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'The ID of the shortcut.',
      `shortcut_set` varchar(255) NOT NULL COMMENT 'The bundle of the shortcut.',
      `uuid` varchar(128) NOT NULL COMMENT 'The UUID of the shortcut.',
      `langcode` varchar(12) NOT NULL COMMENT 'The language code of the shortcut.',
      PRIMARY KEY (`id`),
      UNIQUE KEY `shortcut__uuid` (`uuid`),
      KEY `shortcut_field__shortcut_set__target_id` (`shortcut_set`)
    )

    The missing properties are now found in the shortcut_field_data table. I'm not sure if this done intentionally or not.

    Before

    CREATE TABLE `shortcut_field_data` (
      `id` int(10) unsigned NOT NULL COMMENT 'The shortcut.id of the shortcut.',
      `langcode` varchar(12) NOT NULL DEFAULT '' COMMENT 'The language.langcode of this variant of this shortcut.',
      `default_langcode` int(11) NOT NULL DEFAULT '1' COMMENT 'Boolean indicating whether the current variant is in the original entity language.',
      `title` varchar(32) DEFAULT NULL COMMENT 'The title of the shortcut.',
      PRIMARY KEY (`id`,`langcode`)
    )

    After

    CREATE TABLE `shortcut_field_data` (
      `id` int(10) unsigned NOT NULL COMMENT 'The ID of the shortcut.',
      `shortcut_set` varchar(255) NOT NULL COMMENT 'The bundle of the shortcut.',
      `langcode` varchar(12) NOT NULL COMMENT 'The language code of the shortcut.',
      `title` varchar(255) DEFAULT NULL COMMENT 'The name of the shortcut.',
      `weight` int(11) DEFAULT NULL COMMENT 'Weight among shortcuts in the same shortcut set.',
      `route_name` varchar(255) DEFAULT NULL COMMENT 'The machine name of a defined Route this shortcut represents.',
      `route_parameters` longblob COMMENT 'A serialized array of route parameters of this shortcut.',
      `default_langcode` tinyint(4) NOT NULL DEFAULT '1' COMMENT 'Boolean indicating whether field values are in the default entity language.',
      PRIMARY KEY (`id`,`langcode`),
      KEY `shortcut_field__shortcut_set__target_id` (`shortcut_set`)
    )
tstoeckler’s picture

In comment.install, we had the following entry for the Comment entity's cid field.

'cid' => array(
  'type' => 'serial',
  'not null' => TRUE,
  'description' => 'Primary Key: Unique comment ID.',
),

which ends up having length 11.

In the patch, we declare this field as an int, which ends up having length 10. Many incrementing fields were 11 and now are 10 in length.

Hmm... actually the ID field of an entity is declared as serial. See ContentEntitySchemaHandler::processBaseTable(). I don't know where the different lengths come from.

The shortcut table is missing the following properties: weight, route_name, route_parameters

...

The missing properties are now found in the shortcut_field_data table. I'm not sure if this done intentionally or not.

Yes, this is intentional. Those properties were probably not put on the base table because they are not translatable. However, we put all fields on the data table - translatable or not - so that we don't have to join on the base table on normal load queries. We should check the actual queries, however, whether this actually works currently. But, yes, that is how it should be. This issue has just revealed an inconsistency where Shortcut module is not following best practices.

plach’s picture

@269:

I probably did not explain myself well: when I say per-entity-type services I just mean that every storage class can implement its own ::createInstance() method and define its own dependencies and services. As long as we return an implementation of TableMappingFactoryInterface (I guess), there is really no point in requiring the service string identifier to be fixed. For instance:

<?php
class ContentEntityDatabaseStorage extends ContentEntityStorageBase implements SqlStorageInterface, ContentEntitySchemaHandlerInterface {

  public function
__construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, FieldInfo $field_info, TableMappingFactoryInterface $table_mapping_factory) {
   
// ...
 
}

}

class
NodeStorage extends ContentEntityDatabaseStorage {

 
/**
   * {@inheritdoc}
   */
 
public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
    return new static(
     
$entity_type,
     
$container->get('database'),
     
$container->get('entity.manager'),
     
$container->get('field.info'),
     
$container->get('node.table_mapping_factory')
    );
  }

}

class
CommentStorage extends ContentEntityDatabaseStorage {

 
/**
   * {@inheritdoc}
   */
 
public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
    return new static(
     
$entity_type,
     
$container->get('database'),
     
$container->get('entity.manager'),
     
$container->get('field.info'),
     
$container->get('comment.table_mapping_factory')
    );
  }

}
?>

@275:

We could do this as a follow-up, but I think it would imply a small (probably irrelevant) API change: we would probably end-up changing the services passed to ContentEntityDatabaseStorage::__construct(). I think it would be perfectly fine, though, this is getting huge.

plach’s picture

Wrt the schema differences outlined in #248 and #277, here are my thoughts:

  1. For serial id columns the int display length difference is due to the fact that we are now declaring those as unsigned, so they require one-less character to be displayed, in fact both int(11) and int(10) unsigned would display 10 digits (as they both have a 4 byte storage capacity -> 2^32), but the former would optionally display also the minus sign. From my POV the patch is actually fixing things by declaring ids with the unsigned attribute, but we can remove it otherwise.
  2. For aggregator stuff: if making the {aggregator_feed}.image column smaller is a problem (IMHO the current limit of 65535 bytes is ok), we should change the field definition to string_long.
  3. For comment stuff: the only concerning change is the homepage column, in fact by changing it from varchar to text we might make certain type of queries slower:

    Instances of BLOB or TEXT columns in the result of a query that is processed using a temporary table causes the server to use a table on disk rather than in memory because the MEMORY storage engine does not support those data types (see Section 8.8.5, “How MySQL Uses Internal Temporary Tables”). Use of disk incurs a performance penalty, so include BLOB or TEXT columns in the query result only if they are really needed. For example, avoid using SELECT *, which selects all columns.

    (from the MySQL reference manual)
    To address this we should probably change the UriItem schema definition to varchar(2048). This size is supported since MySQL 5.0.3 so we should be fine as our minimum requirement is MySQL 5.0.15.

  4. For custom block stuff: we are changing log from longtext to varchar(255), which is going to severely reduce the revision log maximum size. We probably need to change the log definition type to string_long.
  5. For file stuff: the uri column would be fixed by the UriItem change suggested in #3; the filesize column probably requires a new big_int field item, à la string_long; the status column needs to become a boolean item.
  6. For node stuff: the type column needs to be limited to a max of 32 as the other bundle names; status/promote/sticky definitions should become boolean; the log column should become a string_long.
  7. For shortcut stuff: the shortcut_set column needs to be limited to a max of 32 as the other bundle names.
  8. The other differences in varchar lengths should be fixed by specifying the original lenghts as max_length in the related field definitions, the schema handler should use that information to set the max size of varchar columns.

I think we can fix most differences here but the ones requiring bigger changes (if any) could be deferred to a follow-up (discussed this with @catch in Szeged).

jessebeach’s picture

We could do this as a follow-up, but I think it would imply a small (probably irrelevant) API change: we would probably end-up changing the services passed to ContentEntityDatabaseStorage::__construct(). I think it would be perfectly fine, though, this is getting huge.

I doubt anyone will be this deep into entities and storage during the beta phase. I'd venture we'll be well within the limits of acceptable churn to do this as a followup rather than further complicate an already massive patch.

I logged the issue #2265779: Allow storage classes to define their own dependencies and services.

Berdir’s picture

Nobody is quite sure on how to deal with the dependency injection for base/subclasses and if it's an API change or not I think. We'd like say it's not one but the problem is that as soon as you have a storage that needs something else, you need to duplicate the parent create() and __construct() methods, so changing it *will* break those subclasses.

We also do that for content entity storage, see comment and user implementations. Those would have to change too.

But agreed that we can still do that later.

jessebeach’s picture

StatusFileSize
new302.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-schema-auto-2183231-282.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.74 KB

#280:2 "For aggregator stuff: if making the {aggregator_feed}.image column smaller is a problem (IMHO the current limit of 65535 bytes is ok), we should change the field definition to string_long."

This gives us, conservatively, about 16K characters. I think this length will be plenty.

#280:3, updated UriItem to varchar(2048)

#280:4, the log field is now string_long

#280:5.1, fixed with the change in #280:3

#280:5.2, introduced integer_big type. For the life of me, I can't find tests for the data types, so I don't know how to add coverage for this.

#280:5.3, changed integer to boolean.

#280:6-8, Haven't gotten to them yet.

tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerBigItem.php
    @@ -0,0 +1,42 @@
    +class IntegerBigItem extends IntegerItem {

    I think we should rather introduce a new setting on the existing Integer class. Even though we have String and StringLong, etc. we did the same thing for the 'unsigned' setting instead of introducing an UnsignedInteger class.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UriItem.php
    @@ -52,7 +52,8 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
    +          'length' => (int) $field_definition->getSetting('max_length'),

    Yay, this is an oversight. Nice catch!

  3. +++ b/core/modules/file/lib/Drupal/file/Entity/File.php
    @@ -255,12 +255,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['status'] = FieldDefinition::create('boolean')
           ->setLabel(t('Status'))
           ->setDescription(t('The status of the file, temporary (0) and permanent (1).'));

    It seems we should update the description also?

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new301.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,297 pass(es), 77 fail(s), and 22 exception(s).
[ View ]
tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new655 bytes
new301.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,275 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reverting the UriItem schema change from #283 for now, apparently SQL doesn't like that for some reason.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new684 bytes
new302.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,241 pass(es).
[ View ]

That was my fault. Incorrect merge above...

plach’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UriItem.php
@@ -52,7 +52,7 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
+          'type' => 'text',

This was addressing #280, we should fix the test instead.

jessebeach’s picture

StatusFileSize
new1.28 KB
new302.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Thanks for the reroll on those 8.x changes. That was a nasty merge indeed.

Reverting the UriItem schema change from #283 for now, apparently SQL doesn't like that for some reason.

The Feed entity sets the max_length setting to NULL, causing the length of our varchar to be (int) NULL...BOOM. I've removed the settings override in the Feed Entity so it uses the 2048 default.

I'm working on the remaining comments in #280 now.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new302.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new5.68 KB
  1. 280:6.1, set the length of the 'type' field value to BUNDLE_MAX_LENGTH
    $fields['type'] = FieldDefinition::create('entity_reference')
    ->setLabel(t('Type'))
      ->setDescription(t('The node type.'))
      ->setSettings(array(
        'target_type' => 'node_type',
        'max_length' => BUNDLE_MAX_LENGTH,
      ))
      ->setReadOnly(TRUE);
  2. 280:6.2,

    status/promote/sticky definitions should become boolean

    They are already. Maybe tstoeckler updated these?

  3. 280:6.3

    the log column should become a string_long

    Updated.

  4. 280:7

    Set the max_length field setting to BUNDLE_MAX_LENGTH.

  5. 280:8, the rest of the varchar definitions look fine. The title of the shortcut_set went from 32 to 255. That seems fine.
  6. 285:1
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerBigItem.php
    @@ -0,0 +1,42 @@
    +class IntegerBigItem extends IntegerItem {

    I think we should rather introduce a new setting on the existing Integer class. Even though we have String and StringLong, etc. we did the same thing for the 'unsigned' setting instead of introducing an UnsignedInteger class.

    I just had the File entity set the size of the filesize field to big.

    $fields['filesize'] = FieldDefinition::create('integer')
        ->setLabel(t('File size'))
        ->setDescription(t('The size of the file in bytes.'))
        ->setSettings(array(
          'unsigned' => TRUE,
          'size' => 'big',
        ));
  7. 285:3
    +++ b/core/modules/file/lib/Drupal/file/Entity/File.php
    @@ -255,12 +255,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['status'] = FieldDefinition::create('boolean')
           ->setLabel(t('Status'))
           ->setDescription(t('The status of the file, temporary (0) and permanent (1).'));

    It seems we should update the description also?

    Indeed, updated to: "'The status of the file, temporary (FALSE) and permanent (TRUE).'".

The interdiff includes the UriItem change from #292. I neglected to push that commit to my diffing repo for piling on a few more commits, so...it's there.

tstoeckler’s picture

Without having diffed the schema myself (in a while, at least) the changes look good to me.

Except for one little thing:

+++ b/core/modules/file/lib/Drupal/file/Entity/File.php
@@ -255,14 +255,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
-      ->setSetting('unsigned', TRUE);
+      ->setSettings(array(
+        'unsigned' => TRUE,
+        'size' => 'big',
+      ));

Here and elsewhere:

Minor, but please use setSetting() multiple times instead of setSettings(). See #2236903: DataDefinition::setSettings() removes other settings (unintended) for the reasoning. (Only fix this when re-rolling anyway: again, it's minor! :-))

tstoeckler’s picture

+++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
@@ -356,7 +356,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+        'max_length' => BUNDLE_MAX_LENGTH,

+++ b/core/modules/shortcut/src/Entity/Shortcut.php
@@ -167,7 +167,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+        'max_length' => BUNDLE_MAX_LENGTH,

Oops, forgot this part: This is now EntityTypeInterface::BUNDLE_MAX_LENGTH

That's why the install is failing.