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.
Comments
Comment #1
chx CreditAttribution: chx commentedUse case: you want to know when you tagged an entity, ie add a timestamp to a taxonomy_term_reference field.
Comment #2
catchHere'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.
Comment #3
chx CreditAttribution: chx commentedWell, given #1 I am RTBC'ing this.
Comment #4
yched CreditAttribution: yched commentedI'm sorry but I need to review this :-).
Comment #5
yched CreditAttribution: yched commentedStarted 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
Comment #6
Pasquallesubscribe
Comment #7
catchyched 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.
Comment #8
yched CreditAttribution: yched commented"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.
Comment #9
yched CreditAttribution: yched commentedAnd 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 ?'.
Comment #10
catchI 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.
Comment #11
catchAlright 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.
Comment #12
catch#872146: Impossible to alter field_schema was duplicate.
Comment #13
heshanlksub
Comment #14
droplet CreditAttribution: droplet commentedcan 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.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedSubscribe :(
I wish this gets backported but thats unlikely i guess
Comment #16
BTMash CreditAttribution: BTMash commentedSo 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:
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.
Comment #17
zhangtaihao CreditAttribution: zhangtaihao commented@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.
Comment #18
BTMash CreditAttribution: BTMash commented@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 (withhook_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.Comment #19
zhangtaihao CreditAttribution: zhangtaihao commented@yched:
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 invokeshook_field_schema()
anddrupal_alter()
's it?Comment #20
zhangtaihao CreditAttribution: zhangtaihao commented@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.
Comment #21
BTMash CreditAttribution: BTMash commented@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.Comment #23
BTMash CreditAttribution: BTMash commented#21: 691932_21.patch queued for re-testing.
Comment #25
zhangtaihao CreditAttribution: zhangtaihao commentedI 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 bothdrupal_get_schema()
anddrupal_get_complete_schema()
(which is called by the former) public functions.Anyway, it is curious the kinds of errors this patch generates.
Comment #26
zhangtaihao CreditAttribution: zhangtaihao commentedI 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?
Comment #27
zhangtaihao CreditAttribution: zhangtaihao commentedOops.
Comment #28
BTMash CreditAttribution: BTMash commentedI 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?).
Comment #29
zhangtaihao CreditAttribution: zhangtaihao commentedI 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.
Comment #30
BTMash CreditAttribution: BTMash commented@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.
Comment #31
zhangtaihao CreditAttribution: zhangtaihao commentedAgreed. Perhaps one of us can work on the D7 backport.
Comment #32
BTMash CreditAttribution: BTMash commentedRolled a D7 version of your patch.
Comment #33
xjmThis patch looks simple and straightforward. Small point about the API documentation:
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#hooksIt 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.
Comment #34
xjmOh, also, let's get
@see
between the hook and its function or otherwise explain how it's invoked. Edit: and probably tofield_create_field()
too?Comment #35
BTMash CreditAttribution: BTMash commentedAssigning it to self so I remember to work on your recommended changes.
Comment #36
BTMash CreditAttribution: BTMash commentedOk, here is a new stab at your recommended changes and a test case.
Comment #37
zhangtaihao CreditAttribution: zhangtaihao commentedI understand it may be commonplace, but does relying on an image field for a test make core trickier to maintain?
Comment #38
BTMash CreditAttribution: BTMash commentedGood 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 :)
Comment #39
BTMash CreditAttribution: BTMash commentedHere 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.
Comment #40
xjmAlright, 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:
I think these two arrays have an extra two spaces of indentation for the keys.
Let's add @see hook_field_schema_alter() at the end of the field_create_field() docblock.
Need a docblock for this guy.
This should be "Tests."
Let's fix those, and then I think this is RTBC.
Comment #41
BTMash CreditAttribution: BTMash commentedAlright, this hopefully takes care of the formatting issues :)
Comment #42
xjmComment #43
xjmThis should be one line (and again the third-person verb). How about:
With the detailed docblock for the class above, maybe for this one we could just do:
I think that's a little more clear.
I'd reroll with these changes myself, but then I couldn't RTBC it. :)
Comment #44
BTMash CreditAttribution: BTMash commentedHopefully, third time is the charm :)
Comment #45
xjmThanks!
Comment #46
yched CreditAttribution: yched commentedI need to take a closer look at this. Will try to do so asap.
Comment #47
yched CreditAttribution: yched commentedAlso, 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")
Comment #48
zhangtaihao CreditAttribution: zhangtaihao commentedI 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?
Comment #49
yched CreditAttribution: yched commentedCCK'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.
Comment #50
BTMash CreditAttribution: BTMash commentedJust 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.
Comment #51
BTMash CreditAttribution: BTMash commented#44: 691932_44.patch queued for re-testing.
Comment #52
mlncn CreditAttribution: mlncn commentedRunning 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.
Comment #53
catchMoving 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.
Comment #54
radimklaskaBackported 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.
Comment #55
5n00py CreditAttribution: 5n00py commentedI 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:
Comment #56
chanderbhushan CreditAttribution: chanderbhushan commentedThis 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);
}
}
Comment #57
anydigital CreditAttribution: anydigital commentedLooks 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.
Comment #58
Barry_Fisher CreditAttribution: Barry_Fisher commentedAttached 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 !! :)
Comment #60
5n00py CreditAttribution: 5n00py commentedAny progress with this issue?
Comment #61
dasjoi've had success using the D7 patch from #44 so far:
http://drupalcode.org/project/geocluster.git/blob/0ab147c4fcd0d9156c00ca...
http://drupalcode.org/project/geocluster.git/blob/0ab147c4fcd0d9156c00ca...
Comment #62
drupalhooked CreditAttribution: drupalhooked commented#58: field_d7-backport-for-hook_field_schema_alter_691932_58.patch queued for re-testing.
Comment #64
wamilton CreditAttribution: wamilton commentedJust 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.
Comment #65
wamilton CreditAttribution: wamilton commented...and that's not how one queues it
Comment #67
djdevin#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?
Comment #68
wamilton CreditAttribution: wamilton commentedSince 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.
Comment #69
droplet CreditAttribution: droplet commented8.x in D7? It doesn't make sense to me.
Comment #70
tim.plunkettSo the test code wouldn't actually be enabled.
Comment #71
yched CreditAttribution: yched commentedFolks, 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...
Comment #72
tim.plunkettSo, won't fix?
Comment #73
BTMash CreditAttribution: BTMash commentedI 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):
Comment #74
BTMash CreditAttribution: BTMash commentedI've also created #2060629: Remove hook_schema_alter() from core to go along with this issue.
Comment #75
attiks CreditAttribution: attiks commentedI 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?
Comment #76
BTMash CreditAttribution: BTMash commentedWell, 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
, andfield_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 ofhook_field_storage_create_field
andhook_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?
Comment #77
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI 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.
Comment #78
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedPatch in #65 works for me.
Comment #79
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedBtw, 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.
Comment #80
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedAt #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.
Comment #81
Shellingfox CreditAttribution: Shellingfox commented65: field_d7-backport-for-hook_field_schema_alter_691932_64.patch queued for re-testing.
Comment #83
Shellingfox CreditAttribution: Shellingfox commentedComment #84
Shellingfox CreditAttribution: Shellingfox commentedComment #85
Shellingfox CreditAttribution: Shellingfox commentedSorry for didnot see the status + version of this issue. I'm change it back to 7.x and make sure the test can run.
Comment #87
Shellingfox CreditAttribution: Shellingfox commented83: field_d7-backport-for-hook_field_schema_alter_691932_83.patch queued for re-testing.
Comment #89
Shellingfox CreditAttribution: Shellingfox commentedComment #90
mgifford89: field_d7-backport-for-hook_field_schema_alter_691932_89.patch queued for re-testing.
Comment #91
BTMash CreditAttribution: BTMash commentedAdding related issue.
Comment #92
jhedstromThe patch in #89 is missing the use of the new
field_retrieve_schema()
infield_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.
Comment #93
jhedstromOops, forgot the interdiff.
Comment #95
davidsickmiller CreditAttribution: davidsickmiller commentedSimilar 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.
Comment #96
cilefen CreditAttribution: cilefen commentedComment #98
burkeker CreditAttribution: burkeker commentedIn 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.
Comment #99
5n00py CreditAttribution: 5n00py commented@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.
Comment #100
cilefen CreditAttribution: cilefen commented@5n00py Instead of complaining, review the issue. Some issues don't get committed because of no reviews.
https://www.drupal.org/patch/review
Comment #101
jhedstromLooks 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.
Comment #102
ohthehugemanatee CreditAttribution: ohthehugemanatee at Forum One commentedTested 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.
Comment #103
ohthehugemanatee CreditAttribution: ohthehugemanatee at Forum One commentedI'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?
Comment #104
lesonunique CreditAttribution: lesonunique commentedApplied 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
Comment #105
dinarcon CreditAttribution: dinarcon at Agaric commentedHello,
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?
Comment #106
andypostBase and config field implementations does not allow alter
\Drupal\Core\Field\FieldStorageDefinitionInterface::getSchema()
and there's no way to alter field propertiesFeel 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()
Comment #107
jhedstrom@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.
Comment #109
BTMash CreditAttribution: BTMash as a volunteer commentedConsidering 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.
Comment #110
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThis 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.
Comment #112
dgtlmoon CreditAttribution: dgtlmoon commentedI 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...
Comment #116
MustangGB CreditAttribution: MustangGB commentedIs this a non-issue for D8, and only needed for D7?
Comment #120
parasolx CreditAttribution: parasolx as a volunteer and at Hadafi Solution & Resources commentedIs this issues still valid or already closed? This hook really help if it implement in core.
Comment #121
jhedstromI 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.
Comment #122
andypostOn 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 hereComment #123
MustangGB CreditAttribution: MustangGB commented@andypost So, it's only needed for D7 then =P
Comment #125
MustangGB CreditAttribution: MustangGB commentedRerolled #105 and moved to D7 as a D8 version is not needed.
Comment #126
parasolx CreditAttribution: parasolx as a volunteer and at Hadafi Solution & Resources commentedApply 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.
Comment #127
AndyF CreditAttribution: AndyF at Fabb commentedPer #79 I've tried to update the patch to allow the hook to live in .install files.
Thanks!
Comment #128
Gnanasampandan Velmurgan CreditAttribution: Gnanasampandan Velmurgan as a volunteer commentedI have reviewed and tested the patch #125 on Drupal 7.67. That patch works fine. Thanks.
Comment #129
brad.bulger CreditAttribution: brad.bulger commentedReroll for 7.95
Comment #130
MustangGB CreditAttribution: MustangGB commentedThis 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.Comment #131
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI 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.
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 functionfield_retrieve_schema()
does this by default:I am not suggesting this is wrong or a problem, but better to ensure this specific change would not cause any side-effects.
2.
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: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 incache_field
table most of the time, this should be very minor and not a problem at all.Just a small nitpick:
I think that the
@return
part in the phpdoc should be prepended by a newline.Adding a tag for @Fabianx review.
Comment #132
anrikun CreditAttribution: anrikun commented+1 for patch at #129
Comment #133
Fabianx CreditAttribution: Fabianx at HeroDevs commentedApproved!
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.
Comment #135
mcdruidThank 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.
Comment #137
MustangGB CreditAttribution: MustangGB commentedHurray, great addition.
Comment #138
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI have drafted a simple CR here: https://www.drupal.org/node/3406312