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.

  1. 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.
  2. 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.

Related

CommentFileSizeAuthor
#181 1952394-175-api-docs-interdiff.patch2.13 KBtstoeckler
#175 interdiff.txt28.56 KBtim.plunkett
#175 changes.txt5.31 KBtim.plunkett
#175 config_translation-1952394-175.patch235.33 KBtim.plunkett
#174 config-translation-module-174.patch235.6 KBGábor Hojtsy
#162 ConfigTranslationEdit.png93.1 KBGábor Hojtsy
#162 ConfigTranslationOverview.png38.73 KBGábor Hojtsy
#159 ConfigTranslation.png192.91 KBGábor Hojtsy
#157 Config translation architecture (1).jpg55.09 KBGábor Hojtsy
#155 Config translation architecture.jpg53.91 KBGábor Hojtsy
#153 config-translation-module-153.patch235.87 KBGábor Hojtsy
#150 config-translation-module-150.patch232.83 KBGábor Hojtsy
#144 1952394-config-translation-core-144.patch196.5 KBGábor Hojtsy
#136 1952394-config-translation-core-136.patch191.02 KBvijaycs85
#136 1952394-diff-131-136.txt21.03 KBvijaycs85
#131 1952394-config-translation-core-131.patch193.81 KBvijaycs85
#131 1952394-diff-123-131.txt56.49 KBvijaycs85
#126 interdiff.txt982 bytestim.plunkett
#126 config-translation-1952394-126.patch193.58 KBtim.plunkett
#125 config-translation-1952394-124.patch193.59 KBtim.plunkett
#125 interdiff.txt47.12 KBtim.plunkett
#123 1952394-config-translation-core-123.patch194.01 KBvijaycs85
#123 1952394-diff-115-123.txt132.09 KBvijaycs85
#115 1952394-config-translation-core-115.patch161.61 KBvijaycs85
#115 1952394-diff-113-115.txt95.25 KBvijaycs85
#113 1952394-config-translation-core-113.patch137.57 KBvijaycs85
#113 1952394-diff-109-113.txt72.92 KBvijaycs85
#109 1952394-config-translation-core-109.patch98.02 KBvijaycs85
#109 1952394-diff-104-109.txt19.15 KBvijaycs85
#104 1952394-config-translation-core-104.patch95.68 KBvijaycs85
#104 1952394-diff-93-104.txt56.97 KBvijaycs85
#95 Screenshot from 2013-08-21 20:07:16.png75.03 KBKartagis
#95 Screenshot from 2013-08-21 20:07:33.png70.67 KBKartagis
#95 Screenshot from 2013-08-21 20:08:27.png47.92 KBKartagis
#95 Screenshot from 2013-08-21 20:08:46.png54.12 KBKartagis
#93 1952394-config-translation-core-93.patch96.13 KBvijaycs85
#93 1952394-diff-90-93.txt3.84 KBvijaycs85
#90 1952394-config-translation-core-90.patch95 KBvijaycs85
#90 1952394-diff-88-90.txt80.45 KBvijaycs85
#88 1952394-config-translation-core-88.patch172.34 KBvijaycs85
#88 1952394-diff-68-88.txt28.96 KBvijaycs85
#72 d8mi-config-trans-site-info-english.png89.73 KBKristen Pol
#72 d8mi-conf-trans-site-info-greek.png48.04 KBKristen Pol
#68 1952394-config-translation-core-68.patch169.57 KBvijaycs85
#68 1952394-diff-50-68.txt92.64 KBvijaycs85
#64 1952394-config-translation-core-64.patch77.98 KBvijaycs85
#64 1952394-diff-50-64.txt77.01 KBvijaycs85
#50 1952394-config-translation-core-50.patch77.01 KBvijaycs85
#50 1952394-diff-49-50.txt10.61 KBvijaycs85
#49 1952394-config-translation-core-49.patch79.97 KBvijaycs85
#49 1952394-diff-45-49.txt11.68 KBvijaycs85
#45 1952394-config-translation-core-45.patch79.02 KBvijaycs85
#45 1952394-40-45.txt16.53 KBvijaycs85
#40 1952394-config-translation-core-40.patch78.05 KBSchnitzel
#30 Screenshot_5_20_13_1_19_PM.png80.17 KBdagmita
#29 1952394-config-translation-core-29.patch72.58 KBSchnitzel
#24 drupal-teddy-needs-config-translation.jpg68.05 KBvijaycs85
#24 drupal-doggie-needs-config-translation.jpg72.35 KBvijaycs85
#14 1952394-config-translation-core-14.patch73.7 KBvijaycs85
#14 1952394-diff-13-14.txt471 bytesvijaycs85
#13 1952394-config-translation-core-13.patch73.69 KBvijaycs85
#13 1952394-config-diff-8-13.txt63.18 KBvijaycs85
#8 config-translation-module-8.patch59.4 KBGábor Hojtsy
#5 Screenshot_3_29_13_6_13_PM 2.png31 KBGábor Hojtsy
#4 Screenshot_3_29_13_6_12_PM.png47.53 KBGábor Hojtsy
#4 Screenshot_3_29_13_6_13_PM.png27.37 KBGábor Hojtsy
#4 Screenshot_3_29_13_6_13_PM.png27.37 KBGábor Hojtsy
#4 Screenshot_3_29_13_6_14_PM.png57.61 KBGábor Hojtsy
#4 Screenshot_3_29_13_6_15_PM.png87.97 KBGábor Hojtsy
#2 config-translation-snapshot-2.patch56.21 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +Configuration schema

Add config schema tag for tracking.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
56.21 KB

Here 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.

plach’s picture

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.

If 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.

Gábor Hojtsy’s picture

Screenshots:

Site information (look translate tab):
Screenshot_3_29_13_6_12_PM.png

