Problem/Motivation

EntityDefinitionUpdateTest fails currently with PostgreSQL as database backend.

  1. This exception is related to #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest, and fixed.
  2. Another exception, which seems to be that PostgreSQL is creating/updating a field with a not null constraint when the test says that it should create it without that constraint.
    Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23502]: Not null violation: 7 ERROR: null value in column "new_bundle_field_shape" violates not-null constraint DETAIL: Failing row contains (entity_test_update, 0, 1, 1, und, 0, null, !0Po&a!d). in Drupal\Core\Database\Driver\pgsql\Connection->query() (line 149 of /var/www/drupal8.dev/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php).
    
    Drupal\Core\Database\Driver\pgsql\Connection->query(Object, Array, Array)
    Drupal\Core\Database\Driver\pgsql\Insert->execute()
    Drupal\system\Tests\Entity\EntityDefinitionUpdateTest->testBundleFieldCreateDeleteWithExistingEntities()
    

See why IntegrityConstraintViolationException is being thrown instead of DatabaseWrapperException in pgsql driver and see what mysql driver is doing. Maybe sqlite driver too?

Proposed resolution

Current patch allows the test to catch the appropriate exceptions. The test can then call pass within the catch block.

Remaining tasks

Write patch.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mradcliffe’s picture

Looking into this a bit more regarding the second exception.

mradcliffe’s picture

Oh, wait, the test for this exception SHOULD throw an exception. That's an odd way of doing that in this test. This seems to be added in #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities.

Maybe testBundleFieldCreateDeleteWithExistingEntities should be changed to expect the exception? Is that possible with SimpleTest? It's possible in PHPUnit.

Berdir’s picture

No, it's not possible. that's the common way to do it in Simplenews. It also wouldn't help to change it..

The problem seems to be that PostgreSQL throws a different exception that is not a DatabaseExceptionWrapper ?

I guess we just need to change this to catch DatabaseException instead, which is an interface that both the wrapper and IntegrityConstraintViolationException implement.

Berdir’s picture

Oh, the thing that is uncommon is to then have another test in the catch(). I think the more common way is to just put a $this->pass() in there and have the code there below. But no need to change that here.

mradcliffe’s picture

Issue summary: View changes

Oh, right. Thanks, @Berdir.

daffie’s picture

Status: Active » Needs review
FileSize
1.69 KB

After this fix a get the following error:

Pass      Other      EntityDefinitionU  202 Drupal\system\Tests\Entity\EntityDe
    EntityDefinitionUpdateManager reports the expected change summary.
