Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This will convert field_language_default and storage_default to field.settings.
Comment | File | Size | Author |
---|---|---|---|
#42 | 1942346-42.patch | 16.06 KB | swentel |
#38 | 1942346-38.patch | 16.93 KB | swentel |
#38 | interdiff.txt | 2.41 KB | swentel |
#34 | 1942346-34.patch | 16.25 KB | swentel |
#32 | 1942346-32.patch | 16.25 KB | swentel |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #2
swentel CreditAttribution: swentel commentedTagging with D8MI as well
Comment #3
swentel CreditAttribution: swentel commentedMissed the 'extra_weights' example in field.api.php (although they are not used in actual code though) and moved language_default to state.
Comment #4
swentel CreditAttribution: swentel commentedBetter title
Comment #6
swentel CreditAttribution: swentel commentedMixing up API's and and epic typo
Comment #8
swentel CreditAttribution: swentel commentedAdded upgrade path and should fix most of the tests.
Comment #10
BerdirLooks great. Might be possible to remove some variable tables from those DUBT tests?
Comment #11
swentel CreditAttribution: swentel commentedcould be, I'll look into it, but will be for tomorrow. Other two tests need the field config as well, I'm wondering whether we should just simply install field config always by default in the base unit test ?
Comment #12
yched CreditAttribution: yched commented- in EditTestBase and FieldUnitTestBase: Looks like this should be replaced by $this->installConfig(array('field)), and var $default_storage can go away as a class member
- in FieldTestBase: this seems unneeded to begin with, since that is a WebTest, the config will be applied as part of installing field.module ?
The comment doesn't apply anymore now that this is in state ?
Also, the existence of that comment (encouraging site admins to control the value of the field_language_fallback variable) makes me question whether this should really be in state() :-/
The current code in HEAD overwrites that value on every language enable / disable, but maybe setting $conf['field_language_fallback'] in settings.php was a feature.
We probably want to ping @plach about that.
Yeah, so this variable is now long gone, it doesn't seem a good idea to keep a misleading reference to it in sample code, makes you think field module has such a config value.
We should replace the sample code in hook_field_attach_rename_bundle() / hook_field_attach_delete_bundle() with an imaginary use case. Something like :
Also, sigh at field_attach_(create|rename|delete)_bundle() and associated hooks still being part of Field API...
We really need to move forward on #1187784: Standardize adding a new bundle to an entity type.
Comment #13
yched CreditAttribution: yched commentedHm, I guess we'd now need to add the new config value to the config schema that got added in #1919164: Create configuration schemas for field module...
+ Minor: looking at the setting name now, I guess 'default_storage' would make more more sense than 'storage_default' ?
Comment #14
yched CreditAttribution: yched commentedRe-title. A bit less accurate, but easier to find :-)
Also, while #1735118: Convert Field API to CMI is not the place to start adding installConfig('field') in all DUTB tests that enable field.module, I guess this issue is ? :-/
Comment #15
plachYep, in D7 ET exposes a UI to set the value of that variable. We might want to do that in D8 too, hence I definitely convert it to a configuration setting rather than storing in the state. It's totally something we may want to migrate across environments.
Comment #16
swentel CreditAttribution: swentel commentedThis should be better.
Comment #17
swentel CreditAttribution: swentel commentedBah forgot the config schema - give me a few moments.
Comment #18
swentel CreditAttribution: swentel commentedAnd the schema - also moved the settings in alphabetical order in the file.
Comment #20
swentel CreditAttribution: swentel commentedMissed on test.
Comment #21
yched CreditAttribution: yched commentedThanks for the feedback @plach.
What worries me a bit is to have language enable / disable resulting in changing the value of another setting in field.settings. Config is config, state is state. Such coupling & side effects sound like a recipe for messy deployments.
What I understand from #15 is :
- we want a field.settings.language_fallback config entry; It's just configuration, aka "does the site admin want field language fallback to ever trigger when applicalble". Doesn't ever change unless the admin changes it.
- at runtime, language fallback is triggered or not based on the condition :
(field.settings.language_fallback == TRUE) && (there are more than two languages on the site).
i.e
(a condition on the config defined by the admin) && (a condition on the current runtime state)
Whether the second condition is read directly from language config or precomputed and placed in state(), I don't know for sure.
But IMO we shouldn't denormalize those two conditions in a single value written back in config. Config should be normalized, non redundant, non coupled.
@plach, thoughts ?
Comment #22
plachTotally agreed, I missed that change. I think what we want is a new API function, something like:
Then we can remove those two
hook_language_*
implementations.Comment #23
yched CreditAttribution: yched commentedSounds fine.
In this scenario, though, I guess the upgrade path should always set the new field.settings.language_fallback value to TRUE, then.
The sites that will want this to FALSE are the D7 sites that were using settings.php to override the value computed from the state of enabled languages, and they will have to update this piece anyway.
Comment #24
plachYep, sounds reasonable.
Comment #25
swentel CreditAttribution: swentel commented@yched - do we also want to kill field_bundle_settings variable with this one ?
Comment #26
yched CreditAttribution: yched commentedHmm - field_bundle_settings still holds what would go in EntityFormDisplays for extra fields, right ?
Then maybe let's not kill it right now ?
Comment #27
swentel CreditAttribution: swentel commentedPartly - field api cmi will kill the 'display' part, entityform display the 'form' part, but we're still left with the 'view modes' entry, i.e which one has custom settings or not. Maybe we should create a dedicated issue for that, because it might be more complex conversion, depending on how we approach it.
Comment #28
yched CreditAttribution: yched commentedSeparate issue ++
Comment #29
swentel CreditAttribution: swentel commenteddone - #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable
Comment #30
swentel CreditAttribution: swentel commentedTagging
Comment #31
swentel CreditAttribution: swentel commentedrerolling now field api cmi is in
Comment #32
swentel CreditAttribution: swentel commentedNew patch with the new API function, is that ok ?
Comment #34
swentel CreditAttribution: swentel commentedUgh, update_8003 -> update_8004
Comment #36
yched CreditAttribution: yched commentedLooks good - well, aside from the test fail :-)
Don't remember exactly, but shouldn't this be true now ?
Maybe rather "... is enabled."?
Comment #37
plachI guess so, in D7 it is.
Comment #38
swentel CreditAttribution: swentel commentedShould be good now.
Comment #39
yched CreditAttribution: yched commentedLooks good to me :-).
Temptative RTBC if it comes back green.
Comment #40
swentel CreditAttribution: swentel commentedif this one goes in first, #1966998: Add a constant OR use TRUE/FALSE for active storage vs. inactive storage and other properties needs a re-roll.
Comment #41
xjmIt took me a few reads to realize that these are out-of-scope whitespace cleanups.
Does this need an upgrade path test?
For the reason these are being removed, see #21, #22, and #23. Does this behavior have test coverage to ensure it works the same as it did previously? Also, I don't actually see where the setting ever actually gets set now?
Comment #42
swentel CreditAttribution: swentel commentedRe-roll after #1966998: Add a constant OR use TRUE/FALSE for active storage vs. inactive storage and other properties got in (trailing spaces in field.api.php were fixed there).
I *think* we have tests for this behavior yes, but I was hoping that plach could confirm this.
Comment #43
catchAgreed with the helper for dealing with the language_multilingual case and this is straightforward otherwise.
Committed/pushed to 8.x.
Comment #44.0
(not verified) CreditAttribution: commentedUpdated issue summary.