Part of #1696224: Remove system_settings_form()
Convert different language forms to use system_config_form().
Related to #1751348: Convert locale settings to configuration system
Comment | File | Size | Author |
---|---|---|---|
#30 | drupal8.config-language-negotiation.30.patch | 14.67 KB | n3or |
#30 | interdiff.txt | 728 bytes | n3or |
#28 | drupal8.config-language-negotiation.28.patch | 14.39 KB | n3or |
#28 | interdiff.txt | 8.34 KB | n3or |
#26 | drupal8.config-language-negotiation.26.patch | 14.15 KB | n3or |
Comments
Comment #1
n3or CreditAttribution: n3or commentedConverts language_negotiation_configure_session_form().
Comment #3
sun1) You have a underscore vs period difference in the config key there ;) It should be a dot/sub-key.
2) How about we name the config object
language.negotiation
, essentially removing the top-level 'negotiation' key?Comment #4
n3or CreditAttribution: n3or commentedFixed #3.
Comment #5
sunRenamed language.settings into language.negotiation.
Comment #6
sunSimplified the locale module update -- no need to re-invent the wheel ;)
We should also convert the other language negotiation settings in this issue/patch into the same config object.
Comment #7
sunErm, ->delete()->save() ...? ;)
This can be removed entirely. ;)
Hm... the added language update duplicates the existing locale module update...?
We should keep the existing locale module update only.
Comment #8
n3or CreditAttribution: n3or commentedContains compatibility update to current core and changes of comment #7 (hopefully I did it right).
EDIT: Can't remove the following code:
config('language.negotiation')->delete('session.parameter');
(-> The tests "LocaleUninstallTest" and "LocaleUninstallFrenchTest" in module "locale" failed.)
Comment #9
xjm#8: drupal8.config-language-negotiation.8.patch queued for re-testing.
Comment #10
aspilicious CreditAttribution: aspilicious commentedthere is no update function for the variable
Comment #11
n3or CreditAttribution: n3or commentedDo you mean the code I removed because of comment #7?
Comment #12
aspilicious CreditAttribution: aspilicious commentedsun didn't asked to remove both update functions, just to delete one and keep the locale.install update function.
Comment #13
n3or CreditAttribution: n3or commentedYou're right! Thank you. Added update function in this patch again. Additionally I converted language_negotiation_configure_url_form().
The variables "language_negotiation_url_prefixes" and "language_negotiation_url_domains" are part of this transformation and their structure doesn't fit cleanly for the config system. It would be fine to find a structure optimized for language and config system.
It could be a cleaner solution to create a config object for every negotiation handler (e. g. language.negotiation.url). That means we have to iterate through all config objects if we want to create/delete a language and have to add/remove the relevant keys. But at the moment there isn't a possibility to select config objects by prefix (e. g. config_by_prefix(language.negotiation.) ). In addition there is no possibility to automate the create/delete process because there is no specification in which keys/subkeys we have to create/remove a language specific entry.
As an alternative we could create a config object for every language, but there is the same problem by creating/removing negotiation handlers. Thus there is a kind of three dimensional structure, which can't resolved cleanly by config system.
In a first step (this issue) I converted the mentioned variables (as is) into a single configuration object (language.negotiation) with default values for D8 default negotiation handlers and language. That makes it possible to fix this issue. In a follow up issue we can concentrate on finding and creating a structure which handles this dependencies better.
(I talked with sun about this problem and I hope I summarized this problem correct and understandable.)
The attached patch contains the first version for transformation of mentioned form and there are some parts missing in attached interdiff.
Comment #15
sun::delete() deletes the entire configuration object. You want ::clear($key). :)
Comment #16
n3or CreditAttribution: n3or commentedFixed #15 and a fatal error in "language upgrade test". Need now a complete recheck.
Some code I removed is necessary for a successful upgrade so I added this code again. (The language/locale upgrade path is really hard to understand!)
Comment #17
n3or CreditAttribution: n3or commentedComment #19
n3or CreditAttribution: n3or commentedShould become green..
Comment #20
n3or CreditAttribution: n3or commented#19: drupal8.config-language-negotiation.19.patch queued for re-testing.
Comment #21
Gábor HojtsyComment #22
andypostIt still neds a work for other variables and will need for variables that are introduced with #1191488: META: Integrate l10n_update functionality in core
Suppose we need to consistency in naming and probably postpone this
Comment #23
andypostFiled separate issue #1751348: Convert locale settings to configuration system to focus this on language module only
Comment #24
sunCan someone explain whether the l10n_update variables are in core already and whether we really need to incorporate them in this issue/conversion? It's a bit confusing, since this issue is about Language module settings, but l10n_update is about Locale module, I think?
It looks like we want to change the LANGUAGE_NEGOTIATION_URL_* constants from integers to strings like we did for other conversions (e.g., 'prefix', 'domain', etc).
When doing so, I think we also want a better config key than url.determination_part -- url.source might work.
Can we write the chained method calls as usual (on separate lines)?
Comment #25
Gábor HojtsyIndeed, l10n_update is in the locale module, this issue should be focused on language module I think.
Comment #26
n3or CreditAttribution: n3or commentedPatch should apply to current core and I changed chained method calls (#24).
Comment #27
Gábor HojtsyThanks for the continued work! I'm not fully up to date in terms of how config standards apply now, so consider this my best thinking only :)
Negotiation settings are not language specific, they are global to the site, so I'm not sure about that @todo?
Also, the '0' looks pretty odd here. It sounds like it would make sense to swap this to be 'prefixes' and 'domains' and change the constants to use those names too, no?
The actual prefixes and domains are not cleared out?
Otherwise looks good to me. Thanks again!
Comment #28
n3or CreditAttribution: n3or commented- Replaced url.determination_part with url.source (see #22)
- Now we use string-identifiers in url.source (see #22/#24)
- Removed unnecessary "clear" in language_uninstall (see #24)
There are some other forms related to language negotiation. But I think if we put every (language negotiation) form conversion into this patch, it will become a patch from hell. So can we first make this part rtbc?
Comment #30
n3or CreditAttribution: n3or commentedFixed failing test.
Comment #31
Gábor HojtsyLooks good to me.
Comment #32
webchickCommitted and pushed to 8.x. Thanks!
Comment #33
sun@n3or and I discussed this patch to some lengths, and we definitely intended to keep the scope very limited, and I'm glad those parts landed now.
We also considered to simply re-open and keep this issue in order to perform the additional conversions, but in hindsight, that might not be a good idea, since that always presents a confusing situation for reviewers and other contributors.
Thus, @n3or, or whoever else wants to tackle the further language negotiation variable conversions: Let's make sure to create an issue for those and link to it from here. Thanks! :)
Good job!
Comment #35
ianthomas_ukAs suggested in #33, I have opened a followup: #2102477: Convert remainder of language negotiation settings to configuration system
Comment #35.0
ianthomas_ukUpdated issue summary.