Problem/Motivation
Contact categories, custom block types, new block configuration, filter formats, user roles, etc. are saved with unknown language. However these config entities contain user facing textual pieces, so this becomes a problem when translating, given that we don't know the source language. The Drupal shipped configuration should be English, the user created configuration should use specific language codes too. Some of the shipped configuration was fixed in #1935000: Some configuration entities are created as in language unknown but there were some left around.
Proposed resolution
- Fix shipped configuration to use langcode: en as per #1935000: Some configuration entities are created as in language unknown.
- Fix creation of new configuration entities to use the site default language (or include a language selector when needed)
Remaining tasks
- Review
- Test
User interface changes
There will be a UI, if we need to change/create one or more of the entity forms.
API changes
No API changes.
Related issues
#1935000: Some configuration entities are created as in language unknown for previous fixes.
#1935022: Add a language selector on views fixed this for Views.
#1945226: Add language selector on menus fixes this for newly created menus (built in menus were fixed in the first patch).
Comment | File | Size | Author |
---|---|---|---|
#40 | 1947814-config-entity-40.patch | 12.87 KB | vijaycs85 |
#40 | 1947814-diff-33-40.txt | 1.08 KB | vijaycs85 |
#33 | 1947814-config-entity-33.patch | 12.75 KB | vijaycs85 |
#33 | 1947814-diff-31-33.txt | 621 bytes | vijaycs85 |
#31 | 1947814-config-entity-31.patch | 12.15 KB | vijaycs85 |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedrelated? #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable
Comment #2
Gábor HojtsyNo, contact categories are NOT content entities, they cannot have bundles. #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable is not related.
Comment #3
Gábor HojtsyRetitled for the general problem that affects multiple modules. Here is a patch covering the following:
- custom block type form
- block config form
- contact category form and the one shipped category
- filter format form
- user role form and the two shipped user roles (the standard profile shipped admin role is already ok)
Manual testing of these worked. Not sure what kind of automated testing we should put behind these.
Other possible config entities, that might be affected: breakpoints, breakpoint groups, editors (does not seem to actually have user facing textual content though), entity displays, image styles, picture mapping, shortcut sets. Note that vocabularies have this covered via the language selector. Views got a language selector recently in #1935022: Add a language selector on views and menus are proposed to have a language selector in #1945226: Add language selector on menus. These other config entities do not have language selectors (and might or might not need one). At the minimum we need them to save in a sensible language code, so they are translatable. Contrib could also put language selectors on them by altering the forms if it wants to.
I also figured that custom blocks for some reason want to add a language selector but blocks don't have them by default. This patch might break that. Hopefully we have some test coverage for that which will catch it :D
Let's get reviews and testbot running.
Comment #4
Gábor HojtsyForgot to add proper langcode to shipped custom block type too.
Comment #5
Gábor HojtsyUpdated issue summary for full scope.
Comment #5.0
Gábor HojtsyUpdate summary.
Comment #6
vijaycs85#4: config-entities-new-save-as-default-language-4.patch queued for re-testing.
Comment #7
vijaycs85Looks good to me, sending it for re-testing... RTBC if green...
Comment #8
vijaycs85Just had a word with @Gábor Hojtsy on IRC. This needs tests. adding tag...
Comment #9
tim.plunkettWhat about this?
Comment #10
tim.plunkett.
Comment #12
Gábor Hojtsy@tim: well, that breaks most entities :) They assume there is no pre-existing langcode value field that they would need to unset / override. I'm not sure its best to now go and modify the forms that extend from the base entity. At least I'd say this should be in the config entity form base class to begin with, not the generic entity form base class.
Comment #13
Gábor Hojtsy@tim.plunkett: seems like there is no form class for config entities in particular. We can either introduce one (inheriting from EntityFormController specifically), do the changes as needed individually, like we did above; or make the base form addition conditional on whether there is already an element for that. In the attached patch I chose this last option, so it keeps the added langcode element as-is if already provided by extended implementations. The parent form controller is invoked last after the elements are added so this should be a possible solution. What do you think?
Comment #14
vijaycs85Updating test cases to check the config entities are getting saved in site default language instead of und.
Comment #15
Gábor HojtsyYay, test updates look great :)
This looks suspicious. The langcode should be the default langcode regardless of whether the format is new or old. Shipped format config files lack langcode: en in them (or have langcode: und specifically)?
Comment #16
vijaycs85#14: 1947814-config-entity-14.patch queued for re-testing.
Comment #17
vijaycs85As discussed on IRC, the problem is langcode in Entity class.
and
Fixes the issue. However not sure, if it can be changed.
Comment #18
Gábor HojtsyI think that would be an interesting cut through the base of part of the problem. Not sure how it affects existing entity types, but we will see.
Comment #20
vijaycs85Updating patch that fixes 4 out of 7 fails in #17
Fixed:
BlockStorageUnitTest.php (line: 109)
ConfigEntityTest.php (line: 44, 58, 100)
SaveTest.php (line: 46)
UserPictureUpgradePathTest.php (line: 47)
Todo:
Basically failing when try to pass langcode by anyway.
1. TermLanguageTest.php (line: 66, 70, 78)
2. TranslationLinkTest.php (line: 58)
3. UserLanguageCreationTest.php (line: 75, 93)
Comment #21
vijaycs85Comment #23
Gábor HojtsyI looked at the taxonomy test. It seems like for some reason the selected langcode does not override the original one added with this patch to init to the site default. I could not track down the reason, the taxonomy form uses the regular http://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/e... code that seems to be capable to override it... Any help in tracking this down would be great! Thanks!
Comment #24
vijaycs85Updating defualt langcode in ConfigEntityBase instead of Entity.
Comment #25
tim.plunkettThis should be in the create method of the storage controller.
Comment #26
vijaycs85Thanks for that quick review @tim.plunkett. Does it work?
Comment #28
vijaycs85#26: 1947814-config-entity-26.patch queued for re-testing.
Comment #29
vijaycs85For 2 fails in #24, just need fix on content entity test case asset changes made on #20.
Still not sure about 5 fails in #26. Sent it for re-testing...
Comment #31
vijaycs85Fixing all 4 test cases.
Comment #33
vijaycs85Updating test fail in ConfigImportUITest.php...
Comment #34
andypostBut how to pass special language when needed to init with 'und'?
Comment #35
vijaycs85@andypost, as far is I discussed with @Gábor Hojtsy, we always define config entity with site default language.
Comment #36
tim.plunkettI've not heard that, so I'm not going to RTBC. If @Gábor Hojtsy confirms, this is good to go from my perspective.
Comment #37
YesCT CreditAttribution: YesCT commentedI took a look, and the changes make sense while reading the code and it looks good standards wise.
Comment #38
Gábor HojtsyI agree we need to answer the question: "How is it possible after this patch to create entities in other languages". Looks like this will overwrite the langcode value provided in create($values) with the site default language? What about only doing this fallback is there was no langcode provided in $values? That would be possible before the new $class code in create() even, like so:
Instead of overwriting whatever got into the object in the first place. What do you think?
Comment #39
tim.plunkettYes, that's how we should do it if we care about config entities having langcodes. Glad I asked :)
Comment #40
vijaycs85Updating changes mentioned in #38
Comment #41
Gábor HojtsyYay, looks great now. Thanks!
Comment #42
alexpottCommitted e97a786 and pushed to 8.x. Thanks!
Comment #43
Gábor HojtsyWoot, superb, thanks! I've added a change notice for this for reference in the future for ongoing conversions. http://drupal.org/node/1965388
Comment #44
YesCT CreditAttribution: YesCT commented@tim.plunkett pointed out to me that in #1945226-20: Add language selector on menus
I had to move the call to parent::form to the end.
Because this patch checked first if langcode was added, and if not it added it.
Then in the menu form function, it tried to add it in a different place, but it was already added.
I'm making a note here in case there are other places beside menus where we want to move the call to parent::form
Comment #45
Gábor HojtsyYeah, we found at other places the parent form call was at the end, which is the expected way to write the form :) We also added automated testing to the above items, so they should be covered/verified. Menus were not touched in this issue specifically since #1945226: Add language selector on menus exists.
Comment #46.0
(not verified) CreditAttribution: commentedFix ref in UI text section.