Problem/Motivation

Schema::fieldSetDefault and Schema::fieldSetNoDefault, and their legacy procedural counterparts db_field_set_default and db_set_no_default, are implemented in the core db drivers, and are tested in SchemaTest, but in fact are used nowhere in core.

IMHO all these methods should just be deprecated, in favor of Schema::changeField with a full field specification passed in.

Some db systems may not have the DDL specific for adding/removing only the default value of a field, forcing therefore to use changeField anyway. But in that case you would have to introspect the field spec from the actual field to be changed, and there are cases when this can be messy (for example to determine if a field is unsigned).

Proposed resolution

Deprecate Schema::fieldSetDefault, Schema::fieldSetNoDefault, as well as db_field_set_default and db_set_no_default.

Remaining tasks

1. Discuss/agree if this makes sense
2. Write patch
3. The rest

User interface changes

none

API changes

2 public methods in the Database API deprecated

Data model changes

none

CommentFileSizeAuthor
#25 2993663-25.patch16.55 KBmondrake
#25 interdiff_23-25.txt1.59 KBmondrake
#23 interdiff_21-23.txt1.35 KBmondrake
#23 2993663-23.patch16.46 KBmondrake
#21 2993663-21.patch16.48 KBmondrake
#21 interdiff_14-21.txt11.32 KBmondrake
#14 interdiff_9-14.txt1.1 KBmondrake
#14 2993663-14.patch14.67 KBmondrake
#9 2993663-9.patch13.84 KBmondrake
#9 interdiff_7-9.txt781 bytesmondrake
#7 2993663-7.patch13.84 KBmondrake
#7 interdiff_6-7.txt2.92 KBmondrake
#6 interdiff_4-6.txt2.78 KBmondrake
#6 2993663-6.patch10.92 KBmondrake
#4 2993663-4.patch8.13 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented
Parent issue: #2848161: [meta] Replace calls to deprecated db_*() wrappers » #2319859: [META] Deprecate contents of database.inc
Related issues: +#2848161: [meta] Replace calls to deprecated db_*() wrappers
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 2993663-4.patch8.13 KB

Let's see what actually breaks by adding the deprecation message.

Status: Needs review » Needs work

The last submitted patch, 4: 2993663-4.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 2993663-6.patch10.92 KB
new interdiff_4-6.txt2.78 KB

#4 confirms the IS

Schema::fieldSetDefault and Schema::fieldSetNoDefault, and their legacy procedural counterparts db_field_set_default and db_set_no_default, are implemented in the core db drivers, and are tested in SchemaTest, but in fact are used nowhere in core.

Here, changing the SchemaTest to use Schema::changeField with a full field specification passed in.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new interdiff_6-7.txt2.92 KB
new 2993663-7.patch13.84 KB

Now, with deprecation tests.

If we do this, we need to have a CR and then adjust the deprecation messages.

Status: Needs review » Needs work

The last submitted patch, 7: 2993663-7.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_7-9.txt781 bytes
new 2993663-9.patch13.84 KB

...

andypost’s picture

andypost
he/him
Primary language Russian
commented

