Problem/Motivation
Same as #2355909: language.settings config is not scalable, needs that for third party settings storage, so it has a place to store our settings.
Postponed on #2355909: language.settings config is not scalable
Proposed resolution
Use third party settings on language settings to store content translation settings.
Remaining tasks
Do it.
User interface changes
None.
API changes
content_translation_set_config(), content_translation_get_config(), content_translation_get_config_key() would be replaced/removed. May be kept for BC.
Comment | File | Size | Author |
---|---|---|---|
#30 | 2363155-content-translations-30.patch | 24.16 KB | penyaskito |
#25 | 2363155-content-translations-25.patch | 24.12 KB | penyaskito |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyComment #3
tstoeckler:-)
Comment #4
YesCT CreditAttribution: YesCT commentedComment #5
YesCT CreditAttribution: YesCT commentedComment #6
catch#2355909: language.settings config is not scalable is in.
Comment #7
penyaskitoUse ContentLanguageSettings third party settings storage. Kept the functions for now, not sure if we want to leave them, deprecate them or remove them.
Comment #8
penyaskitoPS: IMHO we should remove them before it is too late.
Comment #10
andypost+1 to deprecate procedural wrappers
PS: There's actually a few failures
Comment #11
penyaskitoI didn't know that you need to provide a schema for third_party settings, but makes lots of sense.
Comment #12
penyaskitoFound this @todo:
Proposal:
Remove:
Add:
Comment #14
penyaskitoThat was not my fault... We are supposed to be using existing entities.
Comment #15
penyaskitoRemoving:
Adding:
Comment #16
Gábor HojtsyLooks generally good except:
Should have an empty line before this one.
Also the naming of this schema element is *confusing*. Should it be language.content_settings.*? I know this should have been said in #2355909: language.settings config is not scalable but not too late here either :)
Making top level keys like that is confusing because there may be a content_settings module in contrib which would have this namespace. Let's keep core schema elements in the core namespace.
I think its confusing to have enable/disable and setEnabled all, sounds confusing.
Extra s :)
Comment #17
plachNice :)
Can we retrieve the content manager just once and save it in a
$manager
variable? That would improve readability.I agree with Gabor, having the three methods is confusing. I'd keep only the setter.
Comment #18
yched CreditAttribution: yched commented[edit : never mind, @Gabor was quicker than me]
Comment #19
penyaskitoAdressing #16, #17, #18.
Comment #20
Gábor HojtsySorry I did not find this one before:
false should be FALSE in PHP in Drupal's code style.
Looks good otherwise.
Comment #21
penyaskitoFixed.
Comment #22
Gábor HojtsyAmazing, looks all good to me :)
Comment #23
plachAwesome work!
RTBC +1
Comment #24
alexpottThis looks fantastic. One very small thing.
Let's not use the static functions here. We should add a protected method to load everything from the already injected entity manager.
Comment #25
penyaskitoFixed #24.
Comment #26
Gábor HojtsyHum, I am not sure this is an improvement? I mean loadByEntityBundle nicely abstracted this fallback / decision logic and now it is here again :/
Comment #27
alexpott@Gábor Hojtsy those static loaders should not be used in injected classes - they obscure dependencies and prevent simple unit testing.
Comment #28
alexpottThis does not return $this - it returns the config entity.
Comment #29
andypostThe related issue should be in-sync with this one
Comment #30
penyaskitoGood catch, fixed the return type doc.
Comment #31
Gábor HojtsyNice, thanks! I think this resolves all concerns. Thanks @alexpott for pointing out that the local nice-ness would actually be problematic for injectability. Make sense.
Comment #32
alexpottWe should have a d8 to d8 CR record for this to help existing multilingual sites. That CR should reference https://www.drupal.org/node/2382481
Comment #33
Gábor HojtsyChange notice draft at https://www.drupal.org/node/2391205
Comment #34
plachLooks great
Comment #35
plachComment #36
alexpottCommitted 4efd487 and pushed to 8.0.x. Thanks!
Comment #38
Gábor HojtsyThanks again @penyaskito!
Comment #39
Gábor HojtsyThanks again @penyaskito!