Problem/Motivation

On #2351991: Add a config entity for a configurable, topic-based help system an attempt was made to create a config entity for help topics. This has currently been postponed to 8.1.x, but it is a contrib module in the meantime (sandbox at the moment).

Unlike most/all other config entities, this one has a Canonical route, which displays the help topic. And, that route is under the "admin" path.

We ran into a problem trying to do this: these help topics need to be translated. But there is some logic in core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php which detects "This is a config entity on an admin path" and forces it *not* to be translated... This is a good choice for config entities in general, because when you are editing config (which is the normal operation) you want to be editing the untranslated config. But it makes it impossible to display the translated help topic.

So there needs to be some way to make config entities on some admin paths be translated.

Proposed resolution

On that other issue, a patch was made that lets admin routes for config entities have a use_current_language parameter that makes the config entity get translated on output (see comment #6 for that patch).

On this issue, we need to get this patch into Core so that the Help topic entity can be potentially used as a contrib module, and to enable any other config entities that need to be translated under 'admin' paths to work as well.

And we need a test for it that doesn't rely on the Help topic entity.

Remaining tasks

Get the patch reviewed and committed.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the assumption that all admin path displays of config entities should be in the original language is flawed
Prioritized changes Bug fixes are prioritized.
Disruption This is not a disruptive change. No core or contrib modules will need to change. The change is only activated if a routing.yml file (or route from an alter class) contains the "use_current_langauge" directive in its config entity parameter options.

User interface changes

None.

API changes

Modules will be able to put "use_current_language" in parameter options in routes to indicate that the config entity parameter being loaded on an admin path should use the negotiated/current language rather than its default language.

Files: 
CommentFileSizeAuthor
#31 interdiff.txt1.69 KBjhodgdon
#31 2369035-30-with-docs.patch7.78 KBjhodgdon
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,749 pass(es). View
#27 2369035-27-test-only-FAIL.patch4.7 KBjhodgdon
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,747 pass(es), 2 fail(s), and 0 exception(s). View

Comments

jhodgdon’s picture

Issue tags: +Needs tests

So the patch above will work -- the tests for the Help entity worked fine. However for this to get into core without the Help entity, we'd probably need tests.

That might be kind of a pain to do... my suggestion would be:

a) Find a test config entity that we hopefully have somewhere

b) Add a new route to it with a use_current_language option on the entity parameter. The relevant bit from the proposed help entity routing.yml file part of the patch was:

+entity.help_topic.canonical:
+  path: '/admin/help-topic/{help_topic}'
+  defaults:
+    _entity_view: 'help_topic.full'
+    _title: 'Help'
+  requirements:
+    _entity_access: 'help_topic.view'
+  options:
+    parameters:
+      help_topic:
+        type: entity:help_topic
+        # Force load in current interface language.
+        use_current_language: true

c) The test needs to translate the config entity, visit that route, and verify that the translated entity is shown and not the base entity if you visit a URL prefixed with the language code, and that the untranslated entity is shown if you visit a non-prefixed URL.

jhodgdon’s picture

Oh it's important that the route path start with "admin" to exercise this bug!

balagan’s picture

I don't see a patch uploaded in comment #1, why is that?

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Issue tags: +D8MI, +language-config, +sprint
jhodgdon’s picture

RE #3 - There is not a patch file uploaded yet. The issue status is also "active" not "needs review". This was started as a spin-off from another issue that had a patch, but there hasn't been a patch made here yet.

jhodgdon’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,963 pass(es). View

Here's the patch for AdminPathConfigEntityConverter.php from the other issue, in the form of an actual patch file. :)

Still needs a test.

robertdbailey’s picture

