Config entities should not always be untranslated in admin routes

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

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

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

Robert: sure, I'll review :)

empty() is fine on undefined array elements.

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?

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?

Regarding empty() vs isset()
https://www.virendrachandak.com/techtalk/php-isset-vs-empty-vs-is_null/

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.

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.

@robertdbailey: still around?

Rerolled test with fix.

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.

All the proposed changes look great. Thanks!

Looks good to me :)

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

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

Yay, thanks!