Exception Uncaught e Connection.php     152 Drupal\Core\Database\Driver\pgsql\C
    Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42704]: Undefined
    object: 7 ERROR:  index
    "drupal_sgaxxfwfxowfo7gtucqdrw8ltjwzbwmwj_bseumc_cs_idx" does not
    exist: DROP INDEX drupal_SgAXXFWfxoWfo7GTUcqDrw8lTjWzBwMWJ_bsEuMC_cs_idx;
    Array
    (
    )
     in
    Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->deleteSharedTableSchema()
    (line 1190 of
    /var/www/drupal80.com/drupal/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).
    Drupal\Core\Database\Driver\pgsql\Connection->query('DROP INDEX
    drupal_SgAXXFWfxoWfo7GTUcqDrw8lTjWzBwMWJ_bsEuMC_cs_idx')
    Drupal\Core\Database\Driver\pgsql\Schema->dropIndex('entity_test_update',
    'entity_test_update_field__new_base_field__format')
    Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->deleteSharedTableSchema(Object,
    NULL)
    Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->performFieldSchemaOperation('delete',
    Object)
    Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->onFieldStorageDefinitionDelete(Object)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->onFieldStorageDefinitionDelete(Object)
    Drupal\Core\Entity\EntityManager->onFieldStorageDefinitionDelete(Object)
    Drupal\Core\Entity\EntityDefinitionUpdateManager->applyUpdates()
    Drupal\system\Tests\Entity\EntityDefinitionUpdateTest->testBaseFieldCreateUpdateDeleteWithoutData()
    Drupal\simpletest\TestBase->run(Array)
    simpletest_script_run_one_test('20',
    'Drupal\system\Tests\Entity\EntityDefinitionUpdateTest')
Berdir’s picture

daffie’s picture

FileSize
7.51 KB

I have added the patch from #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest to the patch from comment #6. Also addded the following lines:

diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
index d4a96b3..0dd1dc6 100644
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -549,6 +549,8 @@ public function fieldSetNoDefault($table, $field) {
   public function indexExists($table, $name) {
     // Details http://www.postgresql.org/docs/8.3/interactive/view-pg-indexes.html
     $index_name = $this->ensureIdentifiersLength($table, $name, 'idx');
+    // Remove leading and trailing quots.
+    $index_name = substr($index_name, 1, -1);
     return (bool) $this->connection->query("SELECT 1 FROM pg_indexes WHERE indexname = '$index_name'")->fetchField();
   }

@@ -562,6 +564,8 @@ public function indexExists($table, $name) {
    */
   public function constraintExists($table, $name) {
     $constraint_name = $this->ensureIdentifiersLength($table, $name);
+    // Remove leading and trailing quots.
+    $constraint_name = substr($constraint_name, 1, -1);
     return (bool) $this->connection->query("SELECT 1 FROM pg_constraint WHERE conname = '$constraint_name'")->fetchField();
   }

The EntityDefinitionUpdateTest passes with this patch.

daffie’s picture

FileSize
5.82 KB

Forgot the interdiff.txt.

bzrudi71’s picture

Nice to see a solution here :-)

  1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -70,7 +70,7 @@ protected function ensureIdentifiersLength($identifier) {
    +      $saveIdentifier = $this->connection->escapeTable('drupal_' . $this->hashBase64($identifierName) . '_' . $args[2]);
    

    Just wonder if we really should use escapeTable() to escape index/constraint names, looks a bit odd even if it does the right thing. I think we should add the quotes directly without the use of escapeTable()? Also there should be a comment explaining why we need to quote.

  2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -549,6 +549,8 @@ public function fieldSetNoDefault($table, $field) {
    +    // Remove leading and trailing quots.
    

    nitpick: quots > quotes

  3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -562,6 +564,8 @@ public function indexExists($table, $name) {
    +    // Remove leading and trailing quots.
    

    same here

daffie’s picture

FileSize
7.49 KB
1.76 KB

Thank for the review bzrudi71.
Fixed all points from comment #10.

mradcliffe’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -549,6 +549,8 @@ public function fieldSetNoDefault($table, $field) {
+    // Remove leading and trailing quotes.
+    $index_name = substr($index_name, 1, -1);

This is more of a question.

What happens when the identifier is not hashed/quoted? Wouldn't this arbitrarily trim left and right characters of the identifier?

I am not sure.

bzrudi71’s picture

Status: Needs review » Needs work

@mradcliffe: Yes, you are right, my bad, I have overseen that. In case ensureIdentifiersLenght() does not rewrite the identifier we cant rely on that.

daffie’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
7.5 KB

Can we solve the removing of the leading and trailing quotes with: $index_name = str_replace('"', '', $index_name);?

bzrudi71’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
vlad.n’s picture

Issue tags: -Needs reroll
FileSize
1.7 KB

Re-roll.

bzrudi71’s picture

Status: Needs work » Needs review

Thanks @vlad.n for the reroll!

mradcliffe’s picture

Issue summary: View changes

Updated issue summary.

bzrudi71’s picture

Status: Needs review » Reviewed & tested by the community

This all looks good to me now, thanks all! RTBC for me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 7e24033 and pushed to 8.0.x. Thanks!

No new test fails are introduced by this fix for SQLite - this test does fail on SQLite but for another unrelated reason.

  • alexpott committed 7e24033 on 8.0.x
    Issue #2443663 by daffie, vlad.n: PostgreSQL: Fix system\Tests\Entity\...

Status: Fixed » Needs work

The last submitted patch, 16: 2443663-16.patch, failed testing.

daffie’s picture

Status: Needs work » Fixed

Stupid testbot!

Status: Fixed » Closed (fixed)

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