Suggested commit message
Issue #1952394 by vijaycs85, tstoeckler, webflo, Gábor Hojtsy, Schnitzel, falcon03, YesCT, kfritsche, Ryan Weal, dagmita, likin, toddtomlinson, nonsie, Kristen Pol, dawehner, tim.plunkett, penyaskito, EclipseGC, larowlan, robertdbailey, helenkim, David Hernández, EllaTheHarpy, lazysoundsystem, juanolalla, R.Hendel, Kartagis: Add configuration translation user interface module.
Problem
Drupal 8 has the configuration schema system with schema files that describe configuration so translatable pieces can be identified. With #1905152: Integrate config schema with locale, so shipped configuration is translated committed, locale module uses this to pull in translations from localize.drupal.org and save locally, so shipped configuration is translatable. However for edited configuration or when creating new configuration (your own blocks, your own views, your own contact categories, menus, taxonomy vocabularies), there is no UI whatsoever to translate those. Heck, there is no user interface to translate your site name.
Regressions resolved by this issue
There are also regressions in Drupal 8 fixed by this issue.
- Drupal 7 allowed to provide a native language name which Drupal 8 does not allow (in favor of translating language names and using the native translation). This module provides the user interface for that.
- Drupal 7 also allowed localization of date formats. There were bugos core implementations for that which were removed at #2127941: Remove two (out of 3) bogus and duplicate date languages UIs. This issue provides the user interface to actually translate date formats.
Proposal
Add a configuration translation user interface in core. The module builds forms off of configuration schemas and automatically attaches translation operations to overviews and edit forms. The most up to date work can be followed at http://drupal.org/project/config_translation with regular core patches posted here.
This would normally not be proposed but given http://buytaert.net/code-freeze-and-thresholds, there is a possibility to get new features accepted theoretically. Also Dries indicated earlier this would not be a fit for Drupal 8, but some of the rules changed since then. See #1648930-310: Introduce configuration schema and use for translation for earlier comments on that.
Finally, while we have been working on this issue, we identified and resolved dozens of core bugs and developer experience problems, all of which made Drupal better. This module is in many ways sitting across subsystems like Views. It maps sets of configuration keys to routes. These sets may be based on config entities as well. Then it generates new routes based on those, as well as tabs, config entity list operations and contextual links. These lead to pages served by a pluggable system presenting automatically generated language overviews and then automatically generated translation forms relying on the configuration schema API and typed configuration. The saved files are picked up by locale module as configuration translations.
High level architecture of the module
This is not a 100% detailed structure diagram, since that would probably be more confusing than helpful. This level should help understand what this module does and what subsystems are involved. In short, the module's sole purpose is to write config translation files (at the bottom) providing a user interface to that. Producing those user interfaces is based on configuration schemas (to structure forms) and then the placement of those pages in the right parts of the admin experience are ensured by mapping routes to the configuration names being edited. So in-context translation experiences are provided. For translator-only access, there is also an index of translatable configuration that only requires translator permissions, so every use case is covered.
This is a pure UI module that interacts with existing data from Drupal core from configuration, routes, configuration schema and locale overrides. The later is the only thing that it actually writes (or deletes) based on user interaction.
(Original copy at https://docs.google.com/a/acquia.com/drawings/d/1FBAw-x-_PdtZdco3I4aRDDI...)
Sampling of user interfaces provided
Entry points provided are in-context (tab (2), operation for entity lists (4) and contextual link (3) as appropriate). For those without other admin permissions, there is a dedicated index of all translatable configuration (1):
The translate operations lead to a summary screen with language translation status and actions for editing/deleting and adding translations:
The overview screen delete operation leads to a standard confirm form (no screenshot here, its trivial). The edit/add operations lead to config schema based forms:
Remaining tasks
tl;dr: Commit!
We are only going in circles at this point doing different cleanups. These can be done anytime and will need to continue to adapt to core API changes anyway later on. The module has plenty of test coverage and singlehandedly found dozens of core issues since we are working on it. It has extensive phpunit test coverage and was accessibility tested (see #2003834: Accessibility fixes based on suggestions and then validated in person by @falcon03). See issues at http://drupal.org/project/issues/config_translation for ongoing work, but these can be moved to the core issue queue as well later on.
Core blockers are all fixed. There are core issues that may make parts of the code simpler. All in-code @todos are core dependent and have proper links to core issues.
Testing steps
1) Install Drupal 8 with this patch (or even better check out the 8.x-dev branch of the contrib config_translation module, eg click on http://simplytest.me/project/config_translation/8.x-1.x-dev and start testing there)
2) Enable configuration translations under 'Extend'
3) Add French and German as languages under 'Regional and language configuration' (and/or any other languages)
4) Go to 'Configuration' - 'Site information' => do the translations on this page.
5) Check that site name translates on front page (enable language switcher block for easy language switching)
6) Do the same for Views, contact categories (get multilingual contact emails!), block titles, user email text (get user's language specific block/approval emails) etc.
See also #81 through #83
Known (core) issues
Note for anyone testing, you will most likely hit the "bug" that not all config entities have a proper translation tab. That is due to #2135787: Move static config entity local tasks to local_tasks.yml and #2111823: Convert field_ui / Entity local tasks to YAML definitions which are both core *lack of conversions* of old tabs to new tabs. The old tabs are not added to the new tab system, so we cannot extend them. There is *nothing* to change in this module, as the new tab conversions happen, the translation tabs will automatically show up. Unfortunately those two issues can only be fixed at the same time, again because they convert the old tabs to new tabs for the same pages in different modules.
However although not all config entities have translation tabs, they do have several other entry points as shown above, eg. operations buttons in list controllers and the config translation index itself. So this is not something this patch could change/fix in any way, it is dependent on core conversions that did not happen yet, and the translation pages are accessible and work perfectly.
Sub tasks
Summary of sub-tasks that has been found & fixed in config_translation module, since this issue created.
- #2003834: Accessibility fixes : Heavy Accessibility Improvements (checked by falcon03)
- #1999156: Responsive Configuration Translation UI : Made Responsive and Mobile First
- #2002614: Module name not capitalized properly : Properly capitalized Module Name
- #1998116: Mixed types invoke notices. : Fixed Bug where there where some notices, also added Tests
- #2004242: Fix undefined constant LANGUAGE_NOT_SPECIFIED : Updated for recent Core Changes
- #2004194: Add getLanguageWithFallback() to ConfigGroupMapper : Added getLanguageWithFallback() to ConfigGroupMapper which makes in other places nicer code
- #1999192: Saving of Views Configuration Translation not possible for Display Settings : Fixed Bug where Saving of Views Configurations Translation was not possible and added texts
- #2017887: config_translation.services.yml standards cleanup to get ready for getting into core : service.yml clean up
- #2014965: Remove backward compatible ControllerInterface : Removing backward compatability of controllerInterface
- #2011294: config_translation.admin.css standards cleanup to get ready for getting into core : admin.css clean up
- #2011320: config_translation.api.php hooks standards cleanup to get ready for getting into core : API file cleanup
- #2011348: config_translation.module standards cleanup to get ready for getting into core : Clean up
- #2004690: taxonomy page broken : Fix for taxonmy page broken
- #2019121: All configuration translation pages throwing FATAL error : FATAL error.
- #2020347: Temporary fix for config_translation_enter_context until language context becomes an option : Context fix
- #2020343: remove instance of locale_storage() : locale_storage()--
- #2020867: Update contact module URL in test cases. : contact module URL fix
- #2020961: Warning: undefined index 'source' : Warning fix
- #2017987: RouteSubscriber.php standards cleanup to get ready for getting into core : Route subscriber cleanup
- #2017975: ConfigEntityMapper.php standards cleanup to get ready for getting into core : ConfigEntityMapper.php clean up.
- #2017977: ConfigGroupMapper.php standards cleanup to get ready for getting into core : ConfigGroupMapper.php clean up
- #2004428: Less ugly operations altering : Operation alter
- #2022335: Use LanguageConfigContext instead of UserConfigContext work around : Use LanguageConfigContext
- #2028137: Integrate field configuration translation on the user interface
- #2035393: make ConfigTranslationManageForm label and text conditional on the translatable schema property
- #2017985: ConfigTranslationManageForm.php standards cleanup to get ready for getting into core
- #2011332: Add alter for adding config translation tab
- #2035453: fix mis-spelling (tthree) in help
- #2035449: remove unused arg, which might conflict eventually anyway
- #2029029: check translate links work, and write tests (5d688f0)
- #2029029: check translate links work, and write tests (8ce10bd)
- #2031207: Create EntityMapper for language config entity
- #2031255: Update for new languages as config entities
- #2028127: clean up property names to be camel case instead of underscore separated words to cleanup to get ready for getting into core
- #2028649: Translates operation always added
- #2028067: Clean up the added tests, by not calling procedural wrapper and fixing grammar
- The test should now expect the manage path for filters. (7a211a5)
- #2027817: Convert filter admin altering to the alter hook (5ee63a6)
- #2027817: Convert filter admin altering to the alter hook (d72db4e)
- #2027587: Add tests for custom blocks, contact forms, formats, shortcut listings and settings pages.
- #2003988: Remove explicit path_id_index and config_prefix from mappers because we can compute them automatically
- #2027587: Add tests for custom blocks, contact forms, formats, shortcut listings and settings pages.
- #2027577: Missing translate operation for maintenance settings page
- Clean ups (767a2bf)
- #2004710: Add tests for block, menu, vocabulary and views listings
- #2017981: ConfigTranslationController.php standards cleanup to get ready for getting into core
- #2017983: ConfigTranslationDeleteForm.php standards cleanup to get ready for getting into core
- #2017979: ConfigMapperInterface.php standards cleanup to get ready for getting into core
- #2017989: ConfigTranslationUITest.php standards cleanup to get ready for getting into core
- #2026875: Remove accidental badaboom
- #2038743: docs cleanup use configuration instead of config
- #2044387: Add remaining configuration entity or page into configuration translation module
- #2054183: shortcut renamed
- #2043453: Translation of languages not possible
- #2029407: Add support for node types
- #2044387: Add remaining configuration entity or page into configuration translation module
- #2044387: Add remaining configuration entity or page into configuration translation module
- Several misc updates (574b53f)
- #2065309: Custom block types changed type listing path in core
- #2035455: use twig template instead of function theme_config_translation_manage_form_element()
- #2011290: [meta] standards cleanup to get ready for getting into core
- #2083231: Avoid making up custom terminology: group => names
- #2070055: drupal_set_title() is deprecated
- #2070023: Convert hook_config_translation_group_info to plugin further improvements (e7b6439)
- #2070023: Convert hook_config_translation_group_info to plugin
- #2078805: Rename ControllerInterface to ContainerInjectionInterface
- Remove todo item (82aca4f)
- #2093007: drupalPost => drupalPostForm() in tests and other core fails
- #2085739: Unify tabs, add contextual links
- Remove todo item (d739da4)
- #2085603: Admin path of blocks changed, menus not prefixed with menu- anymore
- #2099509: Add theme column to block entity list controller
- #2098675: Add missing tests for config_translation.mapper_list and config_translation.entity_list
- #2099381: The title gets escaped twice
- #2095145: Provide Config translation UI
- Fix missing user facing entity label (2f9b5e0)
- #2035441: flush caches after deleting translation (config)
- Remove extra entitymanager accidentally added (9ea78f8)
- #2095265: Update Config mapper plugin manager to discovery mapper properly. - Fixed Update Config mapper plugin manager to discovery mapper properly(4a6fee8).
- #2085925: Autogenerate config entity translation mapping as much as is sane
- Fix names of entities (bb6b962)
- #2095265: Update Config mapper plugin manager to discovery mapper properly. - Follow up c5a5976
- #2095265: Update Config mapper plugin manager to discovery mapper properly.
- #2044737: Provide local tasks as plugins
- Remove comment from commented out test. (bac198d)
- #2111087: Make the translation add and edit forms separate classes
- #2111013: Site information is displayed in the wrong language on the site information translation page
- #2110491: Modernize and clean-up controller and forms
- #2097101: Remove deprecated functions
- #2100257: ConfigTranslationDeleteForm needs to implement getCancelRoute method
- Fix docs to not refer to hook_menu() anymore, also cross reference the mapper manager (4ff8d93)
- #2095269: Remove add_edit_tab, core should have default tabs
- #2096003: Make the help page for Configuration translation standards-compliant
- #2100221: Refactor form element into a class
- #2100359: Fix doFieldListTest in ConfigTranslationListUITest
- #2100347: Use checkPlain for Entity lables, bundles etc.
- Duh. Duhuh. (328557d)
- Fix page title for config entity translation lists (0fe6799)
- #2094987: Use InfoHookDecorator for ConfigMapper declarations
- #2119497: Default local tasks are not to be assumed $route_name . '_tab'
- #2114471: Add unittest for ConfigEntityMapper class
- #2113707: Create a ConfigTranslationListControllerInterface
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- #2104925: Convert to work with routes instead of paths in mappers
- Fix strict warning for invoking non-static method on class without instantiating (acc1f26).
- followup suggestion by tstoeckler: Make sure we are checking access to an existing route (a3475e1).
- followup by tstoeckler: Use ElementInterface and remove abstract method that is already on the interface (d972834).
- followup by tstoeckler, vijaycs85: allow reordering label and language name in t() (61e2ece).
- followup suggestions by tstoeckler: Cleanup for date format ajax handling (076faa6).
- #2114931: Create a FormElementInterface
- #2115165: ConfigNamesMapper::getLanguageWithFallback() logic not clear
- #2099997: Use config translation to translate date formats
- #2117015: Check access to edit link with current route access checking
- #2116989: Clean up type altering hook
- followup by Gábor Hojtsy: More fixes for the big larowlan review (e3ff66d).
- #2113701: ConfigTranslationFormBase should implement BaseFormIdInterface
- #2115015: Make use of brand new RouteSubscriberBase
- #2113283: Small clean-ups all around...
- #2114439: Fixes for the big larowlan review
- #2114899: Update for new drupal_get_form() usage
- Remove newline accidentally added (0c8c246).
- #2114439: Fixes for the big larowlan review
- #2106563: ConfigMapperManager cache prefix renamed
- #2113989: Re-enable blocks testing once core bug is fixed
- #2113503: Editing the source configuration should lead back to the translation overview
- followup by Gábor Hojtsy, tstoeckler: Improve config translation test coverage by testing mapper altering (9880a27).
- #2095383: Improve config translation test coverage by adding a test module
- #2123575: Fixes for tim.plunkett and dawehner review
- followup further fixes for tim.plunkett review(3ef10b7).
- Fix for access interface compliance following core change in #2048223; removes one core dependent @todo, yay! (8e3dbfe).
- #2117711: Add tests to date format translation UI
- Add test theme for #2123897: Restore theme .yml support, add/fix tests (f7db3be)
- Add description to config test module (05ddf2e)
- Fix order of 'use' in access check (e2c41d6)
- #2124455: Impact of 2048223 - Add $account argument to AccessCheckInterface::access() method and use the current_user service
- Fix parse error in static create (208adb6).
- Further impact of #2048223 - Add $account argument to AccessCheckInterface (b825e10)
Related
- #2004282: Injected dependencies and serialization hell
- #1910624: [META] Introduce and complete configuration schemas in all of core - will expand the reach of this module to things like entity form modes, etc.
Comment | File | Size | Author |
---|---|---|---|
#175 | config_translation-1952394-175.patch | 235.33 KB | tim.plunkett |
#153 | config-translation-module-153.patch | 235.87 KB | Gábor Hojtsy |
#150 | config-translation-module-150.patch | 232.83 KB | Gábor Hojtsy |
#144 | 1952394-config-translation-core-144.patch | 196.5 KB | Gábor Hojtsy |
#136 | 1952394-config-translation-core-136.patch | 191.02 KB | vijaycs85 |
Comments
Comment #1
Gábor HojtsyAdd config schema tag for tracking.
Comment #2
Gábor HojtsyHere is a snapshot of the current version of the module. It puts translation tabs on things like taxonomy vocabularies, contact categories, views, site information, account settings and so on and on, and lets you translate them to foreign languages. There is obviously a form generation layer on top of the schema system, however there are three additional "layers" that we needed to build
- define configuration file groups, since some forms use multiple config files (eg. account settings form in core), we need to display the configuration for these at one screen too
- define a path to configuration file group system with optional Edit tab addition, so screens having no Edit tabs yet get a tab for that, so they can switch from translation to editing and vice versa
- define an entity mapping system on top of this for config entities, so path patterns are handled and concrete entity loads are then translated to above said config file groups
So the real missing pieces were UI <=> config files where UI is path first and foremost but also optional edit tabs, option to create translate as a tab (most of the time) or just callback (eg. views), etc. I don't think we can extract this kind of information from anywhere, so we have our own way to specify these. We are chipping away at tasks at http://drupal.org/project/issues/config_translation?categories=All but any review comments are welcome nonetheless.
Comment #3
plachIf I undesrtand correctly we had almost the same issues with ET. I guess our best bet to solve these needs the proper way is #1839516: Introduce entity operation providers and friends as you already pointed out there.
Comment #4
Gábor HojtsyScreenshots:
Site information (look translate tab):
Structure of translate tab:
Adding one translation (originals copied in by default):
Translate tabs are added on things, like views listing, menu listing (shown here), vocabularies listing, etc. (note translate operation):
A more complex form, the account settings form that has structured input for emails subject/body pairs:
Comment #5
Gábor HojtsyUploaded one picture twice accidentally.
Comment #6
Gábor Hojtsy@plach: yeah, the operations API would help a bit. It would still not match non-config entity config files to paths and it would likely not give us the title in the form we need to produce a reasonable page title. So either way, this is a design decision in the config system that the config storage is entirely disconnected of the forms and UI managing the config, so we cannot really tell this without codifying the relationships for our needs AFAIS (at least for non entity config).
Comment #7
Gábor HojtsyAdd on D8MI sprint for tracking.
Comment #8
Gábor HojtsyNew roll with more testing coverage! Two new sets of tests added, some bugs fixed.
- http://drupalcode.org/project/config_translation.git/commit/1010f68d843e...
- http://drupalcode.org/project/config_translation.git/commit/369717ca0197...
Comment #9
YesCT CreditAttribution: YesCT commentedI tried this out.
Original values need some work still #1945184-10: Original value display is broken
And it would be really nice to get #1910606: Improve the configurations schemas for Views significantly in to try this out with views more.
It can still use more reviews so leaving it needs review.
Comment #10
sunThis definitely has to be shipped with D8 core.
Let's not repeat the mistake of introducing an API in core without any API consumer in core. We know that this does not work out.
This is RTBC, as soon as it's RTBC.
Comment #11
Gábor HojtsyThis code definitely needs to use the new routing system, forms classes, and so on I think. Many of those things are still baking and being applied to different parts of Drupal, so were not readily available when we worked out the initial version.
Comment #12
mondrakeI also tried this out, nicely. I can not help more, but to say big +1 to comments in #10. Please get this in D8 when it is ready, would love to have as a feature to use in contrib.
Comment #13
vijaycs85Updating new routing system and form (more details at #1985880: Convert to routing system) changes.
Comment #14
vijaycs85Re-rolling to get changes of #1989878: Add 'type' in info file. which is because of #1292284: Require 'type' key in .info.yml files.
Comment #15
Gábor HojtsyNote that the changes in #13 and #14 as well as the above have been at least peer-reviewed in http://drupal.org/project/config_translation
Comment #20
YesCT CreditAttribution: YesCT commentedgood. it passed. I ran that failing test locally earlier today and it passed also.
Comment #21
falcon03 CreditAttribution: falcon03 commentedHas anyone reviewed this UI for its accessibility? If not, is it ready for an accessibility review?
Comment #22
Gábor HojtsyIt might not be the final UI, but I think its ready for an accessibility review, yes. There may be minor accessibility issues with the source value markup, but generally we are just using plan operations lists, translation tabs with classes Drupal tables and classic Drupal forms. There is no JS for example in the whole module :) There is likely possibility to add some extra metainfo to the markup to aid accessibility definitely. Tips around that would be welcome!
Comment #23
falcon03 CreditAttribution: falcon03 commented@Gábor Hojtsy: thank you very much for your reply. I'll be reviewing the UI as soon as I can. So I'm adding the "needs accessibility review" tag.
Comment #24
vijaycs85Comment #25
YesCT CreditAttribution: YesCT commented@vijaycs85
HA! :)
Comment #26
victor-shelepen CreditAttribution: victor-shelepen commentedHello. I've installed a module. But I can not find any local menu tabs to translate. I continue to investigate why.
Comment #27
vijaycs85@likin, here is check list:
1. Have more than 1 language enabled?
2. Logged in as admin or user with permission to administer config translation
3. Are you checking Core config page(site information is a easy one).
4. Last and bit silly, sure it is up to date config_transalation module from d.o/project/config_translation ?
Comment #28
victor-shelepen CreditAttribution: victor-shelepen commentedIt works now. There were problems during the installation. I've fixed. Thank you.
Comment #29
Schnitzel CreditAttribution: Schnitzel commentednewest patch from the module so that it can be applied at simplytest.me
Comment #30
dagmita CreditAttribution: dagmita commentedI tried to translate the site name.
Steps I went through:
1) installing drupal 8 with this patch
2) enabled languages under 'exted'
3) added French and German as languages under 'configuration'
4) went to 'configuration' - 'site information' => on this page I did not find the translation tab
Comment #32
dagmita CreditAttribution: dagmita commentedI figured out my mistakes in testing. so the comment above is outdated. I added the correct steps of testing in the issue summary.
Comment #33
attiks CreditAttribution: attiks commentedi think we add a space after ':'
there are some magic numbers used in this function
Comment #34
attiks CreditAttribution: attiks commentedI just tried the patch and wauuw, great job
Comment #35
falcon03 CreditAttribution: falcon03 commentedI reviewed this UI today during a code sprint at Drupalcon Portland.
The interface works great, but it has some accessibility issues.
First off, it is hard to distinguish the "source" information from the form field where to type its translation. This issue could be solved by adding the language to the label of the information that you are translating.
So, for instance, to translate the site name you could have something like:
Site name (Source language): site_name
Site name (Language_that_you_are_translating_the_information_into): form field to translate it
If this additional information should not be shown to sighted users, it can be wrapped in a span/div element which the "element-invisible" class can be assigned to, so that the information is available for screen reader users, but not for sighted ones.
When you try to translate the displays of a view, you get lots of buttons labelled "display settings". You can understand what display a button refers to only after clicking it. This is not good for accessibility and (maybe( for usability too. And I was also wondering: does it make sense, from a usability point of view, to have the display settings in collapsible details elements?
And finally, I think that the Source text that you are translating into another language should be wrapped in a div/span element, which "lang" attribute should be set to the appropriate value according to the source language. It could be also appropriate to set the "lang" attribute of the form element to translate an element (e.g. the site name) to the appropriate value according to the language that the text is going to be translated into.
Comment #36
Schnitzel CreditAttribution: Schnitzel commentedthis will be fixed here: http://drupal.org/node/1999156#comment-7454380
issues will be fixed here: #2003834: Accessibility fixes
Comment #37
vijaycs85Reg
in #33
we need them to find the position of entity in URL.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedSite configuration seems to work for site_name but it does not work when trying to enable block translation.
I have tried to enable block translation on the "Custom language settings" page. After doing this, when I create a block I am prompted for a language. However, when trying to add a translation I get the following error so I cannot get to the translation page:
Fatal error: Call to a member function getLangcode() on a non-object in /var/www/drupal/core/modules/config_translation/lib/Drupal/config_translation/Access/ConfigNameCheck.php on line 34
I saw that we have a selection when creating blocks to use "interface" translation, so I tried creating one using "content" translation. I have therefore tried using both methods.
Lastly, even if a language is selected when the block is created, it does not show this field when editing the block.
Comment #39
Schnitzel CreditAttribution: Schnitzel commentedthis should be fixed in the newest version of the Configuration Translation module: http://drupal.org/project/config_translation
Comment #40
Schnitzel CreditAttribution: Schnitzel commentedfull patch from the contrib module
Comment #41
Schnitzel CreditAttribution: Schnitzel commentedHere an update what has been achieved since the the last patch:
Comment #42
falcon03 CreditAttribution: falcon03 commentedRe-adding the "needs accessibility review" tag as a reminder to give a final review as soon as possible, probably within next week.
Comment #43
falcon03 CreditAttribution: falcon03 commentedComment #43.0
falcon03 CreditAttribution: falcon03 commentedUpdated issue summary.
Comment #44
Gábor HojtsyAdded this suggested commit message to the summary:
Issue #1952394 by vijaycs85, Gábor Hojtsy, Schnitzel, falcon03, YesCT, Ryan Weal, dagmita, likin, toddtomlinson, nonsie, Kristen Pol: Add configuration translation user interface module.
Comment #44.0
Gábor Hojtsyadd suggested commit meesage
Comment #44.1
Gábor HojtsyUpdate list of people in suggested commit.
Comment #45
vijaycs85Re-roll with below updates:
Comment #47
vijaycs85Created a major issue that's causing test fails #2019121: All configuration translation pages throwing FATAL error
Comment #48
Gábor Hojtsy(Side note: I think I properly updated the list of people who contributed in the suggested commit message, now: "vijaycs85, Gábor Hojtsy, Schnitzel, falcon03, YesCT, Ryan Weal, dagmita, likin, toddtomlinson, nonsie, Kristen Pol, robertdbailey").
Let's fix those remaining issues!
Comment #49
vijaycs85Re-rolling with updates:
Comment #49.0
vijaycs85Update suggested commit message.
Comment #50
vijaycs85Re-rolling with updates:
Comment #50.0
vijaycs85Updated issue summary.
Comment #51
bojanz CreditAttribution: bojanz commented:)
Comment #51.0
(not verified) CreditAttribution: commentedUpdated issue summary with #50
Comment #56
Gábor HojtsyAdded @helenkim due to #2026875: Remove accidental badaboom. Also sent for retest, because the RDFa fails are surely not related.
Comment #56.0
Gábor HojtsyAdded helenkim thanks to https://drupal.org/node/2026875
Comment #57
Gábor HojtsyAdded @kfritsche to list of commit mentions due to amazing config_translation test work. Actual code changes will come in a later compound patch.
Comment #58
Kristen PolAll green... what's left to do? Need more testing?
Comment #59
Gábor HojtsyThere are issues at https://drupal.org/project/issues/config_translation that are still to be done. Most of them.
Comment #59.0
Gábor HojtsyAdding @kfritsche based on amazing config_translation test work.
Comment #60
Gábor HojtsyAdded David Hernández to the list of people on the issue credit list thanks to contributions in several config_translation issues.
Comment #60.0
Gábor HojtsyAdd David Hernández due to several issues in the contrib module.
Comment #61
Gábor HojtsyAdding dawehner thanks to his fix provided in #2019831: people and roles page broken .
Comment #62
catchThis will conflict with the $arg patch if it gets in. Since the second argument isn't used, it could be left off - then if that patch does go in this won't break and vice versa.
tthree.
This should be a twig template straight off - it's not performance sensitive and there's still the goal to remove all theme functions by release - that goal might not get met but let's not add any new ones.
Comment #62.0
catchAdding dawehner thanks to his fix provided in https://drupal.org/node/2019831
Comment #62.1
Gábor HojtsyAdd lazysoundsystem thanks to https://drupal.org/node/2028127
Comment #62.2
Gábor HojtsyAdd juanolalla, R.Hendel to the credit list due to their recent contributions.
Comment #63
YesCT CreditAttribution: YesCT commentedSeveral new issues to clean up @todo's in the code and address @catch's comments from #62 have been created.
There is something for everyone, from novice to challenging, docs to twig. New folks welcome. :)
https://drupal.org/project/issues/config_translation
Comment #64
vijaycs85.
Comment #68
vijaycs85Re-rolling with updates:
Comment #68.0
vijaycs85Updated issue summary to highlight core blockers to getting this in. -y
Comment #69
vijaycs85Comment #70
Kristen PolI applied the patch. Should the translate tab show up if there is only one language? It does currently.
Comment #71
Gábor HojtsyIf your single language is not English, yes. The shipped config like menus, content types, etc. should be in English shipped with Drupal core, so you want to translate that to your non-English. If you only have English then no translate tabs should show.
Comment #72
Kristen PolWhen I translate the site information, I only see a subset of the form fields (site name and slogan). Shouldn't I be able to translate all fields?
English site information
Greek site information translation
Comment #73
Kristen PolRe #71, I only had English and the translate tab showed up.
Comment #74
Gábor Hojtsy@Kristen: well, those are the only *translatable* fields on the form. We don't do *language specific values* we do *translation* only.
Comment #75
penyaskito@Gábor: Would be any way for doing *language specific values* in contrib? IMHO this is a useful feature. Sorry if this is offtopic.
Comment #76
Gábor HojtsyI'm not sure we can target multilingual values as well if we want to see this land in the coming month or so, which is theoretically as much as we have. That would need extension of the schema API for things that are not translatable but multilingual and UI integration for that too (but no locale integration). It may or may not be a huge thing. If someone wants to run with it, sure, but I think we have enough to concentrate here to resolve still.
Comment #77
Kristen PolThanks for the explanation. Bummer that it would be hard to get it in but understandable!
The translation worked fine for me for those two fields. Is there some specific/other testing that is needed?
Comment #78
plachAs Gabor was saying, we cannot afford even a single critical, hence ideally we should test any use case reproducible with D8 core at least. I think a good test would be building a relatively complex site (with just D8 core) and try to translate views, (non-custom) blocks, fields, content types, vocabularies, image styles, contact categories, language names. Basically test any module that has a
.schema.yml
file in itsconfig/schema
directory :)Comment #79
Kristen Pol@plach - Cool! Thanks for the info on the schema.yml as being the key identifier for testing but I find a lot... perhaps we can identify if any of the following *don't* need testing?
Comment #80
Gábor Hojtsy@Kristen: the driving criteria is that:
- configuration with 'label' and 'text' type elements should have translation screens (not all have, but that is not major, integrations are not full yet)
- those translation screens should work and save translations
- those translations should appear on the site when using the config (eg. menu, vocabulary, etc)
- those translations should not show up in the admin form for the config when editing the original value (this may be a problem for some of those config)
So any schema.yml files with "type: label" and "type: text" are relevant for us.
Comment #81
vijaycs85@Kristen Pol, here is the summary of #79 and #80
type: label
ortype: text
b. CASE 2: UI exist, but no translation - we can add an issue in config_translation. config_translation_config_translation_group_info has the list of config page/entity that are added atm.
c. CASE 3: UI exist and translation tab available - We need to make sure we got all fields displaying proper. In case schema and config aren't sync, need to raise a issue with core.
Comment #82
Kristen PolThese have
type: label
And these have
type: text
Comment #83
vijaycs85Thanks again for the list @Kristen Pol. Here is the categories version of #82 as per #81
CASE 1:
CASE 2:
a. system.rss
Created an issue #2044387: Add remaining configuration entity or page into configuration translation module for all in CASE 2.
CASE 3 - Pass:
a. system.site - admin/config/system/site-information/translate
b. system.maintenance - admin/config/development/maintenance/translate
c. menu.menu.* - admin/structure/menu/manage/admin/translate
a. user.settings - admin/config/people/accounts/translate
b. user.mail - admin/config/people/accounts/translate
c. user.role.* - admin/people/roles/manage/anonymous/translate
views.area.schema.yml
views.data_types.schema.yml
views.field.schema.yml
views.filter.schema.yml
views.pager.schema.yml
views.schema.yml
views.sort_expose.schema.yml
views.style.schema.yml
CASE 3 - Not working:
Created an issue #2044389: [META] Fix broken configuration translation pages for all in CASE 3 - Not working.
Comment #84
Kristen Pol@vijaycs85 - thanks for testing and opening the other issues :)
Comment #85
Gábor HojtsyIssues in core that make this buggy or incomplete in places:
- Blocks translations 403: #2035839: Align block config schema with block configuration
- Picture mappings cannot get sub-operations: #2044865: Picture mappings cannot be edited
- Incorrect URL for language translation op in operations: #2044825: Language entity missing uri() method implementation
- Not possible to put translation op on image styles: #1809376: Use EntityListController for image styles
- Date formats need schema, so can be translated: #2038285: Update configuration schema for date formats as entities
- Node types need schema so can be supported: #2029405: Write configuration schema for node types
Comment #85.0
Gábor HojtsyUpdated issue summary with #68
Comment #86
Gábor HojtsyOne more identified for fields:
- #2045043: Field listings operations cannot be altered
Comment #86.0
Gábor Hojtsyupdated testing. need to update blockers still.
Comment #87
Gábor HojtsyNeeds a reroll based on all the changes that went to the contrib module.
Comment #88
vijaycs85Re-rolling with updates:
Comment #88.0
vijaycs85Add EllaTheHarpy thanks to work in https://drupal.org/node/2029407
Comment #90
vijaycs85Seems the patch in #88 got wrong files. updating with a path fix from #2065309: Custom block types changed type listing path in core
Comment #91
pwolanin CreditAttribution: pwolanin commentedshouldn't go into core adding a hook_menu implemetaiton
Comment #92
Gábor Hojtsy@pwolanin: yeah, well, how could we do this without hook_menu()? This is what @dawerhner said in #2044737-5: Provide local tasks as plugins:
So looks like the new core features are not yet capable. I don't think that should delay this issue, should it? Views has the same problem in #2032309: Use local tasks derivatives to provide local tasks for views.
Comment #93
vijaycs85Updating #2045077: Field translatability not wired up on the UI
Comment #94
Gábor HojtsyI think it is time for some serious over-arching reviews here as well!
1. As pointed out by @pwolanin, hook_menu() is still pretty much in this patch and it has outside dependencies (that are also applicable to Views, et. al.). I don't think that should legitimately hold this up. See #92 above.
2. Apart from that this module introduces **one new hook** to map UI paths to sets of config files (or entities). This may be better suited for a plugin system. Anybody agrees? Anybody want to work on that?
Apart of these two things, I don't believe there are any significant outstanding issues with this module, so if you think there are, please step up ASAP and say so :) I believe it covers all core config entities. We know about #1741498: Add a responsive preview bar to Drupal core, and if that lands before this one, it needs coverage too. But all core entities and config files with translatable content are covered I believe. Committing this sooner than later would also be helpful to fix #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed (major bug) in a more holistic way.
There are outstanding configuration schema issues to cover all parts of configuration, but they are outstanding either way, so I don't think there is any pressure on this issue to complete them all before this can land. The proposed patch does include tests to ensure all config files and config entities in core covered actually have a working translation screen which implicitly also tests that there is at least high level configuration schema coverage for *all of them*.
Now bring out your feedback!
Comment #95
KartagisHere are the steps I've taken and results I got:
1- Installed the site in English.
2- Added two languages -- Spanish and Turkish.
3- Browsing to /admin/config/system/site-information/translate, I've translated the site name and slogan to those two languages.
4- Adding the /es and /tr, I've confirmed that site name and slogan get translated.
Screenshots:
Attached screenshots.
Feedback: It would be nice if language names weren't all lowercase. I can submit a patch for that. Am I allowed to?
Regards,
Comment #96
Gábor Hojtsy@Kartagis: thanks for that patch in #2071169: Language names all lowercase, its now in, and will get here in the next reroll of the core patch :)
Comment #96.0
Gábor HojtsyUpdated issue summary with #88
Comment #97
KartagisAdding me as author would be nice :)
Comment #98
Gábor Hojtsy@Kartagis: lol, that already happened earlier. See https://drupal.org/node/1952394/revisions/view/2796591/2812879 :) Looking forward to working even more with you on this :)
Comment #99
KartagisSorry I meant --author for the patch on #2071169: Language names all lowercase :)
Comment #100
YesCT CreditAttribution: YesCT commented@Kartagis ah. authorship is ... strange because it is automatically the committer, but the commit credit goes to the usernames in the log message. Yours was http://drupalcode.org/project/config_translation.git/commit/d7a3cc9 (Doc: https://drupal.org/node/52287)
Also, maybe related to your question... #2042697: Add historical issue credits to Drupal.org user profile
Comment #101
purabdk CreditAttribution: purabdk commentedI installed the config_translation module. I checked the functionality. I manually reviewed the code and functionality.
Functionality is working fine.
Here are some points:
config_translation.api.php - line no 10 - why we kept the @ and curly bracket
config_translation/ConfigGroupMapper.php - line no 253 - why returns the method null, I am not sure about this.
Controller/ConfigTranslationController.php - line no 44 - $langcode = $request->query->get('langcode');
$langcode is unused variable.
Form/ConfigTranslationManageForm.php - line 109 - validation method is not doing anything.
TODO are still in patch file, need to work on that.
Comment #102
Gábor HojtsyI've opened #2083615: Use links annotation for config entity URI like for content entities which would help a great deal to not need to redefine the config entity URL patterns. That would make those available via the config entity info system, so we can automatically grab those, similar to how we do that for config prefix now. Would improve the DX a great deal not just in core itself but for config_translation. Again, another great synergy of this being developed. Support welcome there!
Comment #103
Gábor HojtsyThis notation is used to add a list of functions (in this case documentation functions) to a group. This is normal.
the getType method has been removed from this mapper in recent updates
- this has been recently removed from the contrib copy of the module, patch update coming
- the FormInterface has this method, so we cannot leave it out; however since we are saving translations where we don't know anything about specific required patterns or structures (beyond number, text, etc. fields that the form validation system already ensures), so this needs to stay
Rerolled patch coming with latest changes.
Comment #104
vijaycs85Re-rolling with updates:
Comment #104.0
vijaycs85Add Kartagis for testing, and a couple patches
Comment #108
Gábor Hojtsy@purabkharat: also re the @todo's, I just removed two outdated ones from the code. There are 893 @todo comments in Drupal 8 core as of this point. So I'm not concerned that adding 3 more will be an issue. These are not critical, more nice to haves.
@all: we can simplify the config entity mapping stuff a great deal if we can automate path generation and simplify/automate tab generation. The following two core issues would help *A WHOLE LOT* with improving the DX of integrating with this module, so help on those are very welcome *now*:
#2083615: Use links annotation for config entity URI like for content entities - so we can retrieve the URLs for config entities without needing to duplicate it
#2085907: Ensure all configuration entities have an Edit/Configure tab by default - so we can just add a tab on top of config entity paths and don't need to worry if there was or was not a default edit tab
Comment #109
vijaycs85Re-rolling with new changes (below):
Comment #109.0
vijaycs85Updated issue summary for #104
Comment #109.1
vijaycs85Updated issue summary for #109
Comment #112
YesCT CreditAttribution: YesCT commentedWe planned how to actually get this into core and identified concerns, agreements, strategies and blockers.
Notes: https://docs.google.com/document/d/1iLuLkalLI-sr12sxFOLeEOf-yyIn5ConHHcF...
The config_translation issue queue has the issues added we identified, and core issues also created (and tagged with d8mi and blocker).
So we know this needs work, but also still needs reviews. Leaving at needs review.
Comment #112.0
YesCT CreditAttribution: YesCT commentedAdd webflo and tstoeckler for lots of recent work on config_translation.
Comment #112.1
Gábor HojtsyAdd penyaskito for recent contributions to the module :)
Comment #113
vijaycs85Re-rolling with new changes (below):
Comment #113.0
vijaycs85Add EclipseGC for plugin help, reviews, suggested things.
Comment #113.1
alexpottUpdates from #113
Comment #115
vijaycs85Re-rolling with new changes (below):
Comment #115.0
vijaycs85added core blockers from the prague google doc
Comment #116
larowlanCode review only, didn't test yet but very excited by the possibilities this patch presents.
Is this a plugin candidate? Not many info hooks left in core these days. Edit: ah I see it is using info discovery for dynamic and yaml for static, disregard.
can we add 'use Drupal\Core\Entity\EntityInterface' and type-hint the short-form instead?
Observation: stands out like a sore thumb in an otherwise unit-testable class :(
I don't see any phpunit tests for this class.
nitpick: definitions?
this sentence doesn't read very clearly, one to too many?
FALSE is returned here even if the entity is already set? That seems confusing.
Can we inject the string translation service? That would mean we can get 100% unit test coverage of this class.
What happens if $this->entity isn't set? Should we check first and throw an Exception?
nitpick: Returns the name?
nitpick:proper sentence structure
>80 chars
There are no unit tests for this class either?
nitpick: missing @throws
Can this be injected? (apologies if this has been asked before)
observation: ouch
$this->t
nitpick: missing @throws
?
There is a plan to require an explicit parent entry rather than figure it out automatically by path.
Can we get an @todo to revisit this when that is in. I assume that means we can clean this up significantly? We had logic like this in forum module that we've since killed but I think this sort of stuff still lives on in taxonomy.
Also #2112711: Provide an easier mechanism for using drupal_get_form() directly makes a huge difference here, so perhaps a todo for cleaning that up here too.
If so should we add #access => FALSE?
Can we $this->t here?
In other places we're injecting the field.info service, here we're using the static method on Field, I think this approach limits unit-testability as it requires the container. It also means the dependency is hidden (not visible in the constructor).
$this->t?
nitpick:missing @throws?
What happens if contrib wants a different list controller that has different arguments? Should we have a factory interface/method pair here?
Doesn't appear to be used other than to store $this->mappers? Is there a need for the mapperManager property?
$this->t
lovely!
Same comment about statics as for FieldInfo? - although calls to drupal_set_message so unit-testing use-case redundant
?
is there an ElementInterface? Is it even needed?
love learning new things from reading patches! thanks
slick!
$this->t?
Should we have unit tests for this class?
I'm seeing 'id' instead of machine name after the recent changes to HEAD?
missing scope modifier public|protected?
pretty sure we're not supposed to use $this->container in tests anymore see #2087751: [policy, no patch] Stop using $this->container in web tests (not pointing out all of the uses - there are a few in this patch)
I think we're putting @group Drupal too see #2057905: [policy, no patch] Discuss the standards for phpunit based tests
phpunit++
Comment #117
Gábor Hojtsy@larowlan: thanks for the deep review. Some of your points are already fixed in our dev branch (eg. lack of scoping for test methods, block ID changes, etc. - there have been 4 commits before the above patch was rolled, things move fast https://drupal.org/node/1931812/commits :D), some others are resolved in #2113283: Small clean-ups all around... or to be resolved in other sub-issues already. Many are new though. We'll need to review your comments one by one to ensure all is taken care of as possible.
Comment #117.0
Gábor HojtsyUpdated issue summary for #115
Comment #118
Gábor HojtsyTagging as one of the prague hard problems that we discussed :)
Comment #118.0
Gábor HojtsyPromoted tstoeckler and webflo in the list of people based on their amazing work on the code, modernizing to D8, putting in the list UI, etc. Also added timplunkett for his work on the tabs code along with dawehner.
Comment #119
Gábor Hojtsy@larowlan: All right! You've got feedback for your feedback! All the items where I wrote "Fixed." are actually fixed in #2114439: Fixes for the big larowlan review, that is not yet committed as fixes but the patch is posted, so being tracked. (There are some items already fixed on 8.x as well as indicated). Still many to talk about / look into.
1. You figured this out :)
2. Fixed.
3. To discuss. language_load() does not have an OOP alternative/equivalent that I know of, if you look at https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi... and language_list() consequently, there are lots of mangling done there on top of the config entities (the duality of config entity + runtime language classes exist to serve the config system depending on language); Suggestions welcome. Not that there is a LanguageManager in core which is *not* about managing language objects, it is for language negotiation (also being reworked in #1862202: Objectify the language system)
4. To discuss. Is it a requirement now that classes come with unit tests? :) We did not include lots of unit tests so far, however we have a whole lot of web tests to ensure integration and all the intricate details of how the mappers integrate with routing and form generation.
5. Fixed.
6. Fixed.
7. Debated. The return value tells you whether the action you wanted to do (set with the passed entity) worked. If there already was an entity, your action DID NOT work, so you get FALSE. IMHO the one-line comment properly summarizes that?
8. Fixed.
9. To discuss. Sounds to me like almost every method would need to throw that exception then? I mean most methods use the entity type, id, etc. Is that your suggestion?
10. Fixed.
11. Fixed.
12. Fixed.
13. To discuss. See above. Uses language_load() as well.
14. Fixed.
15. Todo. This is in the realm of #2104925: Convert to work with routes instead of paths in mappers, where these lines will be removed altogether. We may need to do lookups the other way though.
16. Debated. Yeah, ouch, but core does not provide anything better.
17. Fixed.
18. Todo. This is being handled in #2113283: Small clean-ups all around... already.
19. Discuss. Inline docs on the type returned. I think @tstoeckler found some standard to follow here :) Debated?
20. Discuss: The problem is we cannot register each form on its own route, because menu_router() table only allows for items nested to a certain level. If this is not anymore a problem, we can register them as individual routes (ie. add 3 levels of depth to any path we attach to instead of the +1 level we have now with the translate tab taking arguments for langcode and action). I'm not sure any of the issues you linked are related? Did you understand the same problem that I re-explained here? :)
21. Discuss. Yeah, we should. Do you know of a good way to check access to that route/path in the curent system?
22. Fixed.
23. Todo. Indeed.
24. Fixed.
25. Todo. This is being handled in #2113283: Small clean-ups all around... already.
26. Discuss. I don't know :D
27. Todo. This is being handled in #2113283: Small clean-ups all around... already. (really)
28. Fixed.
29. :) More routeness is to come out of #2104925: Convert to work with routes instead of paths in mappers.
30. Todo.
31. Discuss. Same question as 19.
32. Discuss. I'd pass this to @webflo, if he would be around :)
33. :)
34. :)
35. Todo. DerivativeBase does not have t on it :/ We'll need to inject ourselves.
36. Discuss. Same as 4.
37. Fixed (already in 8.x of the module).
38. Fixed (already in 8.x of the module).
39. Discuss. Sounds like this is still discussed, not decided. So not sure if we should move anywhere on this. Core did not decide yet either.
40. Discuss. Same as 39. I'd not follow experimental standards that may or may not be in effect later on. Following what core does is our best target, then conversions later are easier. If we are picking up on forming things, then conversions / updates are all the more painful.
41. :)
Comment #119.0
Gábor HojtsyUpdating issue summary removing core tasks that are done and cleaning up steps to test.
Comment #119.1
YesCT CreditAttribution: YesCT commentedjust nice formatting issue/comment link
Comment #120
larowlan3) Yeah not related to this issue, just saying 'ouch'
4) The class is loosely coupled and a good candidate for unit tests. Either they go in here or we have a follow-up. I know of at least two devs who are keen to write unit tests/get exposure to phpunit so a follow-up is entirely appropriate - so we should file it and mark postponed on this
7) Yes it does, but its weird - just saying
9) My understanding of this is that $this->entity can be NULL so calling ->id() on it would cause a FATAL
13) see 4
15) ok
18 ) ok
19) never seen it before, is all
20) i misunderstood, but this is certainly a different pattern, would be interested in someone like @timplunketts' input
21) See the PathBasedBreadcrumbBuilder, it has something like that, menu_item_route_access is a procedural equiv
35) I think timplunkett just added an issue to add it
36) see 4
39) just pointing out what @alexpott will if he reviews :)
40) ditto 39 but @dawehner
Comment #121
tstoecklerThanks larowlan, very much appreciated! Some comments from my side.
Regarding 19: I added that because PhpStorm then allows autocompleting on inline variables for which the return values cannot be type-hinted (This is the case for generic methods such as createInstance() and such). Since even without an IDE it can only help, I think, it makes sense to add. Also note that we've been adding these to core elsewhere as well, not very widespread, but some. @dawehner likes it just as much as I do :-)
Regarding 26: I already opened #2113707: Create a ConfigTranslationListControllerInterface a couple days ago. My answer would be "yes, there should be" :-)
Regarding 32: Added #2114931: Create a FormElementInterface
Comment #122
Gábor Hojtsy@larowlan:
Resolved 21 by #2117015: Check access to edit link with current route access checking.
Resolved 35: Added a $this->t() implementation to the local task class in #2114439-8: Fixes for the big larowlan review, which would need to be removed once/if #2112575: Add a DerivativeBase in the Core namespace with t() as a helper method lands.
Comment #123
vijaycs85Re-rolling with below changes:
Comment #123.0
vijaycs85clarifying that there are no core blockers anymore.
Comment #124
tim.plunkettReviewing.
Comment #125
tim.plunkettFirst of all, the following tests were misnamed (Ui vs UI), so they haven't been running:
ConfigTranslationListUITest.php
ConfigTranslationUITest.php
ConfigTranslationViewListUITest.php
Check the last patch test results, you won't see them in there.
The patch I'm uploading renames them, but at least one of them fails.
If you're still running a sandbox you'll have to do some tricks to get them renamed with git (you can't do it easily on a mac).
----
Also, there are PHPUnit tests added, but with only 34% coverage. It shouldn't be too hard to boost that up.
This is a code only review, I saw a couple odd things while running the tests that I will call out during a UI review later.
----
I've also made a lot of nitpicky fixes as I went, because Dreditor was unstable with such a large patch.
The stuff I wasn't sure about I left a comment below.
We need a comment why we pass an empty array through. Also, we could have just as easily done
return parent::createInstance($plugin_id, array());
Why are we creating these each time, outside of __construct, and not storing them as $this->discovery?
Then we wouldn't have to duplicate this from DefaultPluginManager.
Here an elsewhere, we should either use String::format(), or sprintf()
I don't understand what is being passed through. It seems to be an Element always, except then it is foreached() through and then it is still an Element? The getDefinition() calls and all don't seem to match up. And I don't follow how it could ever be TypedConfigManager.
Comment #126
tim.plunkettNevermind what I said about the UI being weird, I just made a mistake in my patch :)
EDIT:
Grrr, this is another mistake on my part. Should be
$entity->get('bundle');
.When the UI/Ui tests fail, someone please make that quick fix?
Comment #127
dawehnerAmazing work! Just some minor comments.
Afaik we use a separated css folder
Nitpick: missing space.
It is a bit confusing that we use plugin.manager.config_translation but the classname is ConfigMapperManager
<3
This two interformation disagree.
Is there a reason why the test methods do not have a visibility?
<3 <3 <3
Comment #128
tim.plunkettWell that's a problem.
First of all, ConfigTranslationListUiTest fails for me locally.
Secondly, ConfigTranslationOverviewTest doesn't test enough of ConfigTranslationFieldInstanceListController, we need a time when displayBundle() is true. So, the test needs to set up a field instance, and check the page for that (like admin/config/regional/config-translation/comment_fields) or something).
Anyway, needs work for #125/126/127.
Comment #129
tim.plunkettI moved my fixes to #2123575: Fixes for tim.plunkett and dawehner review and also addressed @dawehner's.
This still needs tests for:
the config overview test of a field instance
finishing the PHPUnit tests
And needs work for the rest of my concerns in 125.
Comment #130
Gábor HojtsyI committed our fixes in #2123575: Fixes for tim.plunkett and dawehner review.
Re general comments in #125:
- We actually have the files with the right names in the contrib repo at http://drupal.org/project/config_translation, so we *do* have those tests running all the time. The fail was brand new and due to #2098089: Date formats cannot be translated (the core UIs are useless). I think @vijaycs85 does not have his git clone cleared out of the old files, so he is not getting the right filenames for rolling the core patch. The files are correct in the contrib repo, nothing to fix there.
- I'm not entirely sure what would be great to unit test. My feelings on the entity mapper unit test for example are a bit mixed. The class is so simple and the tests merely test minor data processing happening in the class. Probably useful to some degree, but not sure they are practical. I agree it would be great to improve the unit test coverage. Not sure about code coverage for core in terms of phpunit, but my sense is we may be in a very good place already :)
Review points in #125:
1. I'm not sure why we pass on an empty plugin definition (and then how are the instances created in the base class?). I'd ask @tstoeckler / @EclipseGC who were the masterminds behind this part of the code.
2/3. Yeah I think the only reason is we implemented config entity discovery in the findDefinitions() method, instead of putting it into our info hook implementation (where we discover the field_ui based mappers). I think we could just move the config entity definition discovery into the module info hook, use $this->discovery and then not need this method. What do you think?
4. Yup, String::format() makes sense.
5. Based on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21... get()/create(), it will always work with an Element, yup.
#127 has all been addressed in #2123575: Fixes for tim.plunkett and dawehner review. Posting an update there for these numbered concerns.
Comment #131
vijaycs85Ok, updating module with existing patch caused that duplicate files in #123. Now adding it as new module with #2123575: Fixes for tim.plunkett and dawehner review( the inter-diff is just changes from #2123575: Fixes for tim.plunkett and dawehner review)
Comment #131.0
vijaycs85Updated issue summary - from #123
Comment #132
Gábor HojtsyOk I found the answer to #125/1 (same as #130/1). So the config mapper manager has this code:
Note that it passes on the plugin definition to the createInstance(). Then however the createInstance() totally ignores that and passes on an empty array. If you look at the createInstance on the factory (https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Plugin%21...), then you can see this array is not supposed to be the plugin definition but supposed to be the plugin *configuration*. The definition is retrieved here (again) from the definitions. The configuration (which in our case is an empty array) is passed on to the static create method. Finally the ConfigNamesMapper has a static create method that at that point properly takes that argument as $configuration, but does not use it. Which is right after all, since we don't have plugin configuration for these plugins, we only have definitions for them.
So the right fix would be to:
a) not pass the definition in getMappers() and
b) remove our createInstance() method, since there is no use for it
c) keep ignoring the $configuration in the static create in ConfigNamesMapper()
So in short, we started using the definition as configuration but threw it away at several points anyway. Its a little bit of misguided code there :D Going to post an update with this fix in #2123575: Fixes for tim.plunkett and dawehner review
Comment #135
Gábor HojtsyThat should now fail on the access check interface change in core that already has a fix on the module :) Just needs a reroll from there :)
Comment #136
vijaycs85ok, Re-rolling with below commits from config_translation:
Comment #136.0
vijaycs85Adding sub-tasks from #131
Comment #138
vijaycs85#136: 1952394-config-translation-core-136.patch queued for re-testing.
Comment #139
Gábor HojtsyApparently the module is not yet fully fixed either for the core API change, no need to retest: https://qa.drupal.org/pifr/test/474493 Need more fixes in the module.
Comment #140.0
(not verified) CreditAttribution: commentedUpdated issue summary - from #136
Comment #141
Gábor HojtsyAlthough concerns from @timplunkett and @dawehner have been resolved, we are impeded to move further by a testbot issue blocking the tests on the contrib module we use to build this project out. It would be a huge waste to keep posting untested things here, so all help in resolving that would be welcome: #2125349: Testbot not testing contrib 8.x with up to date 8.x core?.
Comment #142
webchickIn the meantime, you could open one of those "ignore me just testing" issues ala Comments as Fields, CMI, etc. and upload your patches to that issue for testing.
Comment #143
Gábor Hojtsyhttps://qa.drupal.org/pifr/test/474493 passes green now :)
Comment #144
Gábor HojtsyRerolled with these commits from the module (some dummy looking commits were due to testing the testbot malfunction that was fixed yesterday).
Comment #145
Gábor HojtsySending to testbot and looking for human reviews!
Comment #146
Gábor HojtsyCommitting this would also let us remove 500+ lines of confused and conflicting one-off date format translation code from system module that crosses into the realm of locale.module pretty aggressively.
Comment #147
vijaycs85Comment #147
vijaycs85Comment #148
Gábor HojtsyI'd love to get this into the next alpha release on Nov 18th (Announcement at https://groups.drupal.org/node/364138). It would nicely coincide with DrupalCamp Vienna coming up a few days later (with lots of multilingual Drupal 8 content :)
Please unleash your reviews and things you consider are commit blockers. I think we have been pretty diligent in working through those.
In related news, I need phpunit/PHP version experts on #2123897: Restore theme .yml support, add/fix tests to help figure out why it passes for timplunkett, dawehner and myself but not on testbot :/
Comment #149
Gábor HojtsyDrupal people! Any feedback? Does this all look good now? :)
Comment #150
Gábor HojtsyAlso new patch for your core passing enjoyment! :) Following commits included:
Comment #151
Gábor HojtsyComment #152
Gábor HojtsyRestore tags (hopefully).
Comment #153
Gábor HojtsyHere is a (I think final) update to get this to as nice as any other core module (or I think actually nicer :D). This includes making contextual link and tab dynamic titles work cleanly instead of a hack, cleaning up all @todo comments and using routes for different operations instead of GET arguments that we were forced to do before due to hook_menu() path component limitations. Also some other minor cleanups.
All of our remaining @todo elements are dependent on core. We could wait around for those to get resolved (we are active on most of them, not just standing by), but core modules are also inbetween different APIs and have plenty @todos to convert to new ones as they emerge. This is not less true to config translation.
The patch now also includes a MAINTAINERS.txt entry (which should be mandatory with any new module :) and splits the *.config_translation.yml files to the right core modules as appropriate. These were not in earlier patches and obviously only apply to the core patch not to the contrib project that we use to build the code.
Comment #154
Gábor HojtsyComment #155
Gábor HojtsyComment #156
Gábor HojtsyAttempting to restore lost elements on the summary that did not even show up earlier....
Comment #157
Gábor HojtsyComment #158
Gábor HojtsyComment #159
Gábor HojtsyComment #160
Gábor HojtsyComment #161
Gábor HojtsyComment #162
Gábor HojtsyUpdate again only showed up in node revisions but not in main node :/
Comment #163
Gábor HojtsyComment #164
klonosComment #165
Gábor Hojtsy#2135101: Expand test coverage on configuration translation tabs turned out to be a core problem, namely #2135787: Move static config entity local tasks to local_tasks.yml and #2111823: Convert field_ui / Entity local tasks to YAML definitions. Even without those in core, the config translation module works and there is nothing we can do in the module to fix those core lack of conversions. The module provides various entry points to the translation pages as shown and only some of one of them are affected, so I don't consider this a commit blocker.
Made sure the issue summary is fully up to date and concerns from the above reviews have been resolved. We cannot just sit around and wait for everything in core to snap into place perfectly. We kept marching on finding bugs in core and will do so in the future. I'm confident we will uncover more bugs with core thanks to this module being in. It will be better for core in the end as now finally there is a module integrating some APIs that none other use in core or where there was minimal use before.
Very excited!
Comment #166
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThis really deserves to get committed, preferably before the release of the next alpha. I've tested it and it feels (and works) great. Great work on this and a good example that perseverance is key!
Comment #167
Gábor HojtsyComment #168
Gábor HojtsyComment #169
tim.plunkettAs pointed out in #2102777-80: Allow theme_links to use routes as well as href by @Gábor, it would be very good to get that in before this.
In addition, #2133469: Replace path-based entity links with route names is a follow-up to the original 'links' code, which was used by content_translation even more after #1810350: Remove menu_* entity keys and replace them with entity links.
I was planning on RTBCing this myself once those two were in and this was rerolled, but now that it is RTBC I would like the committers to postponing it on those two issues (which are RTBC themselves!)
Comment #170
Gábor HojtsyAgreed with Tim, if at all we can do all this before the alpha :) We'll try to do our best with some committer help :)
Comment #171
Gábor HojtsyWorking on updates based on those two core patches landing.
Comment #172
tim.plunkettComment #173
tim.plunkettComment #174
Gábor HojtsyUpdated the patch so it considers all recent changes on paths vs. routes. Also improved DX on the default definitions of config translation name mappers to follow the pattern of local tasks, etc. where the name of the plugin === the route name. Also added tstoeckler and vijaycs85 both of whom requested to be added as a maintainers. :)
Once again I'm sure this module is not 100% perfect and there will be core changes which will need to be reflected and adjusted. There will also be more core bugs found through this module :) We can likely continue tinkering with the module in contrib but its likely as good as any core module at this point (IMHO better :) and people will benefit from seeing the complete picture, trying this out, finding more of the bugs (in this module and elsewhere in core thanks to this module). So I think its time for this to get in. :)
Changes in this patch:
Comment #175
tim.plunkettI'm posting the interdiff from gabor's patch (interdiff.txt), as well as some additional code changes I added (changes.txt).
The hook_config_translation_info() hook makes me shudder in fear. It's copied 1:1 from the config_translation_config_translation_info() implementation, but it does a lot of different things and uses the route provider in a scary way.
Comment #176
tstoecklerAwesome! I did not think of doing it this way. Makes perfect sense.
The rest of the changes.txt looks good as well.
Will open an issue to let the testbot have a go at your patch and then get it in.
It's still on the table that that hook will go away completely. That would be part of #2095291: Remove *.config_translation.yml for config forms, use form getConfigNames() . I don't disagree, though, that more beautiful code has been written. It's just the best we can do with the current implementation, I think.
Comment #177
tstoecklerActually, I realized opening an issue in the contrib queue for that is pointless at this point. Your changes look good, so this is RTBC.
Comment #178
Gábor HojtsyDiscussed with tim.plunkett in IRC the docs we can improve alongside improving the implementation itself which needs more core issues to be opened (for field_ui to be reworked in how it defines its routes). So only looking for review on his changes.txt :)
Comment #179
Gábor HojtsyOh, already done by tstoeckler, heh, thanks!
Comment #180
tstoecklerI now realize #176 was not about the actual implementation, but about the hook documentation. Will try to cook up a better example, but that should not block this, IMO.
Comment #181
tstoecklerHere's a stab at improving the insides of hook_config_translation_info(). I'm not providing a full patch, but only the interdiff, as the former would have required this to go back to "needs review", while I still don't think that the simplicity of the example code (while certainly an issue) should hold up the commit of this patch. If someone likes the interdiff, I'll roll a full patch.
Comment #183
tstoecklerSorry, I'm an idiot for not naming my interdiff correctly. #175 is still RTBC.
Comment #184
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #185
Gábor HojtsyNote for anyone testing it, you will most likely hit the "bug" that not all config entities have a proper translation tab. That is due to #2135787: Move static config entity local tasks to local_tasks.yml and #2111823: Convert field_ui / Entity local tasks to YAML definitions which are both core *lack of conversions* of old tabs to new tabs. The old tabs are not added to the new tab system, so we cannot extend them. There is *nothing* to change in this module, as the new tab conversions happen, the translation tabs will automatically show up. Unfortunately those two issues can only be fixed at the same time, again because they convert the old tabs to new tabs for the same pages in different modules.
However although not all config entities have translation tabs, they do have several other entry points, eg. operations buttons in list controllers and the config translation index itself. So this is not something this patch could change/fix in any way, it is dependent on core conversions that did not happen yet.
There are several other core issues which would lead to improvements in this module, they are linked in in @todo notes in the patch. I'm sure we'll find even more like that as time goes on. The point once again is the module is already very solid, and tinkering with it in contrib will not get us the benefits of it being in core (even more eyes on it, crowdsourcing finding more bugs, help resolve config context related issues as we can experiment with translations, etc :)
That's it for my little speech :) Looking forward to have this in core for the next alpha!
Comment #186
klonos...perhaps it would help if that little speech was added in the issue summary (this issue is 186 comments long)?
Comment #187
Gábor Hojtsy@klonos: most of it is already in the "Remaining tasks" section. I'm moving the explanation of the two issues under steps to test where people may encounter the "bug". See changes :)
Comment #188
Gábor HojtsyHide bugos docs patch... Hopefully less confusing for a core committer now.
Comment #189
tstoecklerComment #190
tstoecklerComment #191
webchickReviewing this now! Stay tuned. :)
Comment #192
webchickHere's how far I got. Will need to resume in a couple of hours (today is family day):
As per usual with UX issues, I went in blind without reading the issue summary, so I could try and be as neutral as possible.
1. First, went to the modules page and turned on Config Translation. This also turned on Language and Interface translation modules. (File and Field were already on. Curious that those are dependencies.)
2. Went to admin/structure/views, clicked "Translate" under operations.
Only thing I can do from admin/structure/views/view/content/translate is edit the English translation. Right. I need to set up a language first. Always forget to do that when I test multilingual patches. (Help page didn't mention this step either.)
@TODO: (Separate issue) Maybe show a class=warning DSM that they only have one language enabled on screens like admin/structure/views/view/content/translate that links off to the languages admin page so I don't get blocked.
3. Now I'm at admin/config/regional/language and add a language (Japanese, to see how it deals with RTL).
@TODO: (Separate issue) I'd love any kind of visibility (textual description of the batches) as to what is happening during these 11 steps that take about a minute to run. e.g. "Fetching the translations from localize.drupal.org, unpacking them, whatever"
4. Back to Views translate. Add Japanese translation: admin/structure/views/view/content/translate/ja/add
@TODO: (Follow-up) We should maybe discuss what to do with this screen a bit more. At a glance, it looks like I can only translate two things: Administrative description and Human readable name. But there are literally hundreds of other things, getting to most of them requires clicking into the various collapsed field sets/details. And getting to each one requires click to expand, fill out a bit, click to expand, fill out a bit, etc. which makes what is already a daunting task feel even worse.
TL;DR: Should we auto-expand all of these fieldsets/details by default? Particularly if the English and XXX language versions still match?
5. Lost interest in doing this after a few mins, so just clicked "Save translation." Seemed to save ok.
TO BE CONTINUED. :)
Comment #193
Gábor Hojtsy@webchick: thanks!
Re. 1: The only dependency listed is locale (interface translation) which is the module which actually will pick up the translation files (there is no actual interaction with that module's API outside of that). The module *does not* depend on file or field, I don't know how did you get that, but there is nothing in the info.yml or otherwise to that.
Re 2: Whether one language lets you translate or not *depends*. Eg. if your site is Japanese only, you can already translate the built-in English views (you don't need to have English configured on the website, however the built-in views come in English anyway). That is the practical one language use case, which is/will be very typical. A DSM might actually help explain the situation where there is only one language configured and the original config is in that language. That case would require you to add a new language indeed to do something useful on this screen. (Note that we did consider putting an access check to net let you access this page if the config original language is the only language on the site but concluded it would be very confusing that you have translate operations on some views in a views list (or translate contextual dropdowns on some of your views) but not others. You would have no chance to figure this out without seeing that screen and making the connection about why. It is true that we did not end up helping you make that connection (with a DSM) once you arrive on this screen (yet) :)
Re 3: The batches are pre-existing to this issue. Separate problem indeed :)
Re 4: There is indeed an issue in the config translation queue to improve the views translation experience itself. Most (if not all) other configs are much less structured and have way less depth. The structure of the views page starts to get overwhelming after a while but where is the full depth of a view not overwhelming? :) There are for example dozens of labels at different levels, depending on whether you are in a display or field, etc. the label applies to something else. Translators need to know this context. The API was architected so it allows specific mapper classes to be used for specific things (eg. views) or if not specific mapper classes, their routes may be altered to provide different / optimised user experience for translation, etc. There are several entry points where the experience may be tailored for this most custom use case. It is in core a very unique complexity that other configurations don't have. If you look at content types, date formats or blocks, they are absolutely very simplistic.
Looking forward to more feedback :)
Comment #194
vijaycs85Thanks @webchick and @Gábor Hojtsy for the quick review & comments. just to add @Gábor Hojtsy's comment on#192.4,
Can we have views config schema as a separate custom module (views_config_ui) because:
1. As we can see except views all configuration translation pages are simple and clean.
2. Views is one complex configuration which we might want to split into multiple storage (may be in Drupal 9).
3. we can apply 3 strike rule. now we got views. once we have two more complex configuration (like views), we should look for better approach.
4. This will keep config_translation very generic and would allow as to provide views specific UI and theme (which can just simulate views UI page with in-place editor for labels).
Comment #195
tstoecklerAwesome review, thanks a lot!
A couple of comments from my side as well.
(Following @webchick's numbering not @vijaycs85's):
1. Interface translation (locale.module) does in fact depend on File module, due to the file handling it does with the translation po files, it depends on Field module, that's why all those show up under Config translation as well.
2. DSM++
4. I have also pondered about providing a separate, custom-tailored form for Views, simply because it is such a special-case. So, yeah ++.
Comment #196
webchickThanks for the feedback, all! Back again for round 2. I'll take a look at blocks after I'm done views to see how it compares.
TL;DR: It's awesome. :) If you're curious about my thought process as I was reviewing, feel free to read the innards, else skip to the bottom.
6. Went to the blocks page and added the language switcher block so I could change languages.
@TODO: (Separate issue) Not sure how a normal person would ever know to do this; I only know because I've tested other multilingual patches. Could we enable this block by default when more than one language is added? And/or possibly add another entry point for language switching at admin/config/regional/language, since I already found that page once? Hm.
7. Switch to Japanese, back to admin/structure/views. Awesome!! Now the "Content" view shows "Discontent" as the view name! (I was feeling punchy. ;))
@TODO? (Separate issue) Why did may toolbar orientation and views table columns, etc. not flip RTL once I switched to Japanese?
Ok, so the crazy use case works! Hooray! Let's try the easier one.
8. Still in Japanese, go to ja/admin/structure/block. Click "Translate" on the Language switcher block
@TODO (totally separate issue) Why is "Translate" the only thing I can do? Not delete/remove the block from the layout?
9. Add a Japanese translation.
10. Only thing I can enter is a new description for "Language switcher." Hm. I guess that makes sense? Everything else would be provided via the interface translation module?
11. Tried adding the "Recent comments" block instead, which AFAIK is not yet converted to Views and AFAIK has a "number of comments to show" setting. Turns out, it does not. But something happened and now all my Drupal-provided blocks have a dropdown in the operations column with "Configure" as the default. Cool. So I guess #8's @todo is some kind of bug then.
12. Tried clicking "Translate block" from the tab at the top of a particular block. That works, too. Yay.
13. Give up on blocks for now, cos I can't seem to find a block with additional configuration settings beyond just the admin description.
14. Go to Structure > Content types > Basic page > Translate (ja/admin/structure/types/manage/page/translate). Add a Japanese translation.
15. Yay! Things! This time I translate into 1337 5p34k.
16. Go to ja/node/add, b4s1c p4g3 is there. Yay! Click it and… Hm. t1tl3 is translated. Body, Text format, etc. aren't. That's kinda weird, but I suppose is the best we can do, given title isn't a field.
@Question: Is there an issue somewhere to re-make title a field, or is this the best we can do in D8?
17. Just for giggles, let's translate Body as well! admin/structure/types/manage/page/fields/node.page.body/translate add a JP translation.
18. Back to ja/node/add/page and now the field says b0d33. :)
Well, this is absolutely awesome. I tried kicking the tires on pretty much everything I can think of. Views, Blocks, Content types, and Fields all seemed to work very well. Things I found were minor UX issues or totally unrelated bugs. The code has already been reviewed (and worked on directly) by the best of the best, so I won't bother with that here. AWESOME work, everyone!!!! :D
Despite this being classified as a feature request, I feel this is fundamental to touting the multilingual features in the Drupal 8 release, so I'm happy to make an exception to thresholds in this case.
So it is with much happiness that…
Committed and pushed to 8.x. :D Thanks!!!
This'll be something super awesome to tout in alpha5!! :D
Comment #197
webchickd.o :(
Comment #198
larowlancongratulations to the d8mi team!
this goes at the top of 'this week in drupal core', hands down
Comment #199
YesCT CreditAttribution: YesCT commentedyay! everyone++
Comment #200
DamienMcKennaAmazing work, everyone!
Comment #201
Gábor Hojtsy- Added change notice at https://drupal.org/node/2138053 (https://twitter.com/drupal8changes/status/402380225952374784 if you want to retweet)
- Updated contrib page marking it obsolete and unsupported in favor of the core module: https://drupal.org/project/config_translation
- Wanted to add a new core component for config_translation in the issue queue but @tim.plunkett already did
- Moved all "ongoing" config translation issues to https://drupal.org/project/issues/drupal?component=config_translation.mo... (note most are postponed on other core issues :)
- Rescoped the existing UI polish issue to #2113797: Design and build dedicated translation form for views which should cover the concerns above
Still to open some other followups and respond to some feedback here, but wanted to post interim updates :)
Comment #202
Gábor HojtsyOpened #2138133: Clean up maintainers.txt and changelog.txt for multilingual changes to add this to the changelog (and while at it, add more missing things there and also fill in the missing pieces in maintainers (not for this module but for all of D8MI).
More followups to be opened still.
Comment #203
Gábor Hojtsy#2135787: Move static config entity local tasks to local_tasks.yml also just got RTBC which will make more of the translation tabs appear (without any change in the config translation module itself).
Comment #204
klonos@webchick, #196 question about the title as a field:
original issue: #1188394: Make title behave as a configurable, translatable field (again) -> closed as a duplicate in favor of #1498674: Refactor node properties to multilingual and #2004626: Make non-configurable field translation settings available in the content language settings. The status of both of these issues is set to "fixed" so shouldn't that be doable now? There's also mention of #1513054: Move "Title field label" setting to "Manage Fields" tab.
Comment #205
plachThis is one of my favorite D8 patches! Awesome work guys!
Comment #206
Gábor HojtsyProcessed all remaining @todo items from @webchick:
192/2: Opened #2139185: Notify users when landing on config translation page with only one language listed.
192/3: Opened #2139191: Make batch steps (when adding language) more informative.
192/4: Already has #2113797: Design and build dedicated translation form for views as pointed out above. I copied your comments there too.
196/6: A DSM is *already* in core when you add a language it tells you to add the switcher block: . This was added in #1738374: Provide a cue to enable the language switcher when adding a language and you committed it :) When we wanted to add a block automatically, Bojhan responded with this:
in #1738374-3: Provide a cue to enable the language switcher when adding a language, so we are not adding it automatically. Not sure how to make it more evident though. There is #1891450: message to enable the language switcher block when adding a language is not noticed (Automatically enable?) for your case of people not noticing it, where we can brainstorm more.
196/7: I cannot reproduce the toolbar and views table not going RTL, see:
196/8: I cannot reproduce the configure operation / tab not showing on blocks. The delete will not be a tab. But delete should not be tabs: #1834002: Configuration delete operations are all over the place. See:
196/16: The node title field translatability question was adequately answered by @klonos I think.
With that all followup requests / @todos (in reviews) are addressed I think.
Comment #207
Gábor HojtsyOh, BTW I also dug up why file/field module is required for locale. This commit added that where some file functions were refactored to be in file module instead of global Drupal functions: http://drupalcode.org/project/drupal.git/commitdiff/316c1f4a7a2c9654c372... (locale handles manual .po file uploads and a set of files in a directory as a result). Then file module requires field module because its a field. That is a bit of a twisted relationship, but this is the state of it in core for 15 months at this point.
Comment #208
XanoMinor documentation bug in
config_translation.api.php
: #2140803: hook_config_translation_info(_alter)() docblock references incorrect YAML file.UX/access control issue: #2140925: Translate operation link for config entities points to a 404 page.
Comment #209
Gábor HojtsyAnother try at removing the sprint tag :)
Comment #210
rszrama CreditAttribution: rszrama commentedI'm currently defining a configuration entity type and see the config_prefix property in the entity type annotations I'm referencing, but I don't see it listed in \Drupal\Core\Entity\Annotation\EntityType or documented elsewhere. I couldn't tell which of the related issues added this (if any) so perhaps it's just a convention that isn't officially part of the entity type annotation?
Comment #211
amateescu CreditAttribution: amateescu commented@rszrama, we'll be able to document that config entity annotation property after #2119905: Provide @ConfigEntityType and @ContentEntityType to have better defaults lands, until then, its documentation is located in the doxygen block of
Drupal\Core\Config\Entity\ConfigStorageController
.Comment #212
rszrama CreditAttribution: rszrama commentedGuess I'll follow along there then. Thanks! : )
Comment #214
batigolixIn #2161801: Update hook_help for Config translation module we review the hook_help text that has its origin in this issue.
Linking them together as related.