In #502522: Allow drupal_alter() on the various Field API declarative hooks we deferred hook_field_schema_alter() to a follow-up issue, but then never opened the follow-up issue. I still think we need it - for optimizing queries both in default SQL storage, and in non-SQL storage.

Barry's argument on that issue was that we don't have hook_field_schema_alter() because there's no hook_schema_alter() - except that we do: http://api.drupal.org/api/function/hook_schema_alter/6

For the reasons we have hook_schema_alter() - mainly site-specific tweaks, we should allow the same for field storage.

CommentFileSizeAuthor
#129 drupal-field_schema_alter-691932-129-D7.patch7.27 KBbrad.bulger
#127 interdiff_125-127.txt2.79 KBAndyF
#127 drupal-field_schema_alter-691932-127-D7.patch7.32 KBAndyF
#125 drupal-691932-125.patch6.83 KBMustangGB
#105 diff-92-105.txt441 bytesdinarcon
#105 field-schema-alter-691932-105.patch6.92 KBdinarcon
#2 field_schema_alter.patch1.87 KBcatch
#16 691932_16.patch2.15 KBBTMash
#16 691932_16-d7.patch2.11 KBBTMash
#21 691932_21.patch2.28 KBBTMash
#26 691932_26.patch3.13 KBzhangtaihao
#32 691932_32.D7.patch3.09 KBBTMash
#36 691932_36.patch6.36 KBBTMash
#39 691932_38.patch6.6 KBBTMash
#39 691932_38_testonly.patch2.9 KBBTMash
#41 691932_40.patch6.98 KBBTMash
#44 691932_44.patch7 KBBTMash
#58 field_d7-backport-for-hook_field_schema_alter_691932_58.patch6.91 KBBarry_Fisher
#65 field_d7-backport-for-hook_field_schema_alter_691932_64.patch6.91 KBwamilton
#83 field_d7-backport-for-hook_field_schema_alter_691932_83.patch7.3 KBShellingfox
#89 field_d7-backport-for-hook_field_schema_alter_691932_89.patch6.36 KBShellingfox
#93 interdiff.txt738 bytesjhedstrom
#92 field-schema-alter-691932-92.patch6.91 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Use case: you want to know when you tagged an entity, ie add a timestamp to a taxonomy_term_reference field.

catch’s picture

Status: Active » Needs review
FileSize
1.87 KB

Here's a patch. One issue we have is that it's currently possible to specify 'indexes' in the field definition, and have these merged in with or overwrite the indexes in hook_field_schema() - this looks like a workaround to make up for the lack of a hook_field_schema_alter(), but it's also an API change if we remove it in favour of this.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Well, given #1 I am RTBC'ing this.

yched’s picture

Status: Reviewed & tested by the community » Needs review

I'm sorry but I need to review this :-).

yched’s picture

Started a post with my thoughts on this, but too late for tonight.
Short version is: folks, this is a can of worms.

Will try to expand later this weekend.
Meanwhile, "for your consideration" : #693054: field_update_field() calls the wrong field storage backend

Pasqualle’s picture

subscribe

catch’s picture

yched is afk at the moment, I'm going to guess what his concern is though - apologies if I've guessed wrong.

Something like: "we spent ages coming up with a design that had no dynamic schema and this re-introduces it".

To which I would say:

We allow hook_schema_alter() on any table, including field API tables - this just lets you do that without having to hack core to take advantage of it - also I don't see an obvious use-case apart from custom tweaks and non-SQL storage backends at the moment.

yched’s picture

"we spent ages coming up with a design that had no dynamic schema and this re-introduces it"

Kind of but not exactly :-)

1) Dynamic set of 'components' (columns) for a field value, extensible from a 3rd party module, has never been in the scope of CCK since 4.7 on. I cannot say I have a clear vision of the consequences of opening this can of worms this late.

2) 'field_sql_default' storage can definitely not handle this right now. It has no code to automatically add / remove / update db columns or indexes. No way to ensure no timeouts on big tables. So, enable a module that implements hook_field_schema_alter() on an existing field stored in field_sql_default, and bad things ensue.

3) widgets and formatters rely on a fixed (or at least predictable) set of 'columns' for a given field type. The columns you add with _alter() will only be seen by custom widgets or formatters you write specifically for your own 'extended [text|number|term_ref|whatever] fields'. Same goes for views integration.

2) and 3) still make the alter hook possibly worthy for "custom tweaks and non-SQL storage backends". I wouldn't really know what to write in the PHPdoc for this hook in field.api.php, other that "use this only if you know what you're doing; you're on your own, dude" :-).

But :

