Problem/Motivation
There is this great language selection configuration feature on content types that we recently improved greatly:
However, with language supported on vocabularies, taxonomy terms, files and other entities, we need a way to configure this kind of thing on other entities. Taxonomy terms or vocabularies do not have bundles, but it would make tons of sense to have this kind of setting on vocabularies for their children terms.
Before of the vocab edit:
Proposed resolution
The main complication in this is how the values are stored and applied. Likely we'll need to keep some custom wrapper / accessor code for node types and vocabularies and reuse the UI from one place.
Remaining tasks
Remaining: Verify the issue summary is correct in terms of api changes, and update issue summary saying if documentation needs to be written
Done: code style looks good
Done: have gotten a few code reviews in #25 by fago and #50 by planch, and #102 (plus some early looks by webflo, Berdir and LoMo). Items from those reviews were addresses. And a final look by Gabor in #126
Done: ui review. in #33 has a UI RTBC from Bojhan
User interface changes
Currently the ui with patch looks like:
In the content type edit, there is an improvement to add (TheLanguage) which shows what the current site default language is in the drop down select and also in the summary in the vertical tab.
and:
In the vocab edit (for example), it reuses the generalized ui, and has a label the entity sets.
(from #127)
API changes
(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)
Postponed on this issue
#1810386: Create workflow to setup multilingual for entity types, bundles and fields is postponed on this issue.
Comment | File | Size | Author |
---|---|---|---|
#162 | drupal-language_config-1739928-162.patch | 5.28 KB | plach |
#136 | language_configuration-1739928-136.patch | 53.74 KB | vasi1186 |
#136 | interdiff_135_136.txt | 805 bytes | vasi1186 |
#135 | language_configuration-1739928-135.patch | 53.74 KB | vasi1186 |
#135 | interdiff_133_135.txt | 787 bytes | vasi1186 |
Comments
Comment #1
vasi1186 CreditAttribution: vasi1186 commentedI have one idea of how we can implement this. We can provide a new form element type: "language_setting" (or something like that) that will have these two attributes (among others maybe):
- #entity_type: specifies for what entity type is the setting. This should be mandatory.
- #bundle: optional, specifies for what bundle is the setting (if the entity type has bundles).
We also have to add somehow an additional submit handler in the form (I think this can be done in the process callback of the element). On the submit handler, we will just save the values (we should be able to get the entity type and the bundle, so we can generate unique variable names using them).
The last thing would be to actually create some function that would act as a wrapper to get a specific language setting value for an entity type and bundle.
And of course, then we will have to just use this new form element on the forms where we need to add language settings, and use those settings when we print the language_select form element on the entity add/edit form.
Comment #2
vasi1186 CreditAttribution: vasi1186 commentedAnd for some reason I don't know why I don't have the language system option in the Component select field... that's why I had to change the Component.
Comment #3
Gábor HojtsyJust using the bundle is not going to be enough since we want to have this setting per vocabulary for terms. Vocabularies are not bundles. Not sure a form element is good for this, but I see the appeal. We can start with that and rework it later if need be.
Comment #4
vasi1186 CreditAttribution: vasi1186 commentedYes, that's true, vocabularies are not bundles, but for them we could use the machine name of the vocabulary, instead of the bundle. Basically, the form API would not impose any restriction of what values will the entity_type and bundle properties contain. Maybe it is not such a good idea to name them like this in that case. The main goal I think it is to find something that can uniquely identity a set of entities (and the (entity_type, bundle) combination is a good example).
Comment #5
Gábor HojtsyNaming things is hard. Bundle is an easy choice, it might become misleading when not used for bundle names, but we can use this name for now, yeah :)
Comment #6
vasi1186 CreditAttribution: vasi1186 commentedComment #7
vasi1186 CreditAttribution: vasi1186 commentedComment #8
vasi1186 CreditAttribution: vasi1186 commentedAttached a first patch to be reviewed. It defines a new element type to configure language on entity types. Things that have to be implemented further:
- use this form element on the node bundle configuration form.
- use this form element on the other entity cofiguration forms (like user, vocabularies, etc...)
- write more tests.
But, most important, a feedback about the approach to solve this issue would be very useful.
Comment #9
BerdirThe "the" on the first line is to much: "..., the on the..."
I'm wondering if this could become a problem.
In case we have multiple submit buttons, we often use submit-level submit callbacks, which will skip the global ones, that could become a nasty problem.
There is only #element_validate and no #element_submit, unfortunately.
Not sure how to handle that, maybe we can just document that if you don't use global submit handlers, then you need to call the submit callback yourself in your submit function?
Is there a reason why we can't split this up into two arguments? $entity_type is everywhere else a string , it's quite confusing that it's an aray with type and bundle in this case.
I think we shouldn't add new code that relies on variable_*() functions, this should use config() instead. Can be very similar otherwise I think, it's just the config($name) that is dynamic, with the fixed actual setting names.
One thing that is also missing is the handling of deleted bundles, I think there is a hook for that. You should implement that and delete the corresponding config.
I'm not sure what the long-term bundle plans are, I know that e.g. node type will become a configurable thingy and that is still heavily discussed, not sure if there will be any standardization. I'm for example wondering if it makes sense to add this to the global entity-bundle configuration file/thingy or if it should be separate. You might want to talk with @timplunkett, @sun or someone else that is working on the configurable thingy stuff.
Comment #10
webflo CreditAttribution: webflo commentedA empty string as default value? I think a boolean makes more sense.
This can´t work. $entity_types is undefined. Its $entity_type.
Comment #11
vasi1186 CreditAttribution: vasi1186 commentedYes, because there is no #element_submit I had to add the submit callback on the form itself. What do you think the impact would be to introduce an #element_submit handler in the form API. Was there a special reason why, when the #element_validate was introduced, the #element_submit was not?
There was not actually a particular reason for that, just thought that it would be better to have the parameters more compact. Probably the name of the parameter should be different and not $entity_type. Also, it could happen that the number of the parameters can grow (although in this particular case, it is more probable that it will not happen).
For the variable_*() function, it is true, I will just replace them.
Comment #12
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that solves the issues in #11, and some of the issues from #9:
- replaces the variable_*() calls with config() calls.
- removes the additional "the" in a comment.
Still pending:
- the submit handler and the parameter issues from #9.
Comment #13
BerdirNot sure about #element_submit, the problem is that #element_validate is relatively easy to implement as we already need to go through the form recursively anyway for the validation, but we'd need to do that again to execute the submit handlers once the validation is done and successful. Alternatively keep a reference to them somewhere globally and then loop through that. Would definitely be a separate issue to add support for that.
About the parameter functions, I think that language_save($entity_type, $bundle, $values) is much easier to understand instead of trying to bring it into a single variable. And as said before, it's possible that there will be some sort of standardization and there will be some sort of object representation for a bundle, that would make things easier.
Comment #14
vasi1186 CreditAttribution: vasi1186 commentedReplaced the $entity_type array() with $entity_type and $bundle strings.
Comment #16
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that should pass the tests.
Comment #17
vasi1186 CreditAttribution: vasi1186 commentedComment #18
LoMo CreditAttribution: LoMo commentedI'm working on reviewing this patch and have started by just giving it a quick read-through. It applies cleanly and looks good so far. I'd only point out a couple minor typos:
1) "configation" should be "configuration" (
$form['#language_configation_submit_added']
).2) "Returns the root name…"
Now I'll continue to work on testing functionality. :-)
Comment #19
webflo CreditAttribution: webflo commentedFixed the typos.
Comment #20
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that adds the UI for the default language configuration of taxonomy terms. This can be checked by visiting the edit page of a vocabulary. Also, uses the new form element for the node type edit page.
Comment #21
YesCT CreditAttribution: YesCT commentedThe nice ui is already in place on content types.
This could make it reusable on vocabularies, files, and other entities.
In core, we have vocabularies.
When this works, someone could implement re-using it on files and other entities in contrib (or if files gets into core, etc.)
So for now, in the ui, I'll look at:
all the places the patch will be changing things in the ui
content type edit
/#overlay=admin/structure/types/manage/article
controls defaults when making a node like
#overlay=node/add/article
edit vocabulary
/#overlay=admin/structure/taxonomy/tags/edit
controls defaults when make a term
/#overlay=admin/structure/taxonomy/tags/add
Comment #22
vasi1186 CreditAttribution: vasi1186 commentedA new version that actually applies the settings from the edit vocabulary page into the term add/edit form.
Comment #24
YesCT CreditAttribution: YesCT commentedwhen I save a content type language change I get a red error message
Notice: Undefined index: node_type_language_default in translation_node_type_language_translation_enabled_validate() (line 139 of core/modules/translation/translation.module).
Comment #25
fagolanguage_configuration_element_process() would be a little better self-speaking function name I think.
We should not do abbreviations like this. Use the full wording.
I don't think we want to have a drupal_alter() for that, it's something that just the caller should be able to take care of. Thus I'd suggest having an #options parameters that gets populated by the element info defaults. Then have a function for getting them so it's easier to override: e.g.
#options => array(
'my-stuff' => 'value',
) + language_configuration_element_default_options(),
Personally, I'd prefer not having that kind of magic and just add in your submit handler manually. That makes it more obvious how this works.
save configuration about what? I think we should come up with a more-specific name here. Also, I don't know how we plan to handle saving that. I guess it should all go via the configurable save's routine independently whether it's stored to multiple cmi files or not. But I guess we could go with that for now and revisit it once the bundle objects become converted to configurables.
There should not be a new line between the @param lines.
We use isset() instead of is_null()-
Why does this use module_invoke()?
Comment #26
YesCT CreditAttribution: YesCT commentedlooked at all the places the patch will be changing things in the ui #21
Before patch Vocab edit page
Before patch Term add page
testing.
from clean drupal install
drush -y en translation language locale
apply patch from #22
enable new language, french
/#overlay=admin/config/regional/language
edit content type and uncheck hide language selector
look at node add or edit
these are still the same as before the patch
ok
edit vocab and save
a little weird to see:
Language selector
and then in a field set, another Language selector
The first is the language of the vocab.
The second one, a field set grouped together of a language select and the hide checkbox is to set the defaults for the terms.
This is correct and ok, but was just a bit startling when I noticed two languages.
ok
look at term add
a bit wierd because since I know languages choices are possible I sort of feel like I need to know what language I should be writing in. But Schnitzel explained to me, that this is ok for now. There is another effort to redo the node edit page conpletely, and it might have an option to have something on the side like: "Information about this node. Language: French (forced)." And if that is good, could have a config option to show that on a term edit. And there are a lot of cases where the person adding or editing a term does not need to see the forced language value.
ok
edit vocab and uncheck hide language selector, look at term add
ok
Side note: if there was a contrib module that allowed permissions by role (say) to restrict the languages a user could select, and the hide was unchecked, then it would be a bit weird that they would see a select with only one choice.
Comment #27
YesCT CreditAttribution: YesCT commentedGabor pointed out that #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) is related to my note about restricting what languages show in the language selector.
Comment #28
Bojhan CreditAttribution: Bojhan commentedAlright so a short review:
Fieldset label, its a bit weird to duplicate label information with the field just below it. So lets just rename this to "Language" - unless this is collapsable.
Site default language, needs additional information what that language is; "Site default language (English)".
The label "Hide language selector" is overly technical, the term selector is too technical and should be selection. In addition to that we need to add a description, where this selection is done.
Comment #29
YesCT CreditAttribution: YesCT commentedBojhan suggest in person for " In addition to that we need to add a description, where this selection is done."
To say:
... on creating and editing.
How about instead of
Hide language selector
or
Hide language selection
use:
Hide language selecting.
Comment #30
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that should solve the issues in #25 and implement the suggestions from #28.
@fago: the problem is that the language module is not a required core module, so using any direct call to a function in the language module, without this module being installed, will generate a fatal error. To avoid this, I used the module_invoke() solution. The other one would be to wrap that code inside a module_exists() check (which I did in this patch). If you have some other approach on how to solve this, just let me know.
Comment #32
YesCT CreditAttribution: YesCT commentedpatch from #30
incorporates Bohjan feedback from #28
content type edit
content type edit with dropdown
vocabulary edit
vocabulary edit with drop down
content piece (node) add
content piece (node) add with drop down
Comment #33
Bojhan CreditAttribution: Bojhan commentedLooks good to me! RTBC from a UX perspective.
Comment #34
attiks CreditAttribution: attiks commentedComment #35
YesCT CreditAttribution: YesCT commentedNeeds fix for problem in #34 and #24
Comment #36
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that should fix #24 and hopefully #34. Also, makes some changes in some tests to use the new form element on the node type edit pages.
Comment #38
YesCT CreditAttribution: YesCT commentedMy testing steps in #26 were in the wrong order. Should be:
apply most recent patch
from clean drupal install
drush -y en translation language locale
enable new language, french
/#overlay=admin/config/regional/language
Comment #39
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that tries to solve the #36.
Comment #40
vasi1186 CreditAttribution: vasi1186 commentedComment #41
vasi1186 CreditAttribution: vasi1186 commentedThis patch probably needs some more Tests. Should there be a follow-up issue for that, or should I implement them in this patch (which is starting to get pretty big...)
Comment #42
Gábor Hojtsy@vasi1186: The node type functionality should have test coverage already. Similar coverage for terms would be good to ensure that this works across. I'm not sure we can viably argue it being committed without unfortunately :/
Comment #43
vasi1186 CreditAttribution: vasi1186 commentedOk, I'll do that then.
Comment #44
vasi1186 CreditAttribution: vasi1186 commentedComment #45
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that contains also some tests.
Comment #46
vasi1186 CreditAttribution: vasi1186 commentedComment #48
vasi1186 CreditAttribution: vasi1186 commentedLet's see if the test bot returns green now.
Comment #49
vasi1186 CreditAttribution: vasi1186 commentedComment #50
plachThis looks pretty solid, great work!
Here is my code review, I didn't read the full thread, so sorry if any of the following issues have already been discussed. Didn't test the patch yet.
This code looks a bit confusing at first sight: we may want to introduce a new $option variable instead of initializing the '#options' key and unsetting it short afterwards. Something like:
This information should rather go into the form state. This use of the form array to store data is deprecated. According to the best practices we should introduce a form state key named after the module the code belongs to and nest all the needed info there. Maybe
$form_state['language']['entity_type']
and the same for bundles?Missing "the" before values. Can we slightly reword it?
"An array holding the values to be saved having the following keys:"
If we are dealing with a language code, the key name should be
langcode
.The bundle parameter is not optional, the text in parenthesis is a bit confusing: is the bundle required or not? (looking at the code I'd say it is)
Wouldn't it make sense to store everything under the main language settings, using the
language.
prefix?language.default_configuration.$entity_type.$bundle
We should use the
drupal_container()
here. I guess this should solve also the @todo in the tests below.Not sure whether we actually need a hook here: perhaps it would be enough to leverage plugins to have custom default providers here and just go with the selected one. Can totally be a follow-up, however, but if we go this way I'd just skip the hook for now.
Please remove the trailing ellipsis, we don't use it in comments.
Unneeded empty line, should be removed.
The first sentence is a bit convolute, what about something like: "Since the machine name is not known yet, and it can be changed anytime, we also have to ..."
Comment #51
vasi1186 CreditAttribution: vasi1186 commented@plach: thank you very much for the review, I will try to solve them as soon as possible, but I think I will be able to do this only on Sunday evening, I will be pretty much offline until then.
Comment #52
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that should solve the issues in #50, except this one:
Is there any example in core where drupal_container() is used instead of global $user? I still see lots of places in drupal core where "global $user" is used.
Comment #54
vasi1186 CreditAttribution: vasi1186 commentedLet's see if this patch will pass the tests.
Comment #55
vasi1186 CreditAttribution: vasi1186 commentedComment #56
YesCT CreditAttribution: YesCT commentedPatch in #54 looks good afaik with regards to coding style
Applies to clean 8.x:
something like...
git branch 8.x
git clean -f -x
git clean -f -d
rm -rf some stuff
git checkout the stuff
git reset --hard
git pull --rebase
git branch generalize-lui-1739928
git checkout generalize-lui-1739928
curl -O http://drupal.org/files/language_configuration_1739928_54.patch
git apply language_configuration_1739928_54.patch
(install d8... probably a drush command to do that)
drush -y en translation language locale
drush cc all
add a language at /#overlay=admin/config/regional/language
edit article language settings:
some weirdness with text:
an extra space near commas (might not be a result of this patch)
and missing "current interface language" (or site default language, or whatever that default value is) when compared to the screen shot in the issue summary (http://drupal.org/files/LanguageSettings.png)
Vocabulary edit still looks good:
Term and node edit still looks good.
I compared it to a clean install without the patch...
505 git checkout 8.x
506 git branch -D generalize-lui-1739928
507 git reset --hard
508 git pull --rebase
509 git status
510 git rm -r --cached
511 git clean -f -x
512 git clean -f -d
513 sudo rm -rf sites/default
514 git checkout -- sites/default
515 git status
516 history
517 git pull --rebase
(install it)
518 drush -y en translation language locale
519 drush cc all
(add a language)
and the weird space before (some of the) commas is still there, so not this patch's problem
But the missing default value looks like it's a result of this patch.
Comment #57
YesCT CreditAttribution: YesCT commentedin irc vasi helped me find the place in the js that was responsible for the default setting showing.
rerolled. shows the setting again.
Comment #57.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #58
Gábor HojtsyDiscussed on IRC that @vasi1186 would look into the user preferred language testing and eliminate the todo from the test.
Comment #59
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that also tests the user preferred language.
Comment #60
YesCT CreditAttribution: YesCT commentedWe need a different key name than config?
Maybe we should create the issue now, and mark it postponed on this one (this issue is https://drupal.org/node/1739928). So, I dont think we need this @todo in the patch.
Comment #61
YesCT CreditAttribution: YesCT commentedAlso needs someone to look in detail to do a code review.
Comment #61.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #62
Gábor Hojtsy@YesCT/@vasi1186: indeed, the second @todo should have an issue opened and the commented code removed from the patch. We don't put @todo's like that in the code usually, we open and track followup issues instead :)
Comment #63
TransitionKojo CreditAttribution: TransitionKojo commentedFollow-up issue created at #1775522: Create mechanism to add custom default values for the Language Config element
Comment #64
TransitionKojo CreditAttribution: TransitionKojo commentedI applied the patch from #59, then removed the commented code as suggested in #62. That code was the source of the new follow-up issue #1775522: Create mechanism to add custom default values for the Language Config element
Comment #65
TransitionKojo CreditAttribution: TransitionKojo commentedComment #66
YesCT CreditAttribution: YesCT commentedthis patch addresses the last todo:
+ // @todo: #cofig is maybe not a very good key name...
#config was part of an array that contained the entity type and the bundle information. According to #1 and #4 and verified with vasi in irc, the purpose was to be the information to uniquely identify the entity. #config is replaced with #entity_information
Comment #67
plachThis looked almost RTBC to me so I just fixed the couple of issues below:
Moved this up to improve readbility.
These should be 'langcode'.
This should be $langcode.
Setting to RTBC since I just performed minor code clean-ups.
Comment #69
vasi1186 CreditAttribution: vasi1186 commentedA new test class seems to use old variables, I will re-roll the patch.
Comment #70
vasi1186 CreditAttribution: vasi1186 commentedLet's see now the test bot.
Comment #71
vasi1186 CreditAttribution: vasi1186 commentedtests passed, so I think it can get back to RTBC
Comment #72
plachYes, thanks
Comment #73
catchThis makes me uncomfortable. We're dynamically storing information about entity types and bundles in CMI here, which is the first place that's going to happen. But at least bundles are likely to become ConfigEntity instances at some point (whether a generic Bundle class or node types. etc. individually).
So, if I want to export a node type, I have no way to know that there is a language.settings config object which is storing information about them somewhere. This is an issue that hasn't been resolved in CMI yet, so it doesn't block this patch going in, but I'd really, really like a @todo pointing to a concrete issue that will try to deal with this so we can revisit it later.
And this hunk shows the problem. Node type information is currently split between the {node_type} table, random variables (or config), and sometimes custom tables in contrib modules. Are we going to move more stuff out of the {node_type} table and into separate config silos? Or try to do something else entirely?
I'm assigning to heyrocker for an opinion and hopefully he'll know where this discussion is located.
Rest of the patch looks OK to me so leaving RTBC.
Comment #74
Gábor HojtsyUnless node types and vocabularies are converted to be pluggable in some way for config storage, this is not really possible to store there. I think that can happen independently and we can integrate the two seamlessly after feature freeze. However, applying this feature to vocabularies might be something we would not be able to do later, given the generalisation that we need to do here. So it does not sound like we should postpone this entirely on the introduction of extensibility on vocabularies and node types.
Comment #75
catchYes that's why I said this:
Comment #76
BerdirI think the idea (one idea?) is to build this on top of property API once available. Basically, a module could define additional properties for the node type ConfigEntity and because the save() implementation of the config controller would rely on those properties, they would be saved together with the default configuration.
This would also mean that those additional properties would be documented so that you can check what configuration exists.
I don't think there is an issue for this already.
Comment #77
gddThere is not an issue for this specifically, but it touches on a lot of other issues that have similar problems where we discuss pieces of config that extend other pieces of config. This has come up a lot in a variety of places in the Views work I know. I've asked swentel to chime in here since he knows the most specifically about what future plans are for bundles and nodes in CMI, and I need to read up on this in more detail personally.
Comment #78
webchickI can't tell if this is "needs work" for the @todo asked for in #73/75, or if it's "needs review" pending swentel's feedback, or both. I reviewed the patch and found some things (mostly nit-picks, lots of missing docs, a couple of things that probably could use investigation/explaining), so settling on "needs work."
s it just me or are those flipped?
if it's locked, the put it in t() so it can be translated?
else print it directly?
Yay for API functions and calling them in form submit handlers, rather than making form submit handlers "API functions"! ;)
(tiny nitpick) There's an extra space before $values. Can be fixed on commit unless someone gets to it in the next re-roll.
Since these values are more or less coming in directly from $form_state['values'], should there be some sanitization here to ensure people aren't putting nonsense like "../../../../" in there?
(minor nitpick) Extra newline after "The language code."
if we still "do" not have a default value, just return the value stored in the configuration";" it has to be an actual langcode.
Needs PHPDoc.
(minor nit-pick) Comments should be prefaced by a newline for consistency.
In taxonomy module, this was wrapped in an if (module_exists('language')) check. Why is it not here?
(the line below this) Two "the"s.
Why is $form['langcode'] not also wrapped in an if (module_exists('language')) check? Does it apply if that module is not enabled?
either "buttons have" or "button has".. I think it's the latter.
13 days to next Drupal core point release.
Comment #79
vasi1186 CreditAttribution: vasi1186 commentedThe language_select form element works also without the language module being installed, just that in that case it acts like a simple value field, it does not appear in the form. Some doc for this new element: http://drupal.org/node/1749954
Comment #80
YesCT CreditAttribution: YesCT commentedto reroll #70 because of directory location changes
addressing the review is yet to come.
Comment #81
LoMo CreditAttribution: LoMo commentedRe #78
While we are on nitpicks, shouldn't the "the" at the beginning of the sentence also start with a capital 'T'?
i.e.
// The submit button has custom submit handlers.
Comment #82
YesCT CreditAttribution: YesCT commentedThis patch fixes some formatting. It puts @todo's into the code to track some items from webchicks comments that need to be addressed. Also, the PHPDoc comments that were added, might need to be reworded to be more specific.
I'm not sure about always having a blank line before // comments. Is that really a consistant style?
Comment #83
YesCT CreditAttribution: YesCT commentedThere are still things left to be fixed. But between my confusion with re-rolling it and getting it to reapply and just being tired, I'm posting this as what was done so far, and noting things still left to be done. Feel free to pick this up.
I think the blank should be there between the param and return.
Similar, I think the blank line should be there between the param and return.
Noting the followup issue with regards to catch, heyrocker, Berdir and swentel.
Need a blank line?
( http://drupal.org/node/1354 )
blank needed?
trailing white space.
And I'm not sure if the reason for not wrapping here is the same as in the other location.
check description for accuracy.
check description for accuracy.
Comment #84
YesCT CreditAttribution: YesCT commentedLet's see about the tests too.
Comment #86
YesCT CreditAttribution: YesCT commentedCrud. I was afraid of that.
Comment #87
YesCT CreditAttribution: YesCT commentedThis is in the most recent pull:
I had to change that by hand to the following to get the patch to apply. I need to think more about if the change by hand was the right one, or have someone look at that for me.
Regarding the capital T in comment #81: it's ok when the surrounding lines can be seen:
I fixed the blank lines, and the trailing whitespace.
I did an interdiff on the recent patches and also one to the last one that applied and passed the tests: 70.
Comment #88
YesCT CreditAttribution: YesCT commentedquestion needs looking at still.
question needs looking at still.
question needs looking at still.
Comment #89
YesCT CreditAttribution: YesCT commentedHere is the follow up #1783346: Determine how to identify and deploy related sets of configuration for heyrocker, catch, Berdir and swentel.
Comment #91
YesCT CreditAttribution: YesCT commentedI think I sorted out the merge better.
Comment #93
vasi1186 CreditAttribution: vasi1186 commented#91: language_configuration-1739928-91.patch queued for re-testing.
Comment #95
vasi1186 CreditAttribution: vasi1186 commented@YesCT : is the patch working on your local? On my local the test comes green...
Comment #96
YesCT CreditAttribution: YesCT commented@vasi186 I tested it briefly manually to verify the ui still looked good, and it was ok. I did not "run the tests" as I'm not sure how to do that.
Comment #97
swentel CreditAttribution: swentel commentedI'm totally fine that language configuration is saved into its separate file. It should not ever be inserted into the bundle file once we get there as I don't think that yml file should be dumping ground for anyone.
However, regarding discoverability for export purposes, maybe the naming of the configuration file could/should be changed.
The naming scheme in the fields CMI patch at this point is field.field.field_name and field.instance.entity_type.bundle.field_name. This way, you at least have a hint which instances belong to which entity type and bundle. Fields don't need that hint as it's not tied to a bundle or entity type at all.
If we'd apply this to this patch, you'd get language.default_configuration.entity_type.bundle
Yes, you will have more config files, but you have (already a visual one if you look into the config folder) a clue that this configuration is tied to an entity_type and bundle. Also, think of a the features use case, I then can create, say a feature news with the following configuration files:
- node.news.yml
- field.field.body.yml
- field.instance.node.news.body.yml
- language.default_configuration.node.news.yml
We could even change any cmi naming to start with entity.bundle in case *any* kind of config belongs to that namespace, so we'd get something like
entity_type.bundle.language.default_configuration
entity_type.bundle.field.instance.field_name
entity_type.bundle.node_type (which is kind of weird as the node_type is also in the bundle name of course, but it's just to get an idea).
The problem here is, at least as far as I can see, that we don't have mechanism to force this kind of naming schemes, it will probably be a convention, but who knows what contrib might do. So in the end, if I would change something here, then it is more config files.
Maybe heyrocker, alexpott or sun have more/better ideas here ?
Comment #98
LoMo CreditAttribution: LoMo commentedRe: #96.
Hi YesCT,
This is pretty easy:
/admin/modules
" and enable "Testing"/admin/config/development/testing
* This patch affects so many modules it could take a while to be sure you've selected all the right tests; so it might be better to let ALL the core tests run (that would take a while, but you could take a nap or something ;-) )
Comment #99
vasi1186 CreditAttribution: vasi1186 commentedOr you can just run the test that fails.
Comment #100
gddI agree with swentel in #97 that we should provide a separate file and not have modules inserting configuration into files owned by other modules.
As far as naming, I really prefer the convention that starts with entity_type.bundle. I think it is going to end up being more useful in the end and I believe it properly represents the information hierarchy that is in play with the data. It allows us to get everything associated with a bundle by querying for entity_type.bundle.* which is really convenient. It will be easier to clean up everything when a bundle gets deleted (although if a field type gets deleted then instances are still a pain in the ass to find, but I am guessing that is the rare use case, and in the end something is always going to be harder to find.)
So from the example above we would have
Actually we might be able to do node.news.instance.*.yml? Not sure if there is a use case for instances other than fields.
Discussion at #1776076-100: Convert comment module configuration to CMI is covering similar ground.
Comment #101
YesCT CreditAttribution: YesCT commentedRE #95, 98, 99
@LoMo Thanks, that was way easy!
@vasi It's green on my local.
Comment #102
swentel CreditAttribution: swentel commentedAlso, this patch doesn't take into account when a bundle is renamed. e.g. rename page content type to page_2. When that happens the bundle name needs to be updated (or the configuration file renamed in case this patch switch to individual configuration files - which it should imo)
hook_node_type_update() should be implemented then.
Comment #103
plach@swentel:
Does this mean the every module has to handle a rename? Isn't there a way to provide some magic anywhere?
Comment #104
swentel CreditAttribution: swentel commentedThere's no magic so far, also because CMI has no clue that where configuration for a bundle is stored. I'm not even sure whether that's a task for CMI and if so can be easily fixed anyway.
See http://api.drupal.org/api/drupal/modules%21field%21field.attach.inc/func... which is called by node_type_update to notify fields.
Comment #105
LoMo CreditAttribution: LoMo commentedIt's possible that if you guys (YesCT / Vasi) are both getting green, while there are fails on the testing system, that that could mean there is something which isn't properly configured on the testing server. That's been the case in other tests I've seen (for client projects with Jenkins continuous integration) that were failing on the testing server but running "green" locally.
Comment #106
YesCT CreditAttribution: YesCT commented@LoMo and @vasi1186 xjm and swentel and jthorson pointed out to me in irc that its a core random bug #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting() And that we should retest it till it doesn't randomly fail. :)
Comment #107
YesCT CreditAttribution: YesCT commented#91: language_configuration-1739928-91.patch queued for re-testing.
Comment #108
Gábor HojtsyCame back green, but still needs to take care of the bundle rename problem.
Comment #109
LoMo CreditAttribution: LoMo commentedLooks like it might be fixed now, but still putting back in for a re-test to help ensure that the "random failure" issues are resolved.
#91: language_configuration-1739928-91.patch queued for re-testing.
Comment #109.0
LoMo CreditAttribution: LoMo commenteddeleted the mention about the $user testing for user preferred language, #59 addresses that test.
Comment #110
Gábor Hojtsy@vasi1186: can you work on the bundle rename problem? Or anybody else? Certainly not assigned to heyrocker anymore as far as I can see.
Comment #111
vasi1186 CreditAttribution: vasi1186 commented@Gabor: yes, I will check this tonight.
Comment #112
vasi1186 CreditAttribution: vasi1186 commentedComment #113
vasi1186 CreditAttribution: vasi1186 commentedComment #114
Gábor HojtsyAnybody can drag this through the finish line? Its close :/
Comment #115
vasi1186 CreditAttribution: vasi1186 commentedHi,
really sorry for my late, had a lot to do lately...
I attached the patch with the hook_node_type_update() implemented and some tests. Let's see if the tests come green.
Comment #117
YesCT CreditAttribution: YesCT commented#115: language_configuration-1739928-115.patch queued for re-testing.
Comment #119
YesCT CreditAttribution: YesCT commentedtrying to reroll #115. I wasn't sure how to get an interdiff on a reroll. so I just did a diff on the two patches.
Comment #121
Gábor HojtsyThe failure is on LanguageUpgradePathTest.php line 80, looking for a language selector.
Comment #122
vasi1186 CreditAttribution: vasi1186 commentedyes, seems that somehow the upgrade from drupal 7 to 8 has some issues regarding if the language selector should be hidden or not. I think the issue may be somewhere in the node.install file, in the last update_N() hook, but not 100% sure yet...
Comment #123
Gábor Hojtsy@vasi1186: are you working on this?
Comment #124
vasi1186 CreditAttribution: vasi1186 commentedI checked it a few times, didn't find yet how to solve the issue, but I'm pretty sure it is because the old language settings are not updated correctly. I had a really busy period now, but I will try to check this out again today in the evening, if nobody else will do.
Comment #125
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that should now pass the tests, and the interdiff between #91 and #125
Comment #126
Gábor HojtsyLooks good to me, resolves all concerns raised so far! Thanks!
Comment #127
YesCT CreditAttribution: YesCT commentedHere are screen shots using the latest patch from #125 Everything still looking great.
Steps to test were:
514 cd d8-office
515 git checkout 8.x
516 git reset --hard
517 git pull --rebase
518 git reset --hard
519 git pull --rebase
520 curl -O http://drupal.org/files/language_configuration-1739928-125.patch
521 git checkout -b language_fallback_1776166-125
522 git apply language_configuration-1739928-125.patch
523 drush -y si --account-name=admin --account-pass=admin --site-name=d8-office-language_configuration-1739928-125 --db-url="mysql://root:root@localhost/d8-office"
then enabled translation locale and language
edited the article content type
added a language in the configuration
edited the content type again, also edited the vocabulary
Comment #128
YesCT CreditAttribution: YesCT commentedSome before shots to update the issue summary.
Comment #128.0
YesCT CreditAttribution: YesCT commentedadding to the summary the next step to move the issue forward
Comment #128.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary. with most recent screen shots and uses the issue summary template to try and make it easier to be committed
Comment #129
YesCT CreditAttribution: YesCT commentedI'll number these for ease of referring to later.
This question from webchick in #78, and noted in #88 was not addressed yet. So I'm marking it needs work since I figure webchick will still have the question looking at this for commit again.
This might relate to the locked/flipped question above.
Or the locked/flipped question might be a problem with the previous code that we dont have to solve here. And can be a follow-up.
This todo should be fine. The follow-up issue #1783326: Clicking "reset" does nothing is opened, is being worked on, and is linked back to this issue.
Similar to the first todo, this sanitization question from webchick in #78, and noted in #88 was not addressed yet.
Similar to the first todo, this module_exists wrapping question from webchick in #78, and noted in #88 was not addressed yet.
Comment #129.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary. added the before screen shot of the vocab edit
Comment #130
YesCT CreditAttribution: YesCT commented#1810386: Create workflow to setup multilingual for entity types, bundles and fields is postponed on this issue. Adding that to this issue summary.
Comment #130.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary. with remaining task information about webchicks questions that still need to be addressed
Comment #131
Gábor Hojtsy#129/1: the locked language thing is not at all a problem the code is correct, it is not flipped. Locked languages get a "- NAME -" wrapper where NAME is the language name, the wrapper itself is sent for translations so people can use language-specific wrappers as appropriate. Other languages don't get that wrapper. None of the language names are translated. All of the quoted code is correct and should not have a @todo.
#129/2: we agreed above resolving related sets of CMI config is an existing problem with no existing solution, and we have that issue for it. Should not hold this patch up.
#129/3: for this one, I looked through many call paths, and it indeed looks like in some cases, it might end up with direct form information (machine name to be specific); We should assume that is validated by the machine name input though; I don't see a path where this is called without the form layer? Any specific case you see that should be covered?
#129/4: it is *already* wrapped in a module_exists('language') wrapper, it just happens to be already there, upper in the code and not touched in the patch; it is visible in the patch though.
I think /1 and /4 @todo's are clearly bugos and should be removed. I'm looking for feedback for /3 from webchick (assigning to her for that) and /2 should not hold this patch up.
Comment #132
Gábor HojtsyOne more comment on #129/3: although it is called a CMI path, it is actually a nested CMI key that is being handled there, so ../../../ type of tricks will only make it retrieve/save silly data points, it will not go out of the CMI file that is hardwired in the code. Even if the machine name validation fails for some reason that is. @vasi1186 is working on an update.
Comment #133
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that addresses the issues specified in 129. So:
1. I think Gábor already explained pretty clear why this is like that, so I just removed the @todo from the code.
2. Just removed the @todo from the code.
3. I replaced everything that is not a number, a letter or an underscore, with an underscore. The side effect there is that, for example if you have this combination of variables: ("some type", "some bundle") and ("some_type", "some_bundle") they will return the same key... I think this should actually never happen in core, but if this feature is used by any contrib code, theoretically it can happen.
4. The code was already wrapped in a module_exists(), but that module_exists() call is issued before and does not appear in the patch. This is the entire code:
So it contains also the Language fieldset.
Except 3), where I could still receive some feedback from other people, the other points should be ok.
Comment #134
vasi1186 CreditAttribution: vasi1186 commentedComment #135
vasi1186 CreditAttribution: vasi1186 commentedHad a typo in the previous patch...
Comment #136
vasi1186 CreditAttribution: vasi1186 commentedFixed another small code style issue.
Comment #137
Gábor HojtsyResolved the remaining concern that was deemed valid :) Should be back to RTBC.
Comment #137.0
Gábor HojtsyUpdated issue summary. in irc xjm recommended issues postponed be marked in the summary of the dependency issue.
Comment #138
YesCT CreditAttribution: YesCT commentedThe issue summary is up-to-date and looks like all concerns that have been brought up have been addressed.
Comment #139
Gábor Hojtsy#136: language_configuration-1739928-136.patch queued for re-testing.
Comment #140
Dries CreditAttribution: Dries commentedI haven't tried this patch yet, but I was wondering how we'd deal with language independent tags. For example, imagine I had a tag with name of my cat. It wouldn't necessarily be 'English'. It would be the same name in all languages. I expect that problem to be previously addressed but just making sure.
Otherwise this patch looks good. I will commit it shortly, unless catch or webchick beat me to it.
Comment #141
Gábor Hojtsy@Dries: Language independence is one of the language options that you can assign to terms (as well as nodes, users, etc.). With this patch you can even make a vocabulary that only has such terms and set all of those terms to inherit the language independence from the vocabulary (and hide the UI so it automatically works). Magic :)
Comment #142
YesCT CreditAttribution: YesCT commented@Dries does this look at the values in the drop down help clarify? From #127 http://drupal.org/files/gen-s05-vocab-dropdown-2012-10-12_2249.png
Comment #143
webchickSo all in all, I like this patch a lot.
One thing that worries me a bit though is that in order to "Language selection-ify" an entity, there's quite a bit of copy/paste code. And worse, this copy/paste code seems to be inconsistent between entities.
In the test form:
In content types:
In vocabularies:
As you can see, there are slight variations between all of them. The one in the test module (presumably the simplest implementation) doesn't have a wrapping langcode fieldset, doesn't do a module_exists() check for language. Content types do add a fieldset, and then wrap both it and the field in a module_exists('language') check. However, vocabularies don't. They only do the module_exists() check on the language selector, then does shenanigans with the submit handler that none of the other places do (the comment here doesn't really make sense; node types have a custom submit handler too?).
Now, normally I wouldn't care about consistency pedantry as much (oh, who am I kidding? ;) Yes I would. ;)), but we have at least two more things in D8 we want to convert to entities, which are custom blocks (#1535868: Convert all blocks into plugins) and menu links (#916388: Convert menu links into entities), and possibly user profiles (#1668292: Move simplified Profile module into core). And I literally have no idea which of these 3 patterns is correct to use in those places (or even if they need them, since Comments for whatever reason seem like they don't). Furthermore, this isn't part of any Entity/EntityFormController interface, so nothing is going to bark at any module developer if they forget to add the selector to their config forms. Seems like a recipe for inconsistency.
So it really feels like we need... something... A helper function? An extension to EntityFormController? to make this easier for entity module developers to do the right thing here.
Now. All of that said, this is still an important patch for multilingual that encapsulates a lot of complexity and helps move the ball forward in other areas, so given that Dries's concerns were addressed and given that mine can be tackled in a follow-up, I've committed and pushed this to 8.x, with the addition of some missing PHPDoc on functions in core/modules/language/tests/language_elements_test/language_elements_test.module (please make sure this is done on all RTBC patches in the future; it's part of the documentation gate).
But let's please make sure that these questions are answered in the change notice and, if possible, a follow-up to make it easier for developers to incorporate this element into their modules.
Comment #144
plachTotally agreed with #143. We need more consistency and (even) more ease to set things up. Ideally it should be possible to trigger all this stuff automatically but just providing some entity info key or stuff like that.
That said, awesome work, guys! This is really a cornerstone for entiyy language support!
Comment #145
webchickTaking this off my plate.
Comment #146
webchickWe're once again over the critical task threshold. Please try and find time to write up documentation for your API change ASAP so your fellow contributors' features aren't blocked. Thanks.
Comment #147
vasi1186 CreditAttribution: vasi1186 commentedThe change notification can be checked here: http://drupal.org/node/1816456
Comment #148
webchickAwesome work, this is definitely great as a start, and walks through the API well.
As discussed in IRC, one element that's missing though is a full working example for people upgrading their modules. vasi said he can work on that a bit later today.
Since the change notice exists, and this is just further refinement, downgrading to a "normal" task.
Comment #149
vasi1186 CreditAttribution: vasi1186 commentedAdded the working example: http://drupal.org/node/1816456
Comment #150
Gábor HojtsyOk @vasi1186 just updated with a usage example which looks good to me.
Comment #151
Gábor HojtsyComment #152
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #153
plachPlease clarify if this is by design, but this is the language configuration of taxonomy terms and it's using 'vocabulary' as entity type. This is pretty weird and is breaking things in the translation UI, see #1188388-154: Entity translation UI in core
Comment #154
vasi1186 CreditAttribution: vasi1186 commentedThe thing is that the language_configuration form element has to use an entity type and a bundle name. In case of taxonomy terms, they are entities, but they do not have entity types and bundles, as for example nodes have. So, the idea was to just use 'vocabulary' as the entity type, and the name of the taxonomy as the bundle. Basically, the goal of the (entity_type, bundle) combination is to uniquely identify the object on which the configuration applies. So, in the end, it can be actually anything, but probably in most of the cases will be the entity type and the bundle name, that's why they were also chosen as keys (there were some discussions in the beginning regarding these names). If using 'vocabulary' as the entity_type breaks something (can you also tell what is actually breaking?), then we can change them, the only thing is to find something that uniquely identifies the vocabulary (which in this case is its machine name).
Comment #155
andypostSo what's the initial language for vocabulary here #1809816: Drop remains of landcode in favour of language element ?
Comment #156
vasi1186 CreditAttribution: vasi1186 commentedActually, these settings apply on the taxonomy terms, not on vocabularies. So, you can specify the default language settings for the terms of a vocabulary (on the vocabulary edit page), not for the vocabulary itself. For the case you pointed out, there is no change that has to be made (unless we implement some feature to set the default language configuration for vocabularies).
Comment #157
plachThis is not correct: every entity type has to to explicitly declare itself just to be defined. Moreover taxonomy terms have vocuabulary machine names as bundles, see: taxonomy_entity_info().
Given that, I think we should be simply be using the
'taxonomy_term'
entity type instead of the'vocabulary'
one here to make things work properly. Incidentally this is what the latest patches in the entity translation UI issue are doing and updated tests pass whitout problems.Comment #158
vasi1186 CreditAttribution: vasi1186 commented@plach: yes, you're right, sorry, the vocabulary machine name is the bundle name (they are actually used for the bundle of the #entity_information). But that's actually not the most important thing. The important thing is to have the combination (entity_type, bundle_name) unique for every object on which this element is used. If using 'taxonomy_term' instead of 'vocabulary' it is really fine from my point of view.
Comment #159
Gábor HojtsyShould it be patched then? :)
Comment #160
vasi1186 CreditAttribution: vasi1186 commentedYes, I think we should create another issue for that?
Comment #161
plachI can post an excerpt from the UI patch that already fixes this.
Comment #162
plachHere it is.
Comment #163
andypostVocabulary is an entity, so should have own language. After #1552396: Convert vocabularies into configuration vocabularies are should be configentity
This is a form of vocabulary entity itself. So in case of the form that adds new vocabulary (a bundle) for term it looks ugly to try to find a language for bundle withing none-existent terms.
Comment #164
plach@andypost:
Not sure I get your point. Vocabularies are indeed entities and currently have their own language selector in the Vocabulary form. As you were correctly pointing out in #155, we need a default also for it. However we cannot obviously use the one in the Vocabulary form, since that one govern the taxonomy term language selector and being able to configure a default only after needing it sounds a bit silly.
I think we need a configuration at upper level. IMO a possibily easy way to fix this could be a global language configuration default in the language settings that could be inherited by every entity type not needing something specific, for instance all Configurable entities having a language selector could use it.
Comment #165
vasi1186 CreditAttribution: vasi1186 commentedAgree with plach, that element has to be used on an upper layer. For example: in the case of nodes, it is used on the node type edit form to manage the default settings of the node add form. For taxonomy term, the form is used on the vocabulary edit form to manage the default settings of the term add form. So, for the language of the vocabularies, there has to be some upper level where we could set this.
Comment #166
vasi1186 CreditAttribution: vasi1186 commentedaccidentally changed the status before.
Comment #167
andypostAs I get it right, we need a some default language for/per bundle
- nodes: language defaults for each type managed at admin/structure/types/manage/[node_type]
- terms: language defaults for each vocabulary managed at admin/structure/taxonomy/[voc-name]/edit
Suppose the defaults should be language interface as top-level setting
Comment #168
plachI think a global default is something that needs some discussion and deserves a dedicated issue. Can we RTBC this meanwhile?
Comment #169
andypostSorry, I don't get that this defines a term language. Makes a lot of sense!!!
This context I missed...
After this commited we should re-open #1809816: Drop remains of landcode in favour of language element to discuss default language for vocabulary because it could be used in forum title for example #148145: "Forums" title is not localized
Comment #170
Gábor HojtsyMoving on sprint since this bug blocks #1188388: Entity translation UI in core.
Comment #171
vasi1186 CreditAttribution: vasi1186 commentedThe patch in #162 looks good to me.
Comment #172
catchCommitted/pushed the follow-up.
Comment #173
Gábor HojtsyThanks a lot!
Comment #175
YesCT CreditAttribution: YesCT commentedRelated to comment #9 and #100
Noticed #2014955: Deleted bundles do not have their language configuration deleted while working on #1945226: Add language selector on menus
Did we have tests for deleting config?
Comment #176
YesCT CreditAttribution: YesCT commentedSince I'm trying to add a test for #2014955: Deleted bundles do not have their language configuration deleted
I was looking for a test to see if the config was deleted when the bundle was. I found this test, which is checking if "old" config is deleted when something (machine name) changes. Not quite the same thing I was looking for.
But, while investigating, I changed the machine name of the tags vocab. And this is what is in the language settings file:
Comment #177
Gábor HojtsyLooks like we need an issue for machine name changes too? :)
Comment #177.0
Gábor HojtsyUpdated issue summary to reflect that remaining webchick questions were answered and are no longer todo.