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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Simon Georges’s picture

Subscribe.

jwilson3’s picture

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

marcingy’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +Needs backport to D7

Things 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

yched’s picture

Component: node system » field system
Priority: Normal » Major

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

yched’s picture

Status: Active » Needs review
FileSize
2.62 KB
3.41 KB

Patches for d7 (with an upgrade func) and for d8.

yched’s picture

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

catch’s picture

Status: Needs review » Needs work

+function field_update_7000() {

This should be in a 7.x-extra @defgroup.

This looks fine to me otherwise.

yched’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
2.62 KB

Right.
Reuploaded both patches (the d8 patch is the same than #5)

Simon Georges’s picture

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

neurojavi’s picture

Subscribing

jover’s picture

Subscribe

DamienMcKenna’s picture

Shouldn't this also have a hook_uninstall() to delete the variables?

DamienMcKenna’s picture

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

yched’s picture

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

DamienMcKenna’s picture

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

DamienMcKenna’s picture

One slight improvement, though - it might be better to delete the old variable before adding the new one, i.e. instead of:

+  variable_set('field_bundle_settings:' . $entity_type . ':' . $bundle_new, $settings);
+  variable_del('field_bundle_settings:' . $entity_type . ':' . $bundle_old);

it should be:

+  variable_del('field_bundle_settings:' . $entity_type . ':' . $bundle_old);
+  variable_set('field_bundle_settings:' . $entity_type . ':' . $bundle_new, $settings);

I've updated my patches from #13 accordingly.

yched’s picture

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

xjm’s picture

Tagging.

attiks’s picture

sub

bforchhammer’s picture

Subscribing

Simon Georges’s picture

Any chance to have that into next D7 release ?

yched’s picture

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

StevenPatz’s picture

Looks like this is a D8 task.

yched’s picture

@spatz4000 : This would need to be backported to D7 (#22 has a D7 patch), but has to be committed in D8 first.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine to me.

chx’s picture

Backporting to D7 is a question... this is technically an API break.

xjm’s picture

Issue tags: +API change

Tagging per #26.

yched’s picture

@chx : the data in there is supposed to be accessed by the field_bundle_settings() wrapper func

moshe weitzman’s picture

Issue tags: -API change

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

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

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

FreddieK’s picture

It would be nice to see an update on the status of this for D7. This is a major flaw when working in distributed teams.

yched’s picture

The 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]

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

thanks yched.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

yched’s picture

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

casey’s picture

Status: Needs work » Needs review
casey’s picture

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

DamienMcKenna’s picture

@casey: then Media needs a follow-up task to fix that variable usage.

wojtha’s picture

Follow-up issue in Strongarm: #1306924: Export additionnal settings

Dave Reid’s picture

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

DamienMcKenna’s picture

@davereid: As mentioned above, this needs a follow-up task for Media, rather than Media blocking a core fix.

DamienMcKenna’s picture

tim.plunkett’s picture

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

tim.plunkett’s picture

Er, those were named backwards, but you get the point :)

tim.plunkett’s picture

FileSize
6.51 KB

Just cleaning up some s/upgrade/update and trailing whitespace, no actual changes.

yched’s picture

Gee, 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().

tim.plunkett’s picture

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

yched’s picture

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

yched’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.77 KB
5.9 KB

Fair enough.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thks !
Original patch got in, D7 addition is just the test, so I feel I can RTBC this

tim.plunkett’s picture

FileSize
6.77 KB

Reuploading, something is wrong with that file.

nevergone’s picture

And next step? :)

ZenDoodles’s picture

Reviewed patch. Looks good, applies cleanly. Test works, and code solves the problem well IMO.

Thanks @timplunkett!

danielnolde’s picture

#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!

tim.plunkett’s picture

@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

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.13 release notes

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

Status: Fixed » Closed (fixed)

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

xjm’s picture

Title: Split field_bundle_settings out per bundle » Change notification for: Split field_bundle_settings out per bundle
Priority: Major » Critical
Status: Closed (fixed) » Active
Issue tags: +Needs change record

Sorry thresholds. I agree with webchick that this should include a change notice, despite that the change is transparent.

tim.plunkett’s picture

Status: Active » Needs review

Here's a first attempt at a change notice: http://drupal.org/node/1569594

Berdir’s picture

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

$ grep -R field_bundle_settings *
modules/ds_extras/ds_extras.panels.inc:  $bundle_settings = field_bundle_settings($entity_type, $bundle);
modules/ds_extras/ds_extras.panels.inc:  field_bundle_settings($entity_type, $bundle, $bundle_settings);
modules/ds_extras/ds_extras.vd.inc:  $bundle_settings = variable_get('field_bundle_settings', array());
modules/ds_extras/ds_extras.vd.inc:  variable_set('field_bundle_settings', $bundle_settings);

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?

tim.plunkett’s picture

Also updated the change notice with Berdir's suggestion.

Berdir’s picture

Status: Needs review » Fixed

Change notice looks good to me.

Tor Arne Thune’s picture

Title: Change notification for: Split field_bundle_settings out per bundle » Split field_bundle_settings out per bundle
Priority: Critical » Major

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

added summary

xjm’s picture

Issue tags: -Needs change record
xjm’s picture

xjm’s picture

Issue summary: View changes

Updated the proposed resolution with the actual one.