Structure of translate tab:
Screenshot_3_29_13_6_13_PM.png

Adding one translation (originals copied in by default):
Screenshot_3_29_13_6_13_PM 2.png

Translate tabs are added on things, like views listing, menu listing (shown here), vocabularies listing, etc. (note translate operation):
Screenshot_3_29_13_6_14_PM.png

A more complex form, the account settings form that has structured input for emails subject/body pairs:
Screenshot_3_29_13_6_15_PM.png

Gábor Hojtsy’s picture

Uploaded one picture twice accidentally.

Gábor Hojtsy’s picture

@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).

Gábor Hojtsy’s picture

Issue tags: +sprint

Add on D8MI sprint for tracking.

Gábor Hojtsy’s picture

YesCT’s picture

I 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.

sun’s picture

This 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.

Gábor Hojtsy’s picture

This 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.

mondrake’s picture

I 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.

vijaycs85’s picture

Updating new routing system and form (more details at #1985880: Convert to routing system) changes.

vijaycs85’s picture

Gábor Hojtsy’s picture

Note 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

YesCT’s picture

good. it passed. I ran that failing test locally earlier today and it passed also.

falcon03’s picture

Has anyone reviewed this UI for its accessibility? If not, is it ready for an accessibility review?

Gábor Hojtsy’s picture

It 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!

falcon03’s picture

@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.

vijaycs85’s picture

YesCT’s picture

@vijaycs85

HA! :)

victor-shelepen’s picture

Hello. I've installed a module. But I can not find any local menu tabs to translate. I continue to investigate why.

vijaycs85’s picture

@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 ?

victor-shelepen’s picture

It works now. There were problems during the installation. I've fixed. Thank you.

Schnitzel’s picture

newest patch from the module so that it can be applied at simplytest.me

dagmita’s picture

I 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

dagmita’s picture

I figured out my mistakes in testing. so the comment above is outdated. I added the correct steps of testing in the issue summary.

attiks’s picture

+++ b/core/modules/config_translation/config_translation.admin.cssundefined
@@ -0,0 +1,8 @@
+  width:48%;

i think we add a space after ':'