I'm at BADCamp today and this test looked fun to try to write, so if nobody speaks up I'll take a stab at it. (@Gábor, hope that's ok?)

dawehner’s picture

+++ b/core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php
@@ -88,6 +90,10 @@ public function convert($value, $definition, $name, array $defaults) {
+    if (isset($definition['use_current_language']) && $definition['use_current_language']) {

Isn't that literally the same as !empty()? ... I just care about readability here.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

Robert: sure, I'll review :)

jhodgdon’s picture

Could be... I thought empty() might barf on array elements that are completely unset? Maybe not.

Gábor Hojtsy’s picture

empty() is fine on undefined array elements.

robertdbailey’s picture

Status: Needs review » Needs work
FileSize
2.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,058 pass(es), 5 fail(s), and 0 exception(s). View

Here is a draft of the test for inspection. Still needs a little work, but does this look like what you had in mind, @jhodgdon?

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Set to needs review for testing. Feedback:

  1. +++ b/core/modules/config/tests/config_test/config_test.routing.yml
    @@ -21,6 +21,21 @@ entity.config_test.edit_form:
    +        use_current_language: 'TRUE'
    +        ¶
    

    Extra whitespace.

  2. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
    @@ -120,4 +120,31 @@ public function testHiddenEntities() {
    +     // Verify the form page at test route
    +     // "config_test.entity_using_current_language"
    +     // has the Spanish-translated version of the config_test entity
    

    I don't think we need this comment :)

  3. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
    @@ -120,4 +120,31 @@ public function testHiddenEntities() {
    +     $translation_url = 'admin/structure/config_test/manage/' . $test_entity->id() . '/translate/' . $langcode . '/add';
    +     $this->drupalGet($translation_url);
    +     $edit = array(
    +       "config_names[config_test." . $test_entity->id(). "][label][translation]" => $es_entity_label,
    +     );
    +     $this->drupalPostForm($translation_url, $edit, t('Save translation'));
    +     $this->assertRaw(t('Successfully saved @language translation.', array('@language' => 'Spanish')));
    

    You can also use the language manager to get/set/save the language override. See getLanguageConfigOverride() on the language manager.

  4. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
    @@ -120,4 +120,31 @@ public function testHiddenEntities() {
    +     $this->drupalGet('admin/structure/config_test/entity_using_current_language/' . $test_entity->id());
    +     $this->assertText($es_entity_label);
    

    Is Spanish the site default language in this test?

Status: Needs review » Needs work

The last submitted patch, 12: 2369035-12_TEST_ONLY.patch, failed testing.

balagan’s picture

jhodgdon’s picture

RE #15 - Thanks balagan but that post doesn't answer the question of what they do if you call empty($variable['nonexistent']) on a nonexistent array element. Gabor says it is fine though. :)

robertdbailey’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,093 pass(es), 4 fail(s), and 0 exception(s). View

In response to review points in #13:

#1 & #2 - Nice, fixed.
#3 - Switched. Is that the right idea with setting config overrides?
#4 - Ah right, and I've now switched the test to be French, since that seems to be more common with other tests; so this patch version also creates an admin user, adds French, and then sets it as the default.

Status: Needs review » Needs work

The last submitted patch, 17: 2369035-17_TEST_ONLY.patch, failed testing.

Gábor Hojtsy’s picture

  1. +++ b/core/modules/config/tests/config_test/config_test.routing.yml
    --- a/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
    +++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
    

    The language override feature is a functionality of language module NOT config translation. Config translation is a UI for translations but the API is with language module (and core). The test should be in language module.

  2. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
    @@ -120,4 +120,40 @@ public function testHiddenEntities() {
    +    $this->drupalGet('admin/config/regional/settings');
    +    $edit = array(
    +      'site_default_language' => $langcode,
    +    );
    +    $this->drupalPostForm(NULL, $edit, t('Save configuration'));
    +    $this->rebuildContainer();
    

    Instead of setting the default language and rebuilding the container, you can just request fr/admin/... at the end and get the French page which should result in the French entity.

Gábor Hojtsy’s picture

@robertdbailey: still around?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,440 pass(es), 4 fail(s), and 0 exception(s). View

Rerolled test with fix.

Status: Needs review » Needs work

The last submitted patch, 21: 2369035-21.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Status: Needs work » Needs review
FileSize
6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,399 pass(es), 1 fail(s), and 1 exception(s). View

So implementing my suggestions pretty much required a test rewrite.

1. I looked up where the admin upcast is tested. #2099541: ConfigEntities should not load the Entity translated on Edit Forms introduced that. Unfortunately no dedicated test but a little snippet in the local config test. Since then the feature was moved to the language module also.

2. So I decided we need a dedicated test that both tests the pre-patch only option and the post-patch capability. So a new test in language module was born. I used Rob's base to write that, thanks! Moved the route to language_test and used the configurable language entity type so we have the config entity to work with.

No interdiff because I rewrote almost all lines of the test.

Status: Needs review » Needs work

The last submitted patch, 23: 2369035-22.patch, failed testing.

jhodgdon’s picture

The test looks very straightforward... doesn't quite work though. And eventually we'll probably want to see a test-only patch with the test failing, plus a with-test patch with it passing.

jhodgdon’s picture

Assigned: Gábor Hojtsy » jhodgdon

With Gabor's enthusiastic permission, I'm going to see today if I can get this test working. :)

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
4.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,747 pass(es), 2 fail(s), and 0 exception(s). View
2.81 KB
6.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,767 pass(es). View

I got it working locally. So, here's an interdiff, a test-only patch, and a code-patch-with-test patch.

The test is simple and elegant, just had a few little bugs. The code I am obviously happy with, since I wrote it. :) So, I am +1 on RTBC at this point, but someone else will need to agree (as well as the test bot).

jhodgdon’s picture

Issue summary: View changes

Updated issue summary with beta-eval, not to mention making it into a standard issue summary.

jhodgdon’s picture

Issue summary: View changes

typo

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All the proposed changes look great. Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,749 pass(es). View
1.69 KB

I was thinking about change notices... I don't really think this needs one, but I do think it needs to be documented. So I added a piece to this topic page:
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...

So, one more patch, this one with docs.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me :)

The last submitted patch, 27: 2369035-27-test-only-FAIL.patch, failed testing.

jhodgdon’s picture

Issue tags: -Needs tests

Looks like the test-only patch failed in the expected way, and the with-code patch passed, so I think we're good to go with this!

effulgentsia’s picture

Assigned: jhodgdon » Unassigned

Looks like this was assigned to jhodgdon for authoring, not for commit, so unassigning now that it's RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks all. :-)

  • Dries committed be1661a on 8.0.x
    git commit -m 'Issue #2369035 by jhodgdon, Gábor Hojtsy, robertdbailey:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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