SqlContentEntityStorageSchema does not detect column-level schema changes in non-base-fields

Thanks for opening this, +1 on what I read so far (both code and comments :)

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1758,4 +1759,48 @@ protected function isTableEmpty($table_name) {
   +    $table_mapping = $this->storage->getTableMapping();
   +
   +    $schema = [];
   +    if ($table_mapping->requiresDedicatedTableStorage($storage_definition)) {
   +      $schema = $this->getDedicatedTableSchema($storage_definition);
   +    }
   +    elseif ($table_mapping->allowsSharedTableStorage($storage_definition)) {
   +      $field_name = $storage_definition->getName();
   +      foreach (array_diff($table_mapping->getTableNames(), $table_mapping->getDedicatedTableNames()) as $table_name) {
   +        if (in_array($field_name, $table_mapping->getFieldNames($table_name))) {
   +          $column_names = $table_mapping->getColumnNames($storage_definition->getName());
   +          $schema[$table_name] = $this->getSharedTableFieldSchema($storage_definition, $table_name, $column_names);
   +        }
   +      }
   +    }
   

   We have almost this exact same logic in SqlContentEntityStorageSchema::requiresFieldStorageSchemaChanges(), would it make sense to make the retrieving of the schema into its own method?

   And wonder if we need to add an else case (for custom storage?) that returns FALSE directly, as in such case $schema[$table]['fields'] won't be set and the schema comparison further down will give a notice.

2. +++ b/core/modules/field/src/Tests/Entity/Update/SqlContentEntityStorageSchemaColumnTest.php
   @@ -0,0 +1,106 @@
   +   * Tests column schema-changes are detected for fields with data.
   

   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1758,4 +1759,48 @@ protected function isTableEmpty($table_name) {
   +   * Checks if there are column schema changes.
   

   When we say "column schema-changes" (here and elsewhere in the patch) it may be a bit unclear what we are talking about, do we have such a thing as a "column schema"? Maybe rather something like "Compares schemas to check for changes in the column definitions"?

3. +++ b/core/modules/field/src/Tests/Entity/Update/SqlContentEntityStorageSchemaColumnTest.php
   @@ -0,0 +1,106 @@
   + * Schema changes in fields with data are detected during automatic updates.
   

   Would prepending "Tests that" still make this fit within the 80 cols? :)

4. +++ b/core/modules/field/src/Tests/Entity/Update/SqlContentEntityStorageSchemaColumnTest.php
   @@ -0,0 +1,106 @@
   +
   +
   

   2 empty lines at the end of setUp()

Discussed with @catch, @xjm, @effulgentsia and @webchick, we all agreed that this is a critical issue has a silent fail and updating of the stored schema to something different than the actual schema could cause serious bugs even if this in not called from uppdate.php.

Let me see about adding a patch that addresses comments in #10

actually that isset() shouldn't be needed now that we check for custom storage

Tagging.

The patch looks really great for me.
!

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -186,27 +186,43 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
   +    if ($storage_definition->hasCustomStorage()) {
   +      // The field has custom storage, so we don't know if a schema change is
   +      // needed or not, but since per the initial checks earlier in this
   +      // function, nothing about the definition changed that we manage, we
   +      // return FALSE.
   +      return FALSE;
   +    }
   

   Here is a question, while schema is bound to SQL, custom storages might have a similar concept, so I'm wondering whether we need a new issue about allowing custom storage to opt into that somehow.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -186,27 +186,43 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
   +   *   storage, i.e. the storage must take care of storing the field.
   +   * @return array
   

   Nitpick: missing empty line

Looks great, thanks!

RTBC +1

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -186,27 +186,43 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
+    assert('!$storage_definition->hasCustomStorage();');

nice :)

@dawehner:

Here is a question, while schema is bound to SQL, custom storages might have a similar concept, so I'm wondering whether we need a new issue about allowing custom storage to opt into that somehow.

As I mentioned during the previous critical meeting call, there is a flaw in the API I was not able to address: schema is not necessarily a SQL-specifc concept, but field types return a Schema API definition in their ::schema() method. Ideally we should be able to derive also that schema from field property definitions. The issue you are bringing up is related and a dedicated issue to discuss that makes totally sense to me.

Adding that missing empty line.

Committed 6bf0202 and pushed to 8.0.x. Thanks!

This may have introduced a PHP7 regression, triggering testbot to confirm.

In another issue we got "Fatal error: Uncaught Error: Cannot access protected property Drupal\user\UserStorageSchema::$storage in /var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:212" on Drupal\ban\Tests\IpAddressBlockingTest

@neclimdul is looking at this

Tried to run a site install using a build of PHP7 from today. I can confirm this is an error.

vagrant@php7dev:~/drupal$ drush  si  --db-url=mysql://root:pass@localhost/drupal
You are about to DROP all tables in your 'drupal' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a while. Consider  [ok]
using the --notify global option.
Error: Cannot access protected property Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::$storage in Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->getSchemaFromStorageDefinition() (line 212 of /home/vagrant/drupal/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).
Drush command terminated abnormally due to an unrecoverable [error]
error.

neclimdul> i tried mocking a test case and it was fine so... this might be some sort of obtuse interaction of references or something...

This looks to be a PHP7 bug, but maybe we can do a workaround ourselves...

Opened #2547715: Cannot access protected property $this->storage on Install inside SqlContentEntityStorageSchema:: for the bug found in #22.