+++ b/core/modules/config_translation/config_translation.moduleundefined
@@ -0,0 +1,432 @@
+function config_translation_config_translation_group_info() {

there are some magic numbers used in this function

attiks’s picture

I just tried the patch and wauuw, great job

falcon03’s picture

Status: Needs review » Needs work
Issue tags: -Needs accessibility review

I 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.

Schnitzel’s picture

i think we add a space after ':'

this will be fixed here: http://drupal.org/node/1999156#comment-7454380

Accessibiltiy Review

issues will be fixed here: #2003834: Accessibility fixes

vijaycs85’s picture

Reg

+++ b/core/modules/config_translation/config_translation.moduleundefined
@@ -0,0 +1,432 @@
+function config_translation_config_translation_group_info() {

in #33

we need them to find the position of entity in URL.

Anonymous’s picture

Site 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.

Schnitzel’s picture

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:

this should be fixed in the newest version of the Configuration Translation module: http://drupal.org/project/config_translation

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
78.05 KB

full patch from the contrib module

Schnitzel’s picture

Here an update what has been achieved since the the last patch:

falcon03’s picture

Re-adding the "needs accessibility review" tag as a reminder to give a final review as soon as possible, probably within next week.

falcon03’s picture

falcon03’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Added 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.

Gábor Hojtsy’s picture

Issue summary: View changes

add suggested commit meesage

Gábor Hojtsy’s picture

Issue summary: View changes

Update list of people in suggested commit.

vijaycs85’s picture

vijaycs85’s picture

Status: Needs review » Needs work

Created a major issue that's causing test fails #2019121: All configuration translation pages throwing FATAL error

Gábor Hojtsy’s picture

(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!

vijaycs85’s picture

Issue summary: View changes

Update suggested commit message.

vijaycs85’s picture

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

bojanz’s picture

+function config_translation_entity_operation_alter(array &$operations, \Drupal\Core\Entity\EntityInterface $entity) {
+  $uri = $entity->uri();
+  $operations['translate'] = array(
+    'title' => t('Translate badabumm'),

:)

Anonymous’s picture

Issue summary: View changes

Updated issue summary with #50

Gábor Hojtsy’s picture

Added @helenkim due to #2026875: Remove accidental badaboom. Also sent for retest, because the RDFa fails are surely not related.

Gábor Hojtsy’s picture

Issue summary: View changes

Added helenkim thanks to https://drupal.org/node/2026875

Gábor Hojtsy’s picture

Added @kfritsche to list of commit mentions due to amazing config_translation test work. Actual code changes will come in a later compound patch.

Kristen Pol’s picture

All green... what's left to do? Need more testing?

Gábor Hojtsy’s picture

There are issues at https://drupal.org/project/issues/config_translation that are still to be done. Most of them.

Gábor Hojtsy’s picture

Issue summary: View changes

Adding @kfritsche based on amazing config_translation test work.

Gábor Hojtsy’s picture

Added David Hernández to the list of people on the issue credit list thanks to contributions in several config_translation issues.

Gábor Hojtsy’s picture

Issue summary: View changes

Add David Hernández due to several issues in the contrib module.

Gábor Hojtsy’s picture

Adding dawehner thanks to his fix provided in #2019831: people and roles page broken .

catch’s picture

+++ b/core/modules/config_translation/config_translation.moduleundefined
@@ -0,0 +1,382 @@
+ */
+function config_translation_help($path, $arg) {

This 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.

+++ b/core/modules/config_translation/config_translation.moduleundefined
@@ -0,0 +1,382 @@
+      $output .= '<p>' . t('The Configuration Translation module lets you translate configuration from all around your Drupal site. Views, your site name, contact module categories, vocabularies, menus, blocks, and so on are all stored with Drupal\'s unified configuration system and can be translated with this module. Content, such as nodes, taxonomy terms, custom blocks, and so on are translatable with the <em>Content translation</em> module in Drupal core, while the built-in user interface (such as registration forms, content submission and administration interfaces) are translated with the <em>Interface translation</em> module. Use these tthree modules effectively together to translate your whole site to different languages.') . '</p>';

tthree.

+++ b/core/modules/config_translation/config_translation.moduleundefined
@@ -0,0 +1,382 @@
+/**
+ * Returns HTML for translation manage element.
+ *
+ * @param array $variables
+ *   An associative array containing:
+ *   - element: The element that contains the source and translation childs
+ *
+ * @see ConfigTranslationManageForm::buildConfigForm
+ * @ingroup themeable
+ */
+function theme_config_translation_manage_form_element($variables){
+  $element = $variables['element'];
+  $result = '
+  <div class="clearfix translation-element-wrapper">
+    <div class="source">
+      ' . drupal_render($element['source']) . '
+    </div>
+    <div class="translation">
+      ' . drupal_render($element['translation']) . '
+    </div>
+  </div>
+  ';
+  return $result;
+}

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.

catch’s picture

Issue summary: View changes

Adding dawehner thanks to his fix provided in https://drupal.org/node/2019831

Gábor Hojtsy’s picture

Issue summary: View changes

Add lazysoundsystem thanks to https://drupal.org/node/2028127

Gábor Hojtsy’s picture

Issue summary: View changes

Add juanolalla, R.Hendel to the credit list due to their recent contributions.

YesCT’s picture

Several 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

vijaycs85’s picture

vijaycs85’s picture

Status: Needs review » Needs work
FileSize
92.64 KB
169.57 KB

Re-rolling with updates:

vijaycs85’s picture

Issue summary: View changes

Updated issue summary to highlight core blockers to getting this in. -y

vijaycs85’s picture

Status: Needs work » Needs review
Kristen Pol’s picture

I applied the patch. Should the translate tab show up if there is only one language? It does currently.

Gábor Hojtsy’s picture

If 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.

Kristen Pol’s picture

When 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

d8mi-config-trans-site-info-english.png

Greek site information translation

d8mi-conf-trans-site-info-greek.png

Kristen Pol’s picture

Re #71, I only had English and the translate tab showed up.

Gábor Hojtsy’s picture

@Kristen: well, those are the only *translatable* fields on the form. We don't do *language specific values* we do *translation* only.

penyaskito’s picture

@Gábor: Would be any way for doing *language specific values* in contrib? IMHO this is a useful feature. Sorry if this is offtopic.

Gábor Hojtsy’s picture

I'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.

Kristen Pol’s picture

Thanks 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?

plach’s picture

The translation worked fine for me for those two fields. Is there some specific/other testing that is needed?

As 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 its config/schema directory :)

Kristen Pol’s picture

@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?

./modules/action/config/schema/action.schema.yml
./modules/aggregator/config/schema/aggregator.schema.yml
./modules/block/config/schema/block.schema.yml
./modules/block/custom_block/config/schema/custom_block.schema.yml
./modules/book/config/schema/book.schema.yml
./modules/breakpoint/config/schema/breakpoint.schema.yml
./modules/ckeditor/config/schema/ckeditor.schema.yml
./modules/config/tests/config_test/config/config_test.noschema.yml
./modules/config/tests/config_test/config/config_test.someschema.yml
./modules/config/tests/config_test/config/schema/config_test.schema.yml
./modules/contact/config/schema/contact.schema.yml
./modules/dblog/config/schema/dblog.schema.yml
./modules/editor/config/schema/editor.schema.yml
./modules/field/config/schema/field.schema.yml
./modules/field_sql_storage/config/schema/field_sql_storage.schema.yml
./modules/field_ui/config/schema/field_ui.schema.yml
./modules/file/config/schema/file.schema.yml
./modules/filter/config/schema/filter.schema.yml
./modules/forum/config/schema/forum.schema.yml
./modules/image/config/schema/image.schema.yml
./modules/language/config/schema/language.schema.yml
./modules/locale/config/schema/locale.schema.yml
./modules/menu/config/schema/menu.schema.yml
./modules/node/config/schema/node.schema.yml
./modules/options/config/schema/options.schema.yml
./modules/picture/config/schema/picture.schema.yml
./modules/search/config/schema/search.schema.yml
./modules/shortcut/config/schema/shortcut.schema.yml
./modules/statistics/config/schema/statistics.schema.yml
./modules/syslog/config/schema/syslog.schema.yml
./modules/system/config/schema/system.data_types.schema.yml
./modules/system/config/schema/system.schema.yml
./modules/taxonomy/config/schema/taxonomy.schema.yml
./modules/text/config/schema/text.schema.yml
./modules/toolbar/config/schema/toolbar.schema.yml
./modules/tour/config/schema/tour.schema.yml
./modules/tour/tests/tour_test/config/schema/tour_test.schema.yml
./modules/tracker/config/schema/tracker.schema.yml
./modules/translation/config/schema/translation.schema.yml
./modules/update/config/schema/update.schema.yml
./modules/user/config/schema/user.schema.yml
./modules/views/config/schema/views.access.schema.yml
./modules/views/config/schema/views.area.schema.yml
./modules/views/config/schema/views.argument.schema.yml
./modules/views/config/schema/views.argument_default.schema.yml
./modules/views/config/schema/views.argument_validator.schema.yml
./modules/views/config/schema/views.cache.schema.yml
./modules/views/config/schema/views.data_types.schema.yml
./modules/views/config/schema/views.display.schema.yml
./modules/views/config/schema/views.display_extender.schema.yml
./modules/views/config/schema/views.exposed_form.schema.yml
./modules/views/config/schema/views.field.schema.yml
./modules/views/config/schema/views.filter.schema.yml
./modules/views/config/schema/views.filter_value.schema.yml
./modules/views/config/schema/views.pager.schema.yml
./modules/views/config/schema/views.query.schema.yml
./modules/views/config/schema/views.relationship.schema.yml
./modules/views/config/schema/views.row.schema.yml
./modules/views/config/schema/views.schema.yml
./modules/views/config/schema/views.sort.schema.yml
./modules/views/config/schema/views.sort_expose.schema.yml
./modules/views/config/schema/views.style.schema.yml
Gábor Hojtsy’s picture

@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.

vijaycs85’s picture

@Kristen Pol, here is the summary of #79 and #80

  1. Check for list of .schema.yml that has either type: label or type: text
  2. Check for corresponding front end UI of the config.
  3. a. CASE 1: No UI - not sure how to deal it. But we can note.
    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.
Kristen Pol’s picture

These have type: label

./core/modules/block/config/schema/block.schema.yml
./core/modules/block/custom_block/config/schema/custom_block.schema.yml
./core/modules/contact/config/schema/contact.schema.yml
./core/modules/field/config/schema/field.schema.yml
./core/modules/filter/config/schema/filter.schema.yml
./core/modules/image/config/schema/image.schema.yml
./core/modules/language/config/schema/language.schema.yml
./core/modules/picture/config/schema/picture.schema.yml
./core/modules/shortcut/config/schema/shortcut.schema.yml
./core/modules/system/config/schema/system.data_types.schema.yml
./core/modules/system/config/schema/system.schema.yml
./core/modules/taxonomy/config/schema/taxonomy.schema.yml
./core/modules/text/config/schema/text.schema.yml
./core/modules/tour/config/schema/tour.schema.yml
./core/modules/user/config/schema/user.schema.yml
./core/modules/views/config/schema/views.area.schema.yml
./core/modules/views/config/schema/views.data_types.schema.yml
./core/modules/views/config/schema/views.field.schema.yml
./core/modules/views/config/schema/views.filter.schema.yml
./core/modules/views/config/schema/views.pager.schema.yml
./core/modules/views/config/schema/views.schema.yml
./core/modules/views/config/schema/views.sort_expose.schema.yml
./core/modules/views/config/schema/views.style.schema.yml

And these have type: text

./core/modules/block/custom_block/config/schema/custom_block.schema.yml
./core/modules/config/tests/config_test/config/schema/config_test.schema.yml
./core/modules/contact/config/schema/contact.schema.yml
./core/modules/field/config/schema/field.schema.yml
./core/modules/system/config/schema/system.data_types.schema.yml
./core/modules/system/config/schema/system.schema.yml
./core/modules/text/config/schema/text.schema.yml
./core/modules/tour/config/schema/tour.schema.yml
./core/modules/views/config/schema/views.area.schema.yml
./core/modules/views/config/schema/views.data_types.schema.yml
./core/modules/views/config/schema/views.display.schema.yml
./core/modules/views/config/schema/views.exposed_form.schema.yml
./core/modules/views/config/schema/views.schema.yml
vijaycs85’s picture

Thanks again for the list @Kristen Pol. Here is the categories version of #82 as per #81
CASE 1:

  1. tour.schema.yml
  2. system.data_types.schema.yml
  3. config_test.schema.yml

CASE 2:

  1. system.schema.yml
    a. system.rss
  2. image.schema.yml

Created an issue #2044387: Add remaining configuration entity or page into configuration translation module for all in CASE 2.

CASE 3 - Pass:

  1. custom_block.schema.yml - admin/structure/custom-blocks/manage/basic/translate
  2. contact.schema.yml - admin/structure/contact/manage/personal/translate
  3. field.schema.yml - admin/structure/types/manage/article/fields/node.article.body/translate
  4. filter.schema.yml - admin/structure/custom-blocks/manage/basic/translate
  5. shortcut.schema.yml
  6. system.schema.yml
    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
  7. taxonomy.schema.yml - admin/structure/taxonomy/manage/tags/translate
  8. text.schema.yml - Covered under field.schema.yml
  9. user.schema.yml
    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
  10. covered under views.schema.yml:

    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:

  1. language.schema.yml - admin/config/regional/language/fr/translate
  2. picture.schema.yml - admin/config/media/picturemapping/iphone/translate
  3. block.schema.yml - admin/structure/block/manage/bartik.content/translate

Created an issue #2044389: [META] Fix broken configuration translation pages for all in CASE 3 - Not working.

Kristen Pol’s picture

@vijaycs85 - thanks for testing and opening the other issues :)

Gábor Hojtsy’s picture

Issues 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

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary with #68

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue summary: View changes

updated testing. need to update blockers still.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Needs a reroll based on all the changes that went to the contrib module.

vijaycs85’s picture

Issue summary: View changes

Add EllaTheHarpy thanks to work in https://drupal.org/node/2029407

vijaycs85’s picture

Seems the patch in #88 got wrong files. updating with a path fix from #2065309: Custom block types changed type listing path in core

pwolanin’s picture

Status: Needs review » Needs work

shouldn't go into core adding a hook_menu implemetaiton

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@pwolanin: yeah, well, how could we do this without hook_menu()? This is what @dawerhner said in #2044737-5: Provide local tasks as plugins:

There are many probles we have to figure out first, before that one can be implemented:

Nevertheless, this is a start to show how it should look like.

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.

vijaycs85’s picture

Gábor Hojtsy’s picture

I 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!

Kartagis’s picture

Here 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,

Gábor Hojtsy’s picture

@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 :)

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary with #88

Kartagis’s picture

Adding me as author would be nice :)

Gábor Hojtsy’s picture

@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 :)