4) the current patch only invokes hook_field_schema_alter() in field_create_field(). There are two other places where hook_field_schema() is called :
- field_read_field() - currently, the columns for a field are not stored in the {field_config} table, they are retrieved on each field_read_field() by calling hook_field_schema().
- field_update_field() - changing some field settings might affect the schema (e.g 'max length')
Tricky to decide when to alter and when not to alter :-).
This possibly boils down to : er, how do we stick an alter() hook on stuff that are stored in the db and have load() and update() methods ? I have no existing examples that come to my mind.

yched’s picture

The columns you add with _alter() will only be seen by custom widgets or formatters you write specifically for your own 'extended [text|number|term_ref|whatever] fields'

And those custom widgets/formatters will show up in Field UI as available for all fields of the same base type, but will probably break on 'non-altered' fields, unless you take extra precautions like 'is this column present ?'.

catch’s picture

I think if you want a new widget / formatter you should be creating your own field based on an existing one (like filefield and imagefield). chx's use case for a timestamp stored with term references doesn't need a widget/formatter - it just needs to be able to define a column and populate it on save / update.

So in other words "use this only if you know what you're doing; you're on your own, dude" yes exactly - we have other hooks in core which are similar.

I'll have a look at #4.

catch’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Alright I looked at #4, and it either means invoking the same hook from three places, which is ooky, or actually storing the column definition in $data which doesn't seem overly bad, but would take some refactoring. So moving to D8 for now.

catch’s picture

heshanlk’s picture

sub

droplet’s picture

can I ask.. will it backport to D7 ? (I can hack core first if it planned..)
If not, is it means no way to change field schema in D7 ?
thanks.

ParisLiakos’s picture

