Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
EntityDefinitionUpdateTest fails currently with PostgreSQL as database backend.
- This exception is related to #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest, and fixed.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#16 | 2443663-16.patch | 1.7 KB | vlad.n |
#14 | 2443663-14.patch | 7.5 KB | daffie |
#14 | interdiff-2443663-11-14.txt | 1.22 KB | daffie |
Comments
Comment #1
mradcliffeLooking into this a bit more regarding the second exception.
Comment #2
mradcliffeOh, 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.
Comment #3
BerdirNo, 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.
Comment #4
BerdirOh, 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.
Comment #5
mradcliffeOh, right. Thanks, @Berdir.
Comment #6
daffie CreditAttribution: daffie commentedAfter this fix a get the following error:
Comment #7
BerdirSounds like the same problem as #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest ?
Comment #8
daffie CreditAttribution: daffie commentedI have added the patch from #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest to the patch from comment #6. Also addded the following lines:
The EntityDefinitionUpdateTest passes with this patch.
Comment #9
daffie CreditAttribution: daffie commentedForgot the interdiff.txt.
Comment #10
bzrudi71 CreditAttribution: bzrudi71 commentedNice to see a solution here :-)
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.
nitpick: quots > quotes
same here
Comment #11
daffie CreditAttribution: daffie commentedThank for the review bzrudi71.
Fixed all points from comment #10.
Comment #12
mradcliffeThis 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.
Comment #13
bzrudi71 CreditAttribution: bzrudi71 commented@mradcliffe: Yes, you are right, my bad, I have overseen that. In case ensureIdentifiersLenght() does not rewrite the identifier we cant rely on that.
Comment #14
daffie CreditAttribution: daffie commentedCan we solve the removing of the leading and trailing quotes with:
$index_name = str_replace('"', '', $index_name);
?Comment #15
bzrudi71 CreditAttribution: bzrudi71 commentedComment #16
vlad.n CreditAttribution: vlad.n commentedRe-roll.
Comment #17
bzrudi71 CreditAttribution: bzrudi71 commentedThanks @vlad.n for the reroll!
Comment #18
mradcliffeUpdated issue summary.
Comment #19
bzrudi71 CreditAttribution: bzrudi71 commentedThis all looks good to me now, thanks all! RTBC for me.
Comment #20
alexpottThis 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.
Comment #23
daffie CreditAttribution: daffie commentedStupid testbot!