Kartagis’s picture

Sorry I meant --author for the patch on #2071169: Language names all lowercase :)

YesCT’s picture

@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

purabdk’s picture

Status: Needs review » Needs work

I 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.

Gábor Hojtsy’s picture

I'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!

Gábor Hojtsy’s picture

  1. config_translation.api.php - line no 10 - why we kept the @ and curly bracket

    This notation is used to add a list of functions (in this case documentation functions) to a group. This is normal.

  2. config_translation/ConfigGroupMapper.php - line no 253 - why returns the method null, I am not sure about this.

    the getType method has been removed from this mapper in recent updates

  3. Controller/ConfigTranslationController.php - line no 44 - $langcode = $request->query->get('langcode');
    $langcode is unused variable.

    - this has been recently removed from the contrib copy of the module, patch update coming

  4. Form/ConfigTranslationManageForm.php - line 109 - validation method is not doing anything.
  5. - 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.

vijaycs85’s picture

Issue summary: View changes

Add Kartagis for testing, and a couple patches

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@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

vijaycs85’s picture

vijaycs85’s picture

Issue summary: View changes

Updated issue summary for #104

vijaycs85’s picture

Issue summary: View changes

Updated issue summary for #109

YesCT’s picture

We 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.

YesCT’s picture

Issue summary: View changes