Subscribe :(
I wish this gets backported but thats unlikely i guess

BTMash’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
2.15 KB
2.11 KB

So here is another use case for D7.

Lets say you have a field that has a limited amount of room that it offer (say, the image module with 128 characters for title/alt like at #1015916: Image field "title" allows more data than database column size.). If someone wanted to create a contrib module to rectify this as a possible interim solution, a module needs to be able to alter the amount of room that takes up. So far, this involves:

  • Implement hook_schema_alter
  • Implement hook_create_field
  • Implement hook_update_field

And a bunch of other things. At the 'create field' point, you can change the the db field and make way with working on things. However, the update is where things get bad. If you update the field, it will go back to what the original value was at (so if we had the title at 128 chars, updated it to 1024 chars, updating any field info will revert it back to 128 chars and screwing up any data that may have been in there. Your update hook will bring back the length but that is after the fact). This hook is needed badly for the 7.x branch as well. Can we have a version of both D7/D8 that actually makes this possible and then have a followup issue for D8 to get this cleaned up? Attaching a patch and backport.

zhangtaihao’s picture

@BTMash: This is probably naive on my part, but couldn't you implement hook_field_update_forbid to prevent exactly that from happening? Of course, this would stunt the ability to change the widget (and settings), but given the change would potentially screw up lots of data, implementing hook_field_update_forbid seems sensible.

EDIT: Also, is it just me, or has every patch up to now been documenting hook_schema_alter instead of hook_field_schema_alter? I assume this is a slight typo.

BTMash’s picture

@zhangtaihao, I was trying to fully understand hook_field_update_forbid but if another module wants to do a relevant update, I am wondering how this messes with that. In either case, if modules are able to alter the schema for non-field tables (with hook_schema_alter, it makes sense that field tables should be getting altered as well (or they should go through a generic function to retrieve the schema for a field that can call on hook_schema_alter or the like.

zhangtaihao’s picture

@yched:

The current patch only invokes hook_field_schema_alter() in field_create_field(). There are two other places where hook_field_schema() is called [...] Tricky to decide when to alter and when not to alter

Forgive me if I'm missing the bigger picture, but isn't this where the code is traditionally refactored to use a new function (also to reference @BTMash re: generic function) called something like field_get_schema() which invokes hook_field_schema() and drupal_alter()'s it?

zhangtaihao’s picture

@BTMash: Sorry, I didn't realize you were presenting a case for hook_field_schema_alter in contrast to the current changes you've had to make to achieve a similar effect. My bad.

BTMash’s picture

FileSize
2.28 KB

@zhangtaihao: no worries :) I need to work on how I convey my message across.

Attaching a patch which creates a function _field_retrieve_schema which calls on the common lines called to get the schema for the fields. I'm not sure where the function should get moved, however.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, 691932_21.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review

#21: 691932_21.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 691932_21.patch, failed testing.

zhangtaihao’s picture

I believe the function _field_retrieve_schema should be public instead, not least because others will find it useful, but also to match the coding style in core, which makes both drupal_get_schema() and drupal_get_complete_schema() (which is called by the former) public functions.

Anyway, it is curious the kinds of errors this patch generates.

zhangtaihao’s picture

FileSize
3.13 KB

I have added the default definition for 'foreign keys' to @BTMash's patch in #21 to see what happens. I have also merged in the documentation of the hook.

Oh yes, I made the function public just for now. Is there any reason against that?

zhangtaihao’s picture

Status: Needs work » Needs review

Oops.

BTMash’s picture

Issue tags: +Needs tests

I can't do a review on this since its similar to what I wrote but it looks good to me :) I'm wondering if there might be a way to create tests for this that show the field schema alterations are working (actually...are there tests for hook_schema_alter?).

zhangtaihao’s picture

I think the main problem lies in the sheer breadth of a schema alteration. The point is to find something common, but I'm not entirely sure comparing schema arrays qualifies as a valid test (just a hunch though).

Nevertheless, considering this actually departs somewhat from hook_schema_alter because the field schema is detached from the underlying storage architecture, a possible test is to verify the high-impact alterations, namely modified/added/removed field schema definitions and how they affect field CRUD operations. I haven't thought about the details of these tests yet, just throwing in some ideas. :)

I suppose that, regarding the test for the actual altering mechanism, we can reference some other alteration tests elsewhere in the core test suite (search for "alter" in http://qa.drupal.org/pifr/test/197763).

EDIT:
And no, I can't seem to find a test for hook_schema_alter. There are some tests doing drupal_alter(), but they mostly deal with what gets invoked, in what order, and ultimately whether the changes came through.

BTMash’s picture

@zhangtaihao, Yep, I saw the same tests (and we really just needed to see that the alterations would occur correctly; the drupal_alter tests should be taking care of that). So unless I'm missing something, there isn't a reason to write tests for a field schema alter.

zhangtaihao’s picture

Issue tags: -Needs tests

Agreed. Perhaps one of us can work on the D7 backport.

BTMash’s picture

FileSize
3.09 KB

Rolled a D7 version of your patch.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This patch looks simple and straightforward. Small point about the API documentation:

  1. +++ b/core/modules/field/field.api.phpundefined
    @@ -272,6 +272,17 @@ function hook_field_schema($field) {
    + * Allows modules to alter the schema for a field.
    

    Unlike normal function documentation, hook docs in .api.php files get the imperative instead of the indicative for summaries (i.e. "Allow" instead of "Allows"). I didn't make the rule so don't ask me why. :) http://drupal.org/node/1354#hooks

  2. +++ b/core/modules/field/field.api.phpundefined
    @@ -272,6 +272,17 @@ function hook_field_schema($field) {
    +function hook_field_schema_alter(&$schema, $field) {
    +}
    +
    

    It would be good to have sample code of how the hook might be used in here.

Additionally, we should add an automated test for the new alter hook.

xjm’s picture

Oh, also, let's get @see between the hook and its function or otherwise explain how it's invoked. Edit: and probably to field_create_field() too?

BTMash’s picture

Assigned: Unassigned » BTMash

Assigning it to self so I remember to work on your recommended changes.

BTMash’s picture

Assigned: BTMash » Unassigned
Status: Needs work » Needs review
FileSize
6.36 KB

Ok, here is a new stab at your recommended changes and a test case.

zhangtaihao’s picture

I understand it may be commonplace, but does relying on an image field for a test make core trickier to maintain?

BTMash’s picture

Good point. We'll want to split the hook off into its own module and we can perform a schema_alter on the field_test module's field. Most of the code should remain the same :)

BTMash’s picture

Here is a revised patch which uses field_test. Attaching the tests separately (though of course it would fail since it utilizes a new hook) to show a clean fail.

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests +API addition

Alright, I read through the patch again. The fix addresses the concerns of "ookiness" from earlier in the issue by factoring out field_retrieve_schema(), which seems like a good approach to me. The test also looks good, as does the hook documentation.

A few final doxyen and formatting observations:

  1. +++ b/core/modules/field/field.api.phpundefined
    @@ -274,6 +274,32 @@ function hook_field_schema($field) {
    +    $schema['columns']['additional_column'] = array(
    +        'description' => "Addtional column added to image field table.",
    +        'type' => 'varchar',
    +        'length' => 128,
    +        'not null' => FALSE,
    +    );
    
    +++ b/core/modules/field/tests/field_test_schema_alter.moduleundefined
    @@ -0,0 +1,23 @@
    +    $schema['columns']['additional_column'] = array(
    +        'description' => "Addtional column added to image field table.",
    +        'type' => 'varchar',
    +        'length' => 128,
    +        'not null' => FALSE,
    +    );
    

    I think these two arrays have an extra two spaces of indentation for the keys.

  2. +++ b/core/modules/field/field.crud.incundefined
    @@ -20,6 +20,24 @@
    @@ -129,9 +147,7 @@ function field_create_field($field) {
    

    Let's add @see hook_field_schema_alter() at the end of the field_create_field() docblock.

  3. +++ b/core/modules/field/tests/field.testundefined
    @@ -2379,6 +2379,44 @@ class FieldCrudTestCase extends FieldTestCase {
    +class FieldSchemaAlterTestCase extends FieldTestCase {
    

    Need a docblock for this guy.

  4. +++ b/core/modules/field/tests/field.testundefined
    @@ -2379,6 +2379,44 @@ class FieldCrudTestCase extends FieldTestCase {
    +  /**
    +   * Test that new image field gets an altered alt column and a new column.
    +   */
    

    This should be "Tests."

Let's fix those, and then I think this is RTBC.

BTMash’s picture

FileSize
6.98 KB

Alright, this hopefully takes care of the formatting issues :)

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

  1. +++ b/core/modules/field/tests/field.testundefined
    @@ -2379,6 +2379,48 @@ class FieldCrudTestCase extends FieldTestCase {
    + * Check that field schema can be altered by other modules that implement
    + * hook_field_schema_alter().
    

    This should be one line (and again the third-person verb). How about:

    Tests that the field schema can be altered with hook_field_schema_alter().

  2. +++ b/core/modules/field/tests/field.testundefined
    @@ -2379,6 +2379,48 @@ class FieldCrudTestCase extends FieldTestCase {
    +   * Tests that new test field gets an altered value column and a new column.
    

    With the detailed docblock for the class above, maybe for this one we could just do:

    /**
     * Tests a hook_field_schema_alter() implementation.
     *
     * @see field_test_schema_alter_field_schema_alter()
     */
    

    I think that's a little more clear.

I'd reroll with these changes myself, but then I couldn't RTBC it. :)

BTMash’s picture

FileSize
7 KB

Hopefully, third time is the charm :)

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

yched’s picture

Status: Reviewed & tested by the community » Needs review

I need to take a closer look at this. Will try to do so asap.

yched’s picture

Also, I voiced my concerns about the idea of a hook_field_schema_alter() in #8 above, I'm not sure how the latest patch adresses them (notably "field_sql_storage has no way to alter the actual storage tables of existing fields when you enable a module that suddenly alters the schema")

zhangtaihao’s picture

I believe (3) is a basic assumption in implementing hook_field_schema_alter(). The latest patch deals with (4) accordingly.

As for (1), I believe that allowing fields to be altered works in good judgment. A similar story applies in hook_schema_alter(), and yet the core Schema API trusts whichever module implementing it to get it right (or it'll be in for a spanking, e.g. #1268602: Crash after enabling UUID and #1290986: avoid entity_load() during entity info cache rebuilds).

Nevertheless, your point in (2) is valid. Leaving aside the fact that short of deducing the possibility of altered schema by querying hook_modules_{enabled|disabled} for an implementation of hook_field_schema_alter, I can't find any evidence that any bit of content_alter_db() from CCK in D6 has survived. Theoretically, had the function persisted and still be called before writing a field instance config, the schema could have been updated in real time. Assuming alterations are not too radical, the hook would not have introduced too many difficulties.

Should such a storage-level schema alteration mechanism be implemented, are there other immediate concerns?

yched’s picture

CCK's content_alter_db() was intentionally left out of the "Fields in core" move. Rationale : no live altering of schema tables outside update.php and hook_update_N() (CCK let you change schema-altering field properties in the UI). Data tables are potentially huge, and there is no way to ensure this happens with no timeouts.

Also, I don't know if such schema changes could be supported on non-SQL storage backends. For instance, Mongo field storage stores the whole $entity structure. If the "field schema" gets changed, I don't think it can go and deep-update all existing $entity records.

BTMash’s picture

Just so I understand clearly, CCK let ppl change schema altering properties in the ui? If that is the case, then there is no real equivalent in core. At the very least, certain core fields don't (like image and its hardcoded alt/title lengths).

Couldn't it be argued that part of the onus should be on the contrib/custom module that is implementing this hook to handle whatever else needs to be done for the particular application (like implementing hook_install for changing schemas of any existing fields, form_alters, etc) to work correctly without breaking the application? We're already making a similar assumption with hook_schema_alter.

BTMash’s picture

#44: 691932_44.patch queued for re-testing.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

Running into this need in Drupal 7 to simply add a column, as to the Link field to store some metadata about it. Use cases like that or the example in the patch #44 seem very worthwhile.

Let's fix this for SQL backend supported by core and if problems arise with MongoDB or other backends, let them be addressed by people with real experience in them.

catch’s picture

Assigned: Unassigned » yched
Status: Reviewed & tested by the community » Needs review

Moving this to yched to review, last time he looked at this, was not keen. I'm also less keen than I was when I opened the issue.

radimklaska’s picture

Backported patch from #691932-44: Add hook_field_schema_alter() applied on D7 cleanly. I used it in custom module also for saving some metadata (in my case with date field). Works fine! Thanks for the patch. I think it would be nice to have hook_field_schema_alter() both in D7 and D8.

5n00py’s picture

I also apply this patch on d7 instalation.

I create my own module that implement this hook.
I create fields in db manually(in .install) and add metadata using this hook.
And now my module works great, i test all CRUD for fields and working with existing fields.

But be careful, if you create db fields manually use $field_name prefix.
And don't forget remove ALL you fields from the db on uninstall.

Just litle example about using this hook:

/**
 * Alter field schema
 */
function my_module_field_schema_alter(&$schema, $field) {
  if ($field['type'] == 'addressfield') {
    $schema['columns']['district'] = array(
      'type' => 'varchar',
      'length' => 10,
      'not null' => FALSE,
      'default' => '',
    );
    $schema['columns']['riverside'] = array(
      'type' => 'varchar',
      'length' => 10,
      'not null' => FALSE,
      'default' => '',
    );
  }
}

function my_module_addr_install() {
  _my_module_handle_fields();
}

function my_module_addr_uninstall() {
  _my_module_handle_fields('uninstall');
}

/**
  * Manually add/remove db columns for existint fields
  */
function _my_module_handle_fields($op='install') {
  $fields = field_info_fields();
  foreach ($fields as $field) {
    //My module extends addressfield schema
    if ($field['type'] == 'addressfield') {
      //Perform actions on every table including revisions tables
      foreach($field['storage']['details']['sql'] as $sql) {
        foreach($sql as $table_name=>$table_data) {
          $schema['columns'] = array();
          my_module_field_schema_alter($schema, $field);
          foreach($schema['columns'] as $field_name=>$spec) {
            $field_name = $field['field_name'] . '_' . $field_name;
            if($op == 'install') {
              db_add_field($table_name, $field_name, $spec);
            }
            if($op == 'uninstall') {
              db_drop_field($table_name, $field_name);
            }
          }
        }
      }
    }
  }
}
chanderbhushan’s picture

This is the code for hook_schema_alter
The exmple is given below:--

function My_module_schema_alter(&$schema) {
$schema['table_name']['fields']['expire_coupon_days'] = array(
'type' => 'int',
'not null' => TRUE,
'default' => 0,
);
}

/**
* Implementation of hook_install().
*/

function My_module__install() {
$ret = array();
$new_schema = array();
My_module__schema_alter($new_schema);
foreach ($new_schema['table_name']['fields'] as $name => $spec) {
db_add_field($ret, 'table_name', $name, $spec);
}
}

/**
* Implementation of hook_uninstall().
*/

function My_module__uninstall() {
$ret = array();
$new_schema = array();
My_module__schema_alter($new_schema);
foreach ($new_schema['table_name']['fields'] as $name => $spec) {
db_drop_field($ret, 'table_name', $name);
}
}

anydigital’s picture

Looks like patch from #44 works well for Drupal 7.

I'm writing a new module (Field Mixer) which requires hook_field_schema_alter(). It would be cool if this hook becomes part of Drupal core.

Barry_Fisher’s picture

Attached a backport to D7 having changed the directories to the D7 equivalents (so expecting a testbot fail- although this does work on D7 for me.) Also corrected a couple of spelling mistakes in the test- sorry to be so particular !! :)

Status: Needs review » Needs work

The last submitted patch, field_d7-backport-for-hook_field_schema_alter_691932_58.patch, failed testing.

5n00py’s picture

Any progress with this issue?

dasjo’s picture

drupalhooked’s picture

Status: Needs work » Needs review
Issue tags: -API addition, -Needs backport to D7

Status: Needs review » Needs work
Issue tags: +API addition, +Needs backport to D7

The last submitted patch, field_d7-backport-for-hook_field_schema_alter_691932_58.patch, failed testing.

wamilton’s picture

Version: 8.x-dev » 7.x-dev
Assigned: yched » Unassigned
Status: Needs work » Needs review

Just applied this to HEAD of 7.x branch, not sure what testbot's deal is.

Queuing again.

Un-assigning from yched since he hasn't been in this thread for a year.

wamilton’s picture

...and that's not how one queues it

Status: Needs review » Needs work
Issue tags: -API addition, -Needs backport to D7

The last submitted patch, field_d7-backport-for-hook_field_schema_alter_691932_64.patch, failed testing.

djdevin’s picture

Status: Needs work » Needs review
Issue tags: +API addition, +Needs backport to D7

#65: field_d7-backport-for-hook_field_schema_alter_691932_64.patch queued for re-testing.

Fatal error was the 'variable' table already existing, seems to be a testbot issue?

wamilton’s picture

Category: bug » feature
Status: Needs review » Reviewed & tested by the community

Since all I did was re-roll it and the patch isn't mine and I have it working on a production site with geocluster and I've read it and it seems fine: "looks good to me"

Setting to feature request since not having a hook can't be a bug.

droplet’s picture

+++ b/modules/field/tests/field_test_schema_alter.infoundefined
@@ -0,0 +1,6 @@
+core = 8.x

8.x in D7? It doesn't make sense to me.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

So the test code wouldn't actually be enabled.

yched’s picture

Folks, there is no such thing in D8, this can't be added in D7.
- #64 just reassigned the issue to D7 because the patch was against D7.
- I'm still very not keen on adding that in either D8 or D7, and catch said he was less keen himself...

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev

So, won't fix?

BTMash’s picture

I still stand by my comment in https://drupal.org/node/691932#comment-5373150 that there is no equivalent in core during the move from 6.x to 7.x for cck (in fact, why not just remove the ability to alter table schemas as well? I somehow suppose there is less risk associated in a module that alters non-field tables?). Anyways, this issue seems to be a lost cause.

The only real course of options that I see from this (aside from locally 'hacking core' which is supposedly a bad idea):

  • Install Field Collection and deal with the additional table/field overhead.
  • Install MultiField and deal with / help fix any potential issues since it is a relatively new module.
  • Create an alterable field storage contrib module which, codewise, looks like the field storage module plus this patch (so others can use it as well ^_~).
BTMash’s picture

I've also created #2060629: Remove hook_schema_alter() from core to go along with this issue.

attiks’s picture

I agree with #73 this is a missing piece, we use it sometimes to easily extend other modules, and by reading some of the comments others are doing something similar, so I think we need this for D7.

#71 Are there other (better) ways to accomplish this without hacking core?

BTMash’s picture

Well, after talking with a few other folks in my area that have the same issue (#74 is coming from my need to have *consistent* behavior from a system; what is there now is not it), I figured I would try and implement my own field storage module to try and tackle the issue. But unfortunately, due to weird relationship between field and field_sql_storage, field_create_field, field_update_field, and field_read_fields implement retrieving the schema for a field (ie it does not go through a field storage schema hook to sort all this stuff out). So from what I see, the only place where the schema can be altered in a contrib module is through implementation of hook_field_storage_create_field and hook_field_storage_update_field (so if schema was altered, field_read_fields still won't properly pick up an altered schema.

So my question to core maintainers who won't allow the addition of a hook that is *clearly* needed by other developers (like those coming from d6 cck or needing to increase the size of a column or even adding another index though hooks and not just running commands from CLI): what is the course of action? Is it to create a fork of fields image module to add a caption field? Is it to make every field a field collection (or multifield)? Or are we better of getting rid of stupid slogans like 'The drupal way' and "Don't hack core" and just go ahead and do so?

Sylvain Lecoy’s picture

I would love to see this patch landing in d7.

I don't want to use field collection just to add a caption on an image field. This is just seriously insane.

Sylvain Lecoy’s picture

Status: Needs work » Needs review

Patch in #65 works for me.

Sylvain Lecoy’s picture

Status: Needs review » Needs work

Btw, it would be nice to enhance the patch to actually read the field_schema_alter from the .install files; Currently it needs to be accessible in the .module or the like.

Sylvain Lecoy’s picture

At #76 there is another valid use case other than image 'caption' field, if your MySQL server for whatever reason can't afford field size bigger than 768 and in the emfield module it is set to 2048, this can help hook the schema and fix the issue locally instead of patching the module directly.

Shellingfox’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Shellingfox’s picture

Shellingfox’s picture

Status: Needs work » Needs review
Shellingfox’s picture

Version: 8.x-dev » 7.x-dev

Sorry for didnot see the status + version of this issue. I'm change it back to 7.x and make sure the test can run.

Status: Needs review » Needs work
Shellingfox’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Shellingfox’s picture

Status: Needs work » Needs review
FileSize
6.36 KB
mgifford’s picture

BTMash’s picture

Adding related issue.

jhedstrom’s picture

The patch in #89 is missing the use of the new field_retrieve_schema() in field_update_field(). This patch adds that.

Note, I am also using this patch for use with the Geocluster module. This patch would appear to be the only way to achieve what that module needs to to with regards to fields provided by other modules.

jhedstrom’s picture

FileSize
738 bytes

Oops, forgot the interdiff.

davidsickmiller’s picture

Similar in concept to the image caption use case, I believe this issue would address my use case where I have a specific site that would like to add a field for user-friendly descriptions to the Date Repeat module. Yes, I can use a field collection, but that seems to prevent me from integrating the new field into the Date module's form widget. Theoretically I could use the Conditional Fields module to have the collection's field only displayed when appropriate, but it doesn't seem sophisticated enough for rule to only display when the collection item's date field is repeating.

Looks like my current options are to (1) apply this issue's patch and use hooks, (2) patch the Date module, or (3) have a bad admin UI.

Incidentally, the Date project refactored the code for repeating dates into a separate module, Date Repeat, but because there's no hook_field_schema_alter(), they're forced to handle the schema for Date Repeat in date.install.

cilefen’s picture

Title: No hook_field_schema_alter() » Add hook_field_schema_alter()

burkeker’s picture

In my project I am trying to add an attribute to an entry of entityreference field, and the easiest way seems to be putting that attribute in the field table. Thanks to @jhedstrom for the patch, I've just tried/tested this hook and it works nicely. I hope this patch got reviewed and made it to the release soon.

5n00py’s picture

@burkeker, I have same issue, but with different field. After year of waiiting i found another solution.
I just want to say that this issue created 5 year ago and still not commited.

cilefen’s picture

@5n00py Instead of complaining, review the issue. Some issues don't get committed because of no reviews.

https://www.drupal.org/patch/review

jhedstrom’s picture

Version: 7.x-dev » 8.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +entity storage

Looks like this was never properly set back to 8.x after moved to 7.x for testing.

The field and entity APIs have changed substantially in 8.x, so this needs a pretty hefty issue summary update. It might not even be needed in 8.x, or it might be more difficult. I don't know offhand.

Since this is a feature request, bumping to 8.1.x, but if it can be reclassified as a task, and implemented in a non-disruptive way, it might still be considered for 8.0.x I think.

ohthehugemanatee’s picture

Tested the D7 patch working... makes my life a lot easier. This is a simple patch, and would be RTBC if there was an 8.x approach working.

ohthehugemanatee’s picture

Status: Needs work » Needs review

I'm assured that the ability to modify a field's schema is already fully implemented in D8 in separate issues. @tim.plunkett is that correct? If it's being handled elsewhere in D8, and we have a working D7 patch along with a bunch of use cases that aren't otherwise addressed... what is still needed to get this committed to D7?

lesonunique’s picture

Applied patch #92 on my D7 core, took only 20 lines to add a column to a date field, that is well handled during saving and displaying/filtering by Views

dinarcon’s picture

Hello,

I have applied the patch in #92 on D7 and it works fine. An example implementation can be found at #1040640: Store and display geocoded address information AND/OR address data entered by users. I have attached a new patch correcting some misspelled words. Do we still need a D8 version for this or it has already been addressed in some other way?

andypost’s picture

Status: Needs review » Closed (works as designed)

Base and config field implementations does not allow alter \Drupal\Core\Field\FieldStorageDefinitionInterface::getSchema() and there's no way to alter field properties

Feel free to reopen for reason to allow alter indexes of fields, but I agree with @yched about prevent altering.

PS: Filed #2673688: Remove remains of hook_field_schema()

jhedstrom’s picture

Status: Closed (works as designed) » Needs review
Issue tags: +Needs title update

@andypost without this, what is the recommended approach for contrib discussed above (eg geocluster) that *add* functionality to other module's fields? Should they create an entirely separate field type? If so, then what happens when multiple modules want to add functionality to a single field type? I don't think this should be closed out as those seem like legitimate issues that should be addressed in one way or another.

Status: Needs review » Needs work

The last submitted patch, 105: field-schema-alter-691932-105.patch, failed testing.

BTMash’s picture

Considering hook_schema_alter itself is now gone, hook_field_schema having a possibility is gone now as well. Maybe there will be a way to fire hooks to be extended to our extension fields.

Sylvain Lecoy’s picture

This issue promote code duplication, but it is not bad in a certain way, except for maintenance obviously :P

To add new functionality to a single field type, copy all the schema and create a new field type, then add your functionality and rename your field type accordingly. I know this is boring work but this is how I ended. I think this is a protection for modules relying on field type to have certain db fields, in case you add new ones it should not break compatibility but if you remove it can really mess up a system.

So I guess this is the general idea.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dgtlmoon’s picture

I found the comment at https://www.drupal.org/node/691932#comment-7855361 very interesting, I've been battling with field_collections and its 306~ open bugs for some time. Being able to just add a few "columns" (?) to an existing field would be a perfect solution in many situations.

In my situation, I need to add some meta-data to a media_youtube instance which will be used during the rendering/theming of the output. I have data that is important to each instance of the field, not just a general field setting per content type.

There are many other examples where this is needed.

Sounds to me like the Drupal 8 approach is fundamentally different and that this issue can only realistically apply to Drupal 7, therefor the status of needing to be solved in Drupal 8 first is perhaps incorrect? Just trying to push this one along a bit...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 105: field-schema-alter-691932-105.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

MustangGB’s picture

Is this a non-issue for D8, and only needed for D7?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

parasolx’s picture

Is this issues still valid or already closed? This hook really help if it implement in core.

jhedstrom’s picture

I think modules that take the approach of something like Geocluster still need something like this. That module is an addon to the Geofield module, which stores lat/long, etc, and then Geocluster adds its own columns to store a hash necessary for clustering points. Without a way to alter existing fields, it would have to declare its own field type, which would then make it quite difficult to add to a mapping site when such an approach is needed.

andypost’s picture

On other hand the field class is plugin and plugin implementations could be swapped with custom implemenation
\Drupal\Core\Field\FieldTypePluginManager::__construct() calls $this->alterInfo('field_info'); so this hook already here

MustangGB’s picture

@andypost So, it's only needed for D7 then =P

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

MustangGB’s picture

Version: 8.8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
6.83 KB

Rerolled #105 and moved to D7 as a D8 version is not needed.

parasolx’s picture

Apply and test path #125 on Drupal 7.67. That patch works fine. No error produces. Yes, I know it should be tested on dev version, but to make sure the latest version works fine.

AndyF’s picture

Per #79 I've tried to update the patch to allow the hook to live in .install files.

Thanks!

Gnanasampandan Velmurgan’s picture

I have reviewed and tested the patch #125 on Drupal 7.67. That patch works fine. Thanks.

MustangGB’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7, -Needs issue summary update, -Needs title update

This is still working great, including the change to support *.install added in #127.

#129 is the patch to review.

For anyone interested, my primary use-case thus far is to add a changed property for fields, similar to how we added one for users in #1835754: Add last 'changed' property to user entity.

poker10’s picture

Issue tags: +Needs framework manager review, +Pending Drupal 7 commit

I think the patch looks good in overall.

In addition to the new alter hook addition, there are two changes comparing with the current code:

1.

@@ -388,9 +404,7 @@ function field_read_fields($params = array(), $include_additional = array()) { 
     // Populate storage information.
-    module_load_install($field['module']);
-    $schema = (array) module_invoke($field['module'], 'field_schema', $field);
-    $schema += array('columns' => array(), 'indexes' => array());
+    $schema = field_retrieve_schema($field);

Similar code is on two more places, but here the $schema array does not get 'foreign keys' => array() key inicialized. After the patch is applied, the $schema array gets this key initialized, because the new function field_retrieve_schema() does this by default:

+function field_retrieve_schema($field) {
    ...
+  $schema = (array) module_invoke($field['module'], 'field_schema', $field);
+  $schema += array('columns' => array(), 'indexes' => array(), 'foreign keys' => array());
    ...
+}

I am not suggesting this is wrong or a problem, but better to ensure this specific change would not cause any side-effects.

2.

@@ -130,9 +150,7 @@ function field_create_field($field) {
   // Collect storage information.
-  module_load_install($field['module']);

Before the patch, these functions were loading only one .install file - for the field. After the patch, all .install files are loaded (for obvious reasons), so that any alter hook is found and run:

+function field_retrieve_schema($field) {
+  // Make sure the installation API is available.
+  include_once DRUPAL_ROOT . '/includes/install.inc';
+  module_load_all_includes('install');
    ...
+}

This can have a minor performance effect, but taking into account a fact, that this code is mostly run on field CRUD operations (insert, update, delete) and also in field_read_fields(), where the field definition output is cached in cache_field table most of the time, this should be very minor and not a problem at all.

Just a small nitpick:

diff --git a/modules/field/field.crud.inc b/modules/field/field.crud.inc
@@ -20,6 +20,26 @@
+/**
+ * Retrieves the schema for a field.
+ *
+ * @param array $field
+ *   The field array to get the schema definition against.
+ * @return array
+ *   The field schema definition array.
+ */

I think that the @return part in the phpdoc should be prepended by a newline.

Adding a tag for @Fabianx review.

anrikun’s picture

+1 for patch at #129

Fabianx’s picture

Approved!

I am fine with that change.

I agree with the great review, but think that the additional columns should not make a problem in practice.

  • mcdruid committed 6380331e on 7.x
    Issue #691932 by BTMash, Shellingfox, jhedstrom, dinarcon, AndyF,...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thank you everybody that contributed to this.

Very hard to get credit perfect on an issue that's almost old enough to drive a car (in some places) with well over 100 comments; apologies if I've made mistakes.

  • mcdruid committed fcceb070 on 7.x
    Issue #691932: adding the test files this time; oops
    
MustangGB’s picture

Hurray, great addition.

poker10’s picture

I have drafted a simple CR here: https://www.drupal.org/node/3406312

Status: Fixed » Closed (fixed)

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