Problem/Motivation
Field API stores some information about bundles (settings for extra-fields, list of view modes using dedicated display settings rather than 'default' settings) in the {variables} table (ew), in a single entry spanning all bundles (double ew).
While using the {variables} table is a sad idea to begin with, changing that is d8 material.
The immediate issue is that the "one single variable for all bundles" approach prevents clean exportability of those settings using features. That's a very stupid idea, the blame is all mine (yched speaking), I don't know what I was thinking back then...
Proposed resolution
Split the current monolithic 'field_bundle_settings' variable into separate ''field_bundle_settings_[entity_type]__[bundle]' variables.
See #13, #14, #17 about the 'double underscore' separator.
Remaining tasks
None. (+ this has been committed to d8 already)
User interface changes
None
API changes
Transparent, the settings stored in there are supposed to be accessed through the field_bundle_settings().
Original report by DamienMcKenna
Currently you cannot work with a distributed development team who are each working on different content types and properly export all display settings (possibly other things too) without a lot of hand-holding and conflict resolution. The reason is that this single variable stores certain settings for all content types that is not stored elsewhere, e.g. it stores a list of the view modes that are being used but the individual fields & their corresponding settings are stored elsewhere. Ultimately this variable should be a) moved to a cache table, b) have each setting moved to another per-entity or per-bundle data structure.
Comment | File | Size | Author |
---|---|---|---|
#53 | drupal-1211008-53.patch | 6.77 KB | tim.plunkett |
#51 | drupal-1211008-51-without-update-hook.patch | 5.9 KB | tim.plunkett |
#51 | drupal-1211008-51.patch | 6.77 KB | tim.plunkett |
#46 | drupal-1211008-46.patch | 6.51 KB | tim.plunkett |
#44 | drupal-1211008-44.patch | 5.65 KB | tim.plunkett |
Comments
Comment #1
Simon Georges CreditAttribution: Simon Georges commentedSubscribe.
Comment #2
jwilson3I agree its up to core to set the precedent for per-entity settings stored in unique variables. This will help push adoption of the technique for contrib modules that have the same problem, eg. Content_Access, #871770-3: Content Access support for Features exportables (the content_access issue may have been fixed in d7 already, not sure).
Comment #3
marcingy CreditAttribution: marcingy commentedThings are dealt with in head first - this sounds very much like a feature request to me given that it is a fundamental change, however marking as a task. Also marking as needs backport to d7 but depending on the scope of the api changes it might not be possible
Comment #4
yched CreditAttribution: yched commentedFully agreed, unfortunately, and the blame is all mine:-( - I'd even bump this to 'major'.
The all-inclusive field_bundle_settings variable is plain awful, and a real drag for features-exportability of the corresponding settings (order and visibility of non-field elements like node title, view mode settings).
Comment #5
yched CreditAttribution: yched commentedPatches for d7 (with an upgrade func) and for d8.
Comment #6
yched CreditAttribution: yched commentedAlso, I hope the Content Management initiative will let us move those out of the variables table completely.
At that point, there will be an interesting discussion as to how the config files for fields, instances, bundles (node types...) would be best organized, and the input from Features folks will be highly welcome.
Comment #7
catch+function field_update_7000() {
This should be in a 7.x-extra @defgroup.
This looks fine to me otherwise.
Comment #8
yched CreditAttribution: yched commentedRight.
Reuploaded both patches (the d8 patch is the same than #5)
Comment #9
Simon Georges CreditAttribution: Simon Georges commentedI can confirm the update function
field_update_7002()
is working.Everything seems to be running smoothly after using the patch on a Drupal 7.7 installation.
Comment #10
neurojavi CreditAttribution: neurojavi commentedSubscribing
Comment #11
joverSubscribe
Comment #12
DamienMcKennaShouldn't this also have a hook_uninstall() to delete the variables?
Comment #13
DamienMcKennaFirst off, thanks for working on this yched.
I was wondering, is it a good idea to use a colon in a variable name? Or should a simple underline be used as the separator instead, as with all other per-entity or per-content-type variables, e.g. 'menu_parent_' . $node->type? I've updated the patches from #8 so they use this naming convention instead.
Comment #14
yched CreditAttribution: yched commentedProblem with underscores is that ENTITYTYPE_BUNDLENAME can be ambiguous and cause clashes if either part contains an underscore.
Colons are the standard separator for cache ids, maybe less so for variable names, but they are a good fit.
Comment #15
DamienMcKennaI'd expect the likelihood of finding an {$entity_type}_{$bundle} that overlapped with another {$entity_type}_{$bundle} to be so unlikely it would approach the impossible. I recommend going for the existing convention of using underlines rather than creating a new convention.
Comment #16
DamienMcKennaOne slight improvement, though - it might be better to delete the old variable before adding the new one, i.e. instead of:
it should be:
I've updated my patches from #13 accordingly.
Comment #17
yched CreditAttribution: yched commentedI don't want to sound stubborn, but I still stand by my patches in #8 :
- entity types and bundle names live freely in contrib, and I would not be so confident as to deem clashes too improbable to happen. Or should we use double underscores like theme variants do ?
- I couldn't say I care strongly about this, but I don't get the point of #16. As a general practice and if there's no real reason not to, when saving a copy of a thing, I'd tend to save the new copy of a thing before trashing the old one.
Comment #18
xjmTagging.
Comment #19
attiks CreditAttribution: attiks commentedsub
Comment #20
bforchhammer CreditAttribution: bforchhammer commentedSubscribing
Comment #21
Simon Georges CreditAttribution: Simon Georges commentedAny chance to have that into next D7 release ?
Comment #22
yched CreditAttribution: yched commentedRe-uploading the patches from #8, only using '__' (double underscore) instead of ':' to separate entity type and bundle name in the var names (see comments #13-#17)
Comment #23
StevenPatzLooks like this is a D8 task.
Comment #24
yched CreditAttribution: yched commented@spatz4000 : This would need to be backported to D7 (#22 has a D7 patch), but has to be committed in D8 first.
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedLooks fine to me.
Comment #26
chx CreditAttribution: chx commentedBackporting to D7 is a question... this is technically an API break.
Comment #27
xjmTagging per #26.
Comment #28
yched CreditAttribution: yched commented@chx : the data in there is supposed to be accessed by the field_bundle_settings() wrapper func
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commentedRight. the whole point of the field_bundle_settings() is to let u make internal changes freely. Other APIs are just variable_get() but this is not one of them. Anyway, let's focus on D8 first.
Comment #30
catchI've committed this to 8.x, IMO it looks fine for 7.x too but that's up to webchick of course.
On a side note it's a shame field.attach.inc doesn't use the field_bundle_settings() function for crud but that is already in there.
Comment #31
FreddieK CreditAttribution: FreddieK commentedIt would be nice to see an update on the status of this for D7. This is a major flaw when working in distributed teams.
Comment #32
yched CreditAttribution: yched commentedThe patch for D7 was in #22 (applies with fuzz)
Rerolled, re-uploaded, let's get the bot to test this.
Then it needs someone to set it to RTBC.
[edit : added an issue summary for webchick]
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedthanks yched.
Comment #34
webchickHm. This looks slightly terrifying. :)
Now that we can test the 7.0 -> 7.x upgrade path thanks to #1182296: Add tests for 7.0->7.x upgrade path (random test failures), I'd like to see a test here that demonstrates that the upgrade path is harmless to existing sites. If that passes, then we can backport to D7.
Comment #35
yched CreditAttribution: yched commentedAw. Really, a test ?
The update func is quite harmless, really. Takes the content of a variable, iterates on the entries, creates a separate variable for each one, removes the old variable...
Comment #37
casey CreditAttribution: casey commented#32: field_bundle_settings-1211008-32.patch queued for re-testing.
Comment #38
casey CreditAttribution: casey commentedJust did a search for the variable in a project containing most most used modules. The Media module is directly accessing the variable in media_update_7016(). That *could* be a problem.
This is quite useful for features and stuff though. I'd really like this to be committed to D7 too.
Comment #39
DamienMcKenna@casey: then Media needs a follow-up task to fix that variable usage.
Comment #40
wojtha CreditAttribution: wojtha commentedFollow-up issue in Strongarm: #1306924: Export additionnal settings
Comment #41
Dave ReidHuh? media_update_7016() requires being able to inspect and modify multiple entity types and bundles so currently this patch would be a no-go for Media unless someone can provide a patch that uses the proper APIs.
Comment #42
DamienMcKenna@davereid: As mentioned above, this needs a follow-up task for Media, rather than Media blocking a core fix.
Comment #43
DamienMcKennaA follow-on ticket for media: #1418708: Update media_update_7016 to new field_bundle_settings variable
Comment #44
tim.plunkettOkay, here are update path tests. The first thing I could think of that was stored in there was field_extra_fields, so I used that from poll module.
Comment #45
tim.plunkettEr, those were named backwards, but you get the point :)
Comment #46
tim.plunkettJust cleaning up some s/upgrade/update and trailing whitespace, no actual changes.
Comment #47
yched CreditAttribution: yched commentedGee, I let that drop off my radar. Thx a lot for the test, tim !
On a minor note, it would be more starightforward if the test read the result of field_bundle_settings() rather than field_extra_fields_get_display().
Comment #48
tim.plunkettBut that's too obvious ;)
I wrote it from the point of actually using it. I can reroll it, but it just means a bigger array to mock in the test, and more serialized data to put in the fake DB. I left out the view modes on purpose because more could be added at any time.
Mark CNW and I'll reroll.
Comment #49
yched CreditAttribution: yched commentedWell, the test is about checking the upgrade that changes the storage of field_bundle_settings() so basing it on the result of that function rather than a derivative seems natural.
I feel like a painful pedant, but yes, CNW IMO :-).
Comment #50
yched CreditAttribution: yched commentedComment #51
tim.plunkettFair enough.
Comment #52
yched CreditAttribution: yched commentedThks !
Original patch got in, D7 addition is just the test, so I feel I can RTBC this
Comment #53
tim.plunkettReuploading, something is wrong with that file.
Comment #54
nevergone CreditAttribution: nevergone commentedAnd next step? :)
Comment #55
ZenDoodles CreditAttribution: ZenDoodles commentedReviewed patch. Looks good, applies cleanly. Test works, and code solves the problem well IMO.
Thanks @timplunkett!
Comment #56
danielnolde CreditAttribution: danielnolde commented#D7 backport:
Splitting the field_bundle_settings out per bundle would be awesome to have even in D7
This would make working with bundles in features so much more convenient (or possible ;), more so when writing encapsuled components as features, e.g. for install profiles (where you cannot do the workaround to just throw the whole field_bundle_settings variable in some base feature).
HOWEVER, caution is in order, considering that there are Drupal 7 sites with existing features built with the single field_bundle_settings variable:
@ webchick / @ all: Having this in D7 would be awesome, but we should really consider to either state a clear warning in the release notes or some kind of coordinates effort with the strongarm/features maintainers to ensure that a change in variable use for this does not break _existing_ features which had been exported using the single field_bundle_settings variable!
Comment #57
tim.plunkett@danielnolde, because the variable is being split and then deleted, any feature that manually included field_bundle_settings with strongarm would be marked overridden. There is nothing that could be done from within features or strongarm itself to help this along.
Also, there is a strongarm issue to automatically add the new split up variable to appropriate features that will be committed after this: #1306924: Export additionnal settings
Comment #58
webchickOof. This patch gives me the absolute heebie jeebies to introduce in a stable release, TBH. :( The upgrade path tests look solid, and looks like there's a plan for updating the known affected contrib modules, so that's all great stuff. I still have a feeling we're really going to hear about this one, though. :\ OTOH I can certainly understand why this would cause people trying to work with exportables to stab things.
All things considered, I decided to commit and push this to 7.x. This is definitely something to cover in the release notes. Should we add a change notice as well?
Comment #60
xjmSorry thresholds. I agree with webchick that this should include a change notice, despite that the change is transparent.
Comment #61
tim.plunkettHere's a first attempt at a change notice: http://drupal.org/node/1569594
Comment #62
BerdirLooks good to me, just unsure about the "exporting" in the last sentence. Maybe we can generalize that a bit (any kind of access to that variable is now broken), Display Suite for example seems to be messing with that variable as well:
Not sure how to exactly formulate it though, maybe change "exporting" to "accessing" and change Strongarm part a bit so that it makes sense again?
Comment #63
tim.plunkettI've opened the following bug reports:
Comment #64
tim.plunkettAlso updated the change notice with Berdir's suggestion.
Comment #65
BerdirChange notice looks good to me.
Comment #66
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #67.0
(not verified) CreditAttribution: commentedadded summary
Comment #68
xjmComment #69
xjmComment #69.0
xjmUpdated the proposed resolution with the actual one.