Add webflo and tstoeckler for lots of recent work on config_translation.

Gábor Hojtsy’s picture

Issue summary: View changes

Add penyaskito for recent contributions to the module :)

vijaycs85’s picture

Issue summary: View changes

Add EclipseGC for plugin help, reviews, suggested things.

alexpott’s picture

Issue summary: View changes

Updates from #113

vijaycs85’s picture

Issue summary: View changes

added core blockers from the prague google doc

larowlan’s picture

Code review only, didn't test yet but very excited by the possibilities this patch presents.

  1. +++ b/core/modules/config_translation/config_translation.api.php
    @@ -0,0 +1,79 @@
    +function hook_config_translation_info(&$info) {
    

    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.

  2. +++ b/core/modules/config_translation/config_translation.module
    @@ -0,0 +1,159 @@
    +function config_translation_entity_operation_alter(array &$operations, \Drupal\Core\Entity\EntityInterface $entity) {
    

    can we add 'use Drupal\Core\Entity\EntityInterface' and type-hint the short-form instead?

  3. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Access/ConfigNameCheck.php
    @@ -0,0 +1,84 @@
    +      $target_language = language_load($request->query->get('langcode'));
    

    Observation: stands out like a sore thumb in an otherwise unit-testable class :(

  4. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -0,0 +1,226 @@
    +class ConfigEntityMapper extends ConfigNamesMapper {
    

    I don't see any phpunit tests for this class.

  5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -0,0 +1,226 @@
    +   *   An array of definition with mapper details.
    

    nitpick: definitions?

  6. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -0,0 +1,226 @@
    +   * configuration names to use to serve to check permissions or display a
    

    this sentence doesn't read very clearly, one to too many?

  7. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -0,0 +1,226 @@
    +   *   TRUE, if the entity was set successfully; FALSE otherwise.
    ...
    +   *   TRUE if the entity type was set correctly; FALSE otherwise.
    

    FALSE is returned here even if the entity is already set? That seems confusing.

  8. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -0,0 +1,226 @@
    +    $this->title = t($this->title, array('!label' => $entity->label()));
    ...
    +          'title' => t('List'),
    

    Can we inject the string translation service? That would mean we can get 100% unit test coverage of this class.

  9. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -0,0 +1,226 @@
    +    return array($this->entityType => $this->entity->id());
    

    What happens if $this->entity isn't set? Should we check first and throw an Exception?

  10. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperInterface.php
    @@ -0,0 +1,171 @@
    +   * Returns name of type of data the mapper encapsulates.
    

    nitpick: Returns the name?

  11. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperInterface.php
    @@ -0,0 +1,171 @@
    +   * Checks that all pieces of this configuration mapper has schema.
    

    nitpick:proper sentence structure

  12. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperInterface.php
    @@ -0,0 +1,171 @@
    +   * If the path pattern contains an entity placeholder for example, the entity ID
    

    >80 chars

  13. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
    @@ -0,0 +1,383 @@
    +class ConfigNamesMapper extends DependencySerialization implements ConfigMapperInterface {
    

    There are no unit tests for this class either?

  14. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
    @@ -0,0 +1,383 @@
    +   * {@inheritdoc}
    

    nitpick: missing @throws

  15. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
    @@ -0,0 +1,383 @@
    +    $route_provider = \Drupal::service('router.route_provider');
    

    Can this be injected? (apologies if this has been asked before)

  16. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationBlockListController.php
    @@ -0,0 +1,101 @@
    +    $this->themes = list_themes();
    

    observation: ouch

  17. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationBlockListController.php
    @@ -0,0 +1,101 @@
    +    $header['label'] = t('Block');
    +    $header['theme'] = t('Theme');
    +    $header['category'] = t('Category');
    +    $header['operations'] = t('Operations');
    

    $this->t

  18. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    @@ -0,0 +1,182 @@
    +   * Language translations overview page for a configuration name.
    

    nitpick: missing @throws

  19. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    @@ -0,0 +1,182 @@
    +    /** @var \Drupal\config_translation\ConfigMapperInterface $mapper */
    

    ?

  20. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    @@ -0,0 +1,182 @@
    +    // on a single route due to the hook_menu() parent limitation.
    

    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.

  21. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    @@ -0,0 +1,182 @@
    +        // link when clicked. Do we know better?
    

    If so should we add #access => FALSE?

  22. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationEntityListController.php
    @@ -0,0 +1,139 @@
    +    $header['label'] = t('Label');
    

    Can we $this->t here?

  23. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,196 @@
    +    foreach (Field::fieldInfo()->getInstances($this->baseEntityType) as $fields) {
    

    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).

  24. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,196 @@
    +    $header['label'] = t('Field');
    ...
    +    $header['operations'] = t('Operations');
    

    $this->t?

  25. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -0,0 +1,88 @@
    +   * Provides the listing page for any entity type.
    

    nitpick:missing @throws?

  26. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -0,0 +1,88 @@
    +    $controller = new $class($entity_type, $this->entityManager()->getDefinition($entity_type), $this->entityManager()->getStorageController($entity_type), $this->moduleHandler(), $this->entityManager(), $this->mapperDefinition);
    

    What happens if contrib wants a different list controller that has different arguments? Should we have a factory interface/method pair here?

  27. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    @@ -0,0 +1,148 @@
    +    $this->mapperManager = $mapper_manager;
    

    Doesn't appear to be used other than to store $this->mappers? Is there a need for the mapperManager property?

  28. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    @@ -0,0 +1,148 @@
    +    $row['Label'] = t('Label');
    +    $row['operations'] = t('Operations');
    

    $this->t

  29. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.php
    @@ -0,0 +1,134 @@
    +      'route_name' => $this->mapper->getRouteName(),
    +      'route_parameters' => $this->mapper->getRouteParams(),
    

    lovely!

  30. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.php
    @@ -0,0 +1,134 @@
    +    foreach (Cache::getBins() as $service_id => $cache_backend) {
    

    Same comment about statics as for FieldInfo? - although calls to drupal_set_message so unit-testing use-case redundant

  31. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
    @@ -0,0 +1,355 @@
    +        /** @var \Drupal\config_translation\FormElement\Element $form_element */
    

    ?

  32. +++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/Element.php
    @@ -0,0 +1,56 @@
    +abstract class Element {
    

    is there an ElementInterface? Is it even needed?

  33. +++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/Element.php
    @@ -0,0 +1,56 @@
    +  abstract public function getFormElement($definition, $language, $value);
    

    love learning new things from reading patches! thanks

  34. +++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/Textarea.php
    @@ -0,0 +1,32 @@
    +    $rows = max($rows_words, $rows_newlines);
    

    slick!

  35. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Plugin/Derivative/ConfigTranslationLocalTasks.php
    @@ -0,0 +1,64 @@
    +        $this->derivatives[$tab_name]['title'] = t('Translate @title', array('@title' => drupal_strtolower($mapper->getTypeName())));
    

    $this->t?

  36. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
    @@ -0,0 +1,82 @@
    +class RouteSubscriber implements EventSubscriberInterface {
    

    Should we have unit tests for this class?

  37. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php
    @@ -0,0 +1,492 @@
    +    $this->drupalPlaceBlock('system_powered_by_block', array('machine_name' => $block_machine_name));
    

    I'm seeing 'id' instead of machine name after the recent changes to HEAD?

  38. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationOverviewTest.php
    @@ -0,0 +1,112 @@
    +  function testMapperListPage() {
    
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUITest.php
    @@ -0,0 +1,542 @@
    +  function testSiteInformationTranslationUI() {
    ...
    +  function testSourceValueDuplicateSave() {
    ...
    +  function testContactConfigEntityTranslation() {
    ...
    +  function testAccountSettingsConfigurationTranslation() {
    ...
    +  function testSourceAndTargetLanguage() {
    ...
    +  function testViewsTranslationUI() {
    ...
    +  function testLocaleDBStorage() {
    ...
    +  function testSingleLanguageUI() {
    

    missing scope modifier public|protected?

  39. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUITest.php
    @@ -0,0 +1,542 @@
    +      $source = $this->container->get('config.factory')->get($config_name)->get($key);
    

    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)

  40. +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigMapperManagerTest.php
    @@ -0,0 +1,185 @@
    +   * @dataProvider providerTestHasTranslatable
    

    I think we're putting @group Drupal too see #2057905: [policy, no patch] Discuss the standards for phpunit based tests

  41. +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigMapperManagerTest.php
    @@ -0,0 +1,185 @@
    +  public function providerTestHasTranslatable() {
    

    phpunit++

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary for #115

Gábor Hojtsy’s picture

Issue tags: +Prague Hard Problems

Tagging as one of the prague hard problems that we discussed :)

Gábor Hojtsy’s picture

Issue summary: View changes

Promoted 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.

Gábor Hojtsy’s picture

@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. :)

Gábor Hojtsy’s picture

Issue summary: View changes

Updating issue summary removing core tasks that are done and cleaning up steps to test.

YesCT’s picture

Issue summary: View changes

just nice formatting issue/comment link

larowlan’s picture

3) 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

tstoeckler’s picture

Thanks 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

Gábor Hojtsy’s picture

@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.

vijaycs85’s picture

Re-rolling with below changes:

vijaycs85’s picture

Issue summary: View changes

clarifying that there are no core blockers anymore.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Reviewing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
47.12 KB
193.59 KB

First 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.

  1. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php
    @@ -0,0 +1,258 @@
    +    return $this->factory->createInstance($plugin_id, array());
    

    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());

  2. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php
    @@ -0,0 +1,258 @@
    +    $discovery = new YamlDiscovery('config_translation', $directories);
    +    $discovery = new InfoHookDecorator($discovery, 'config_translation_info');
    +    $discovery = new ContainerDerivativeDiscoveryDecorator($discovery);
    

    Why are we creating these each time, outside of __construct, and not storing them as $this->discovery?

  3. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php
    @@ -0,0 +1,258 @@
    +    foreach ($definitions as $plugin_id => &$definition) {
    +      $this->processDefinition($definition, $plugin_id);
    +    }
    +    if ($this->alterHook) {
    +      $this->moduleHandler->alter($this->alterHook, $definitions);
    +    }
    +    return $definitions;
    

    Then we wouldn't have to duplicate this from DefaultPluginManager.

  4. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php
    @@ -0,0 +1,258 @@
    +        throw new \RuntimeException("No routes found that match the 'edit-form' link of the '$entity_type' entity type. $base_path");
    ...
    +        throw new \RuntimeException("Multiple routes found that match the 'edit-form' link of the '$entity_type' entity type.");
    ...
    +      throw new InvalidMapperDefinitionException($plugin_id, 'The plugin definition of the mapper ' . $plugin_id . ' does not contain a base_route_name.');
    ...
    +      throw new InvalidMapperDefinitionException($plugin_id, 'The list_controller ' . $definition['list_controller'] . ' for plugin ' . $plugin_id . ' does not implement the expected interface Drupal\config_translation\Controller\ConfigTranslationEntityListControllerInterface.');
    

    Here an elsewhere, we should either use String::format(), or sprintf()

  5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
    @@ -0,0 +1,372 @@
    +      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $this->config($name)->get(), $this->baseConfigData[$name]);
    ...
    +   * @param mixed $schema
    +   *   Schema definition of configuration which is a
    +   *   \Drupal\Core\Config\Schema\Element or a
    +   *   \Drupal\Core\TypedData\TypedConfigManager.
    ...
    +    foreach ($schema as $key => $element) {
    ...
    +      $definition = $element->getDefinition() + array('label' => $this->t('N/A'));
    +      if ($element instanceof Element) {
    ...
    +        $sub_build = $this->buildConfigForm($element, $config_data[$key], $base_config_data[$key], TRUE, $element_key);
    

    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.

tim.plunkett’s picture

Nevermind what I said about the UI being weird, I just made a mistake in my patch :)

EDIT:

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
@@ -132,21 +131,19 @@ public function getFilterLabels() {
+      $bundle = $entity->bundle();

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?

dawehner’s picture

Amazing work! Just some minor comments.

  1. --- /dev/null
    +++ b/core/modules/config_translation/config_translation.admin.css
    

    Afaik we use a separated css folder

  2. +++ b/core/modules/config_translation/config_translation.module
    @@ -0,0 +1,192 @@
    +      catch(RouteNotFoundException $e) {
    

    Nitpick: missing space.

  3. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php
    @@ -0,0 +1,258 @@
    +class ConfigMapperManager extends DefaultPluginManager implements ConfigMapperManagerInterface {
    

    It is a bit confusing that we use plugin.manager.config_translation but the classname is ConfigMapperManager

  4. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManagerInterface.php
    @@ -0,0 +1,36 @@
    +   *
    +   * @return \Drupal\config_translation\ConfigMapperInterface[]
    +   *   An array of all mappers.
    +   */
    +  public function getMappers();
    

    <3

  5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Plugin/Derivative/ConfigTranslationLocalTasks.php
    @@ -0,0 +1,122 @@
    +class ConfigTranslationLocalTasks extends DerivativeBase implements ContainerDerivativeInterface {
    ...
    +   * Constructs a new RouteSubscriber.
    

    This two interformation disagree.

  6. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationListUiTest.php
    @@ -0,0 +1,487 @@
    +  function doLanguageListTest() {
    

    Is there a reason why the test methods do not have a visibility?

  7. +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigMapperManagerTest.php
    @@ -0,0 +1,190 @@
    +  public function providerTestHasTranslatable() {
    

    <3 <3 <3

tim.plunkett’s picture

Status: Needs review » Needs work

Well 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.

tim.plunkett’s picture

Issue tags: +Needs tests

I 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.

Gábor Hojtsy’s picture

I 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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
56.49 KB
193.81 KB

Ok, 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)

vijaycs85’s picture

Issue summary: View changes

Updated issue summary - from #123

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Ok I found the answer to #125/1 (same as #130/1). So the config mapper manager has this code:

  /**
   * {@inheritdoc}
   */
  public function createInstance($plugin_id, array $plugin_definition = array()) {
    return $this->factory->createInstance($plugin_id, array());
  }

  /**
   * {@inheritdoc}
   */
  public function getMappers() {
    $mappers = array();
    foreach($this->getDefinitions() as $id => $definition) {
      $mappers[$id] = $this->createInstance($id, $definition);
    }

    return $mappers;
  }

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

Gábor Hojtsy’s picture

That 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 :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
21.03 KB
191.02 KB

ok, Re-rolling with below commits from config_translation:

  • followup urther fixes for tim.plunkett review(3ef10b7).
  • Fix for access interface compliance following core change in #2048223; removes one core dependent @todo, yay! (8e3dbfe).
vijaycs85’s picture

Issue summary: View changes

Adding sub-tasks from #131

Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs accessibility review, -D8MI, -sprint, -language-config, -Configuration schema, -Prague Hard Problems

The last submitted patch, 1952394-config-translation-core-136.patch, failed testing.

vijaycs85’s picture

Gábor Hojtsy’s picture

Apparently 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.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs accessibility review, -D8MI, -sprint, -language-config, -Configuration schema, -Prague Hard Problems

The last submitted patch, 1952394-config-translation-core-136.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary - from #136

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Postponed

Although 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?.

webchick’s picture

In 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.

Gábor Hojtsy’s picture

Status: Postponed » Needs work
Gábor Hojtsy’s picture

Rerolled with these commits from the module (some dummy looking commits were due to testing the testbot malfunction that was fixed yesterday).

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Sending to testbot and looking for human reviews!

Gábor Hojtsy’s picture

Committing 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.

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

I'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 :/

Gábor Hojtsy’s picture

Drupal people! Any feedback? Does this all look good now? :)

Gábor Hojtsy’s picture

Also new patch for your core passing enjoyment! :) Following commits included:

  • 1dd2c9f Issue #2130075 by Gábor Hojtsy: Temporary workaround for config translation should work with new contextual links API (due to core limitations).
  • 2eb7605 Issue #2123897 by Gábor Hojtsy, dawehner: Restore theme .yml support, add/fix tests. (complete)
  • 83064c9 Issue #2130607 by tstoeckler: Minor clean-up.
  • 02114c0 Issue #2113707 followup by tstoeckler: use the entity list controller interface for proper type recognition
  • e606c5b Issue #2130075 (interim patch) by Gábor Hojtsy: Config translation should work with new contextual links API.
Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Restore tags (hopefully).

Gábor Hojtsy’s picture

Here 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.

  • 5b8a47d Issue #2134895 by Gábor Hojtsy: @todo cleanup, have *core* issue links for all of them.
  • c1656e1 Issue #2132863 by tstoeckler, Gábor Hojtsy: Create proper routes for the add/edit/delete forms independently.
  • bb3abf9 Further cleanups from #2132863 by Gábor Hojtsy (comment 64)
  • 760aa81 Issue #2132863 preparatory patch by tstoeckler: Create proper routes for the add/edit/delete forms.
  • fb98fe9 Issue #2134077 by Gábor Hojtsy: Subclass contextual links and local tasks to have proper dynamic titles.
  • fc38665 Issue #2130633 by tstoeckler: Dedicated unit test ConfigNamesMapper.

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.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
53.91 KB
Gábor Hojtsy’s picture

Issue summary: View changes

Attempting to restore lost elements on the summary that did not even show up earlier....

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
55.09 KB
Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
55.09 KB
Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
192.91 KB
Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
192.91 KB
Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Update again only showed up in node revisions but not in main node :/

Gábor Hojtsy’s picture

Issue summary: View changes
klonos’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: -#2135101: Expand test coverage on configuration translation tabs

#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!

Tor Arne Thune’s picture

This 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!

Gábor Hojtsy’s picture

Issue tags: +alpha target
Gábor Hojtsy’s picture

Issue tags: +alpha target
tim.plunkett’s picture

As 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!)

Gábor Hojtsy’s picture

Agreed with Tim, if at all we can do all this before the alpha :) We'll try to do our best with some committer help :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration context

Working on updates based on those two core patches landing.

tim.plunkett’s picture

Issue summary: View changes
tim.plunkett’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
235.6 KB

Updated 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:

  • af48bc7 Issue #2136723 by Gábor Hojtsy: Titles are translated too early (or not at all).
  • 7050cfe Issue #2136677 by Gábor Hojtsy: DX: Make *.config_translation().yml keys === base route names like local tasks, actions, etc.
  • b251b5b Issue #2136551 by tstoeckler, Gábor Hojtsy: Update for theme_links() supporting route names and entity link annotations being routes.
tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
235.33 KB
5.31 KB
28.56 KB

I'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.

tstoeckler’s picture

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Access/ConfigTranslationFormAccess.php
@@ -47,7 +40,7 @@ public function access(Route $route, Request $request, AccountInterface $account
-        $target_language->id != $source_language->id;
+        $target_language->id != $this->sourceLanguage->id;

Awesome! 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.

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.

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.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Actually, I realized opening an issue in the contrib queue for that is pointless at this point. Your changes look good, so this is RTBC.

Gábor Hojtsy’s picture

Discussed 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 :)

Gábor Hojtsy’s picture

Oh, already done by tstoeckler, heh, thanks!

tstoeckler’s picture

I 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.

tstoeckler’s picture

Here'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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 181: 1952394-175-api-docs-interdiff.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

Sorry, I'm an idiot for not naming my interdiff correctly. #175 is still RTBC.

Tor Arne Thune’s picture

Status: Needs work » Reviewed & tested by the community
Gábor Hojtsy’s picture

Note 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!

klonos’s picture

...perhaps it would help if that little speech was added in the issue summary (this issue is 186 comments long)?

Gábor Hojtsy’s picture

Issue summary: View changes

@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 :)

Gábor Hojtsy’s picture

Hide bugos docs patch... Hopefully less confusing for a core committer now.

tstoeckler’s picture

tstoeckler’s picture

webchick’s picture

Reviewing this now! Stay tuned. :)

webchick’s picture

Here'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. :)

Gábor Hojtsy’s picture

@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 :)

vijaycs85’s picture

Thanks @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).

tstoeckler’s picture

Awesome 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 ++.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks 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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

d.o :(

larowlan’s picture

congratulations to the d8mi team!
this goes at the top of 'this week in drupal core', hands down

YesCT’s picture

yay! everyone++

DamienMcKenna’s picture

Amazing work, everyone!

Gábor Hojtsy’s picture

- 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 :)

Gábor Hojtsy’s picture

Opened #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.

Gábor Hojtsy’s picture

#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).

klonos’s picture

@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.

plach’s picture

This is one of my favorite D8 patches! Awesome work guys!

Gábor Hojtsy’s picture

Processed 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:

I don't think we should be doing this, in one of the earliest tests we did it showed that people where confused by the fact that a block suddenly appears when enabling a module. In the usability test its case the search module, in Drupal 6.

Placing a block is almost always a explicit action, when its not people get confused. A good way to inform people about a new block, is adding it to the install success message.

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.

Gábor Hojtsy’s picture

Oh, 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.

Xano’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint

Another try at removing the sprint tag :)

rszrama’s picture

I'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?

amateescu’s picture

@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.

rszrama’s picture

Guess I'll follow along there then. Thanks! : )

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

batigolix’s picture

In #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.