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.

Files: 
CommentFileSizeAuthor
#89 field_d7-backport-for-hook_field_schema_alter_691932_89.patch6.36 KBShellingfox
PASSED: [[SimpleTest]]: [MySQL] 40,762 pass(es).
[ View ]
#83 field_d7-backport-for-hook_field_schema_alter_691932_83.patch7.3 KBShellingfox
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/field/field.api.php.
[ View ]
#65 field_d7-backport-for-hook_field_schema_alter_691932_64.patch6.91 KBwamilton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_d7-backport-for-hook_field_schema_alter_691932_64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#58 field_d7-backport-for-hook_field_schema_alter_691932_58.patch6.91 KBreallifedesign
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_d7-backport-for-hook_field_schema_alter_691932_58.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 691932_44.patch7 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,595 pass(es).
[ View ]
#41 691932_40.patch6.98 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,180 pass(es).
[ View ]
#39 691932_38_testonly.patch2.9 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 34,170 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#39 691932_38.patch6.6 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,183 pass(es).
[ View ]
#36 691932_36.patch6.36 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]
#32 691932_32.D7.patch3.09 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 691932_32.D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 691932_26.patch3.13 KBzhangtaihao
PASSED: [[SimpleTest]]: [MySQL] 34,078 pass(es).
[ View ]
#21 691932_21.patch2.28 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 33,945 pass(es), 0 fail(s), and 778 exception(es).
[ View ]
#16 691932_16-d7.patch2.11 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 691932_16-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 691932_16.patch2.15 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,047 pass(es).
[ View ]
#2 field_schema_alter.patch1.87 KBcatch
Passed on all environments.
[ View ]

Comments

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

Status:Active» Needs review
StatusFileSize
new1.87 KB
Passed on all environments.
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs review

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

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

subscribe

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.

"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.

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 ?'.

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.

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.

sub

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.

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

Status:Needs work» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new2.15 KB
PASSED: [[SimpleTest]]: [MySQL] 34,047 pass(es).
[ View ]
new2.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 691932_16-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

@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.

@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.

@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?

@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.

StatusFileSize
new2.28 KB
FAILED: [[SimpleTest]]: [MySQL] 33,945 pass(es), 0 fail(s), and 778 exception(es).
[ View ]

@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.

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.

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.

StatusFileSize
new3.13 KB
PASSED: [[SimpleTest]]: [MySQL] 34,078 pass(es).
[ View ]

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?

Status:Needs work» Needs review

Oops.

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?).

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.

@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.

Issue tags:-Needs tests

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

StatusFileSize
new3.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 691932_32.D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rolled a D7 version of your patch.

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.

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?

Assigned:Unassigned» BTMash

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

Assigned:BTMash» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.36 KB
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]

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

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

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 :)

StatusFileSize
new6.6 KB
PASSED: [[SimpleTest]]: [MySQL] 34,183 pass(es).
[ View ]
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] 34,170 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

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.

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.

StatusFileSize
new6.98 KB
PASSED: [[SimpleTest]]: [MySQL] 34,180 pass(es).
[ View ]

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

Status:Needs work» Needs review

  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. :)

StatusFileSize
new7 KB
PASSED: [[SimpleTest]]: [MySQL] 34,595 pass(es).
[ View ]

Hopefully, third time is the charm :)

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Needs review

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

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")

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?

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.

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.

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

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.

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.

Backported patch from #691932-44: No 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.

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);
            }
          }
        }
      }
    }
  }
}

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);
}
}

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.

StatusFileSize
new6.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_d7-backport-for-hook_field_schema_alter_691932_58.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Any progress with this issue?

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.

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.

StatusFileSize
new6.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_d7-backport-for-hook_field_schema_alter_691932_64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

...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.

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?

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.

+++ 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.

Status:Reviewed & tested by the community» Needs work

So the test code wouldn't actually be enabled.

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...

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

So, won't fix?

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 ^_~).

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

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?

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?

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.

Status:Needs work» Needs review

Patch in #65 works for me.

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.

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Issue summary:View changes
StatusFileSize
new7.3 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/field/field.api.php.
[ View ]

Status:Needs work» Needs review

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

The last submitted patch, 83: field_d7-backport-for-hook_field_schema_alter_691932_83.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 83: field_d7-backport-for-hook_field_schema_alter_691932_83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.36 KB
PASSED: [[SimpleTest]]: [MySQL] 40,762 pass(es).
[ View ]

Adding related issue.