This change makes API incomplete - you causes contrib to provide full field definition instead of just managing column default value
Then we need complimentary method to retrieve current field definition & cast it to schema definition just to allow change default value(

PS: Also \Drupal\Core\Database\Schema::escapeDefaultValue() looks orphan without this methods

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -109,12 +109,12 @@ public function testSchema() {
-    $this->schema->fieldSetDefault('test_table', 'test_field', 0);
+    $this->schema->changeField('test_table', 'test_field', 'test_field', ['type' => 'int', 'not null' => TRUE, 'default' => 0]);

That's what worries loosing this api-calls without alternative to get current definition of the field (I found no public way to get introspection from schema)

mondrake’s picture

mondrake
Location 🇮🇹
commented

This change makes API incomplete - you causes contrib to provide full field definition instead of just managing column default value

True, but:

  1. from D9 on, not now
  2. do we really want contrib to change default value for tables that they are not owning - for example contrib to change a core table?
  3. if contrib is managing their own stuff, they should be aware of their full definitions

In other words -- IMHO changeField should be the most atomic method to deal with changes on table colums, to ensure integrity. Suppose you have a column defined as Unsigned. You change default to -1. What happens? If you had changeField somehow validating the combination of column attributes (not there yet, but directionally...), you could avoid this ambiguity.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I did some digging to find when this was first introduced, and to see if I could find when it would actually be useful, as I am not really sure of the use case of changing a column default at runtime, except perhaps during an update hook.

db_field_set_default() was first added in the patch in https://www.drupal.org/project/drupal/issues/136171#comment-541294 which was the precursor to the Schema API introduced in the Drupal 6 development cycle. There is a cursory mention of "there are now table manipulation functions: db_add_field, db_drop_field, db_create_index, db_drop_index, db_set_default_value" but it doesn't explain why the last one is actually useful.

I found a couple of mentions in old contrib:

https://cgit.drupalcode.org/boost/tree/boost.install?h=6.x-1.x#n950
https://cgit.drupalcode.org/mass_contact/tree/mass_contact.install?h=6.x...
https://cgit.drupalcode.org/taxonomy_display/tree/taxonomy_display.insta...

All of these cases are where the module owns the schema in question and so they wouldn't need to introspect it.

I am struggling to think of a case where something might want to alter a schema that it doesn't own, yet doesn't know enough about that schema to be able to use the changeField() method.

mondrake’s picture

mondrake
Location 🇮🇹
commented

(...) I am not really sure of the use case of changing a column default at runtime, except perhaps during an update hook. (...)
I am struggling to think of a case where something might want to alter a schema that it doesn't own, yet doesn't know enough about that schema to be able to use the changeField() method.

Exactly, +1. This seems a 'shortcut' for use in update hooks. But contrib have to keep their hook_schema implementation aligned, to ensure that new installation get the same db structure as the one that results from updates, otherwise hell breaks loose. And, if the hook_schema is aligned, the full field spec is available from code without any need to introspect.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2993663-14.patch14.67 KB
new interdiff_9-14.txt1.1 KB

This should fix the Pgsql failure, that was due to using the now deprecated method.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thank you @longwave

This is not ready yet for prime time, but leaving RTBC in the hope of getting core commiter's feeback on the direction.

If we are going ahead in this, the todos are:

  1. write a CR
  2. reference the CR in the deprecation message and adjust the Drupal release of deprecation
  3. adjust the legacy test to actually test the effect of the method (now the SchemaTest has been changed to use changeField, and nothing is testing fieldSet(No)Default anymore

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2993663-14.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need the list of things in #16 done.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake

I will work on that

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs change record
Parent issue: #2319859: [META] Deprecate contents of database.inc » #2848161: [meta] Replace calls to deprecated db_*() wrappers
StatusFileSize
new interdiff_14-21.txt11.32 KB
new 2993663-21.patch16.48 KB
mondrake’s picture

mondrake
Location 🇮🇹
commented

Addressed points in #16. Added draft CR here https://www.drupal.org/node/2999035 and adjusted the overall db_* deprecation CR here https://www.drupal.org/node/2993033.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2993663-23.patch16.46 KB
new interdiff_21-23.txt1.35 KB

Interesting. in case of a database insertion that fails to comply to constraints, pgSql and SQLite drivers throw a much more accurate exception (IntegrityConstraintViolationException) than MySql (DatabaseExceptionWrapper). Here just catching the common base interface class, then.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
@@ -114,6 +115,79 @@ public function testDbChangeField() {
+    catch (DatabaseException $e) {
+      // Do nothing.
...
+    catch (DatabaseException $e) {
+      // Do nothing.

Maybe better to assertFalse(TRUE) here?

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new interdiff_23-25.txt1.59 KB
new 2993663-25.patch16.55 KB

#24 here we are deliberately executing an insert that is due to fail and raise an exception, so whatever is in the catch block cannot fail the test.

Let's make things a bit clearer.

The MySql 8 failure in #23 is unrelated.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Great! Now it looks polished)

  • catch committed ca4076c on 8.7.x 
    Issue #2993663 by mondrake, andypost: Deprecate Schema::fieldSetDefault...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed ca4076c and pushed to 8.7.x. Thanks!

mondrake’s picture

mondrake
Location 🇮🇹
commented

Published CR https://www.drupal.org/node/2999035

mondrake’s picture

mondrake
Location 🇮🇹
commented

Opened #2999569: MySql driver throws a generic DatabaseExceptionWrapper for integrity violation at insertion when column has no default as a follow-up for #23.

Status: Fixed » Closed (fixed)

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