check translate links work, and write tests

I'll do it!

I've manually tested all translate links, here are the results:

1. Block listing: admin/structure/block

   System Help link: admin/structure/block/manage/bartik.help/translate

   Access denied

2. Menu listing: admin/structure/menu

   Administration link: admin/structure/menu/manage/admin/translate

   OK

3. Taxonomy listing: admin/structure/taxonomy

   Tags link: admin/structure/taxonomy/manage/tags/translate

   OK

4. Custom block listing: admin/structure/custom-blocks

   Basic block link: admin/structure/custom-blocks/manage/basic/translate

   OK

5. Contact form listing: admin/structure/contact

   Personal contact form link: admin/structure/contact/manage/personal/translate

   OK

6. Format listing: admin/config/content/formats

   Basic HTML link: admin/config/content/formats/manage/basic_html/translate

   OK

7. Shortcut listing: admin/config/user-interface/shortcut

   Default link: admin/config/user-interface/shortcut/manage/default/translate

   Fatal error: Call to a member function getLanguageWithFallback() on a non-object in .../modules/config_translation/lib/Drupal/config_translation/Access/ConfigNameCheck.php on line 34

8. Role listing: admin/people/roles

   Anonymous user link: admin/people/roles/manage/anonymous/translate

   OK

9. Maintenance settings page: admin/config/development/maintenance

   Translate tab: admin/config/development/maintenance/translate

   OK

10. Site information settings page: admin/config/system/site-information

    Translate tab: admin/config/system/site-information/translate

    OK

11. Account settings page: admin/config/people/accounts

    Translate tab: admin/config/people/accounts/translate

    OK

12. Views listing: admin/structure/views

    • Content link: admin/structure/views/view/content/translate

      OK

    • People link: admin/structure/views/view/user_admin_people/translate

      Access denied

I edited the previous comment to add a second views listing case which response is 403.

I upload the patch for the test to check whether the linked page to translate is correct or not. I check if the response has the html <th>Language</th>

. Is there a better way to identify that the page requested is the appropiate translation page?

The previous patch failed applying so I've had to re-roll it. At least two test should fail: 1 and 7 like in the #3 manual testing.

After the update I've manually re-tested all translate links, here are the results:

1. Block listing: admin/structure/block

   All links

   Access denied

2. Menu listing: admin/structure/menu

   All links

   OK

3. Taxonomy listing: admin/structure/taxonomy

   Tags link: admin/structure/taxonomy/manage/tags/translate

   OK

4. Custom block listing: admin/structure/custom-blocks

   Basic block link: admin/structure/custom-blocks/manage/basic/translate

   OK

5. Contact form listing: admin/structure/contact

   All links

   OK

6. Format listing: admin/config/content/formats

   All links

   OK

7. Shortcut listing: admin/config/user-interface/shortcut

   Default link: admin/config/user-interface/shortcut/manage/default/translate

   Fatal error: Call to a member function getLanguageWithFallback() on a non-object in .../modules/config_translation/lib/Drupal/config_translation/Access/ConfigNameCheck.php on line 34

8. Role listing: admin/people/roles

   All links

   OK

9. Maintenance settings page: admin/config/development/Maintenance

   Translate tab: admin/config/development/maintenance/translate

   OK

10. Site information settings page: admin/config/system/site-information

    Translate tab: admin/config/system/site-information/translate

    OK

11. Account settings page: admin/config/people/accounts

    Translate tab: admin/config/people/accounts/translate

    OK

12. Views listing: admin/structure/views

    • Content link: admin/structure/views/view/content/translate

      OK

    • People link: admin/structure/views/view/user_admin_people/translate

      Access denied

I've found that blocks translate links don't pass the test because the page response is 403 - Access denied, and that is because at the ConfigNameCheck::access method there was a hasSchema and hasTranslatable check. From my point of view this is not the appropriate place to check it because it's not a matter of access. It's just that the translate link or the translate route has no sense if whatever we are viewing is not translatable.

We should move this check to the routing implementation without affecting performance. It's not straightforward to do right know in the hook_menu because the we are managing either ConfigGroupMapper objects, that have the hasTranslatable() method, and ConfigEntityMapper objects, that needs to be loaded to check its translatability.

Also, the config_translation_find_translatable function in config_translation.module, has a @todo comment regarding to update the code of the function, we should make an issue about it.

I'll continue working to fix the error thrown at admin/config/user-interface/shortcut/manage/default/translate

I've found that blocks translate links don't pass the test because the page response is 403 - Access denied, and that is because at the ConfigNameCheck::access method there was a hasSchema and hasTranslatable check. From my point of view this is not the appropriate place to check it because it's not a matter of access. It's just that the translate link or the translate route has no sense if whatever we are viewing is not translatable.

We should move this check to the routing implementation without affecting performance. It's not straightforward to do right know in the hook_menu because the we are managing either ConfigGroupMapper objects, that have the hasTranslatable() method, and ConfigEntityMapper objects, that needs to be loaded to check its translatability.

Also, the config_translation_find_translatable function in config_translation.module, has a @todo comment regarding to update the code of the function, we should make an issue about it.

I'll continue working to fix the error thrown at admin/config/user-interface/shortcut/manage/default/translate

Well, we need to somewhere check if they have schema and are translatable because otherwise the translate link does not make sense on the item. So putting a translate tab on or making a translation operation appear in an operations dropdown in a list will just not lead to anything useful.

We cannot add routes per item, we can only add them wholesale for all items with entity type name wildcards in the path. So the only way to make tabs not appear is if they cannot be accessed. If they can be accessed, they will display, resulting in translate tabs on things that don't make sense.

Eg. you have a single language German website. All the shipped menus coming from Drupal core are English, so they *need* a translate tab. All the menus you created don't make sense to have a translate tab, because you have a single German language website, so you don't have any other language to translate to. It would be UI litter with options you cannot use or do anything with. And then the only way to not make the tab appear on a per item basis is the access check returning no access, in which case, the UI becomes nice.

I think the operations dropdowns should also check for translatability and schema. The trick is schema is per entity type, translatability is per entity. So we need to cache some stuff maybe to make it fast. But a first pass approach at it can be just to do the check in the entity operations alter for now without caching and then we can see if its really slow.

Thanks for the explanation! So I restore the translatability check at the access method, and check the translatability of each block before creating the link at config_translation_form_alter(). So now the link is not created, the test should fail at shortcut translate link check.

I've also re-rolled the patch.

The translate page admin/config/user-interface/shortcut/manage/default/translate throw an error:

Fatal error: Call to a member function getLanguageWithFallback() on a non-object in .../modules/config_translation/lib/Drupal/config_translation/Access/ConfigNameCheck.php on line 34

I fixed by replacing the base path pattern at config_translation_config_translation_group_info, who had {shortcut_es} by putting {shortcut} instead, according to the pattern in shortcut.routing.yml in shortcut module. I think it might be a mistake made because of the label (@label shortcut set).

The tests should all pass now

Need review!

I left an old line when I re-rolled the patch that made tests fail, I've fixed it.

I also add to the test the same condition before testing the block link and route: is this block translatable?

+++ b/config_translation.moduleundefined
@@ -235,7 +235,7 @@ function config_translation_config_translation_group_info() {
   // Shortcut.
-  $items[] = new ConfigEntityMapper('admin/config/user-interface/shortcut/manage/{shortcut_set}', 'shortcut', t('@label shortcut set'));
+  $items[] = new ConfigEntityMapper('admin/config/user-interface/shortcut/manage/{shortcut}', 'shortcut', t('@label shortcut set'));
 

Ha, good find!

+++ b/config_translation.moduleundefined
@@ -284,7 +284,7 @@ function config_translation_form_alter(&$form, &$form_state, $form_id) {
-      if (is_array($block) && isset($block['operations'])) {
+      if (is_array($block) && isset($block['operations']) && config_translation_has_translatable('block.block.' . $id)) {
         $block['operations']['#links']['translate'] = array(
           'title' => t('Translate'),
           'href' => 'admin/structure/block/manage/' . $id . '/translate',
diff --git a/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php

diff --git a/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php
index 0c5de4c..fd1ca44 100644

index 0c5de4c..fd1ca44 100644
--- a/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php

--- a/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.php
+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.phpundefined

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.phpundefined
+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.phpundefined
@@ -81,12 +81,20 @@ class ConfigTranslationListUITest extends WebTestBase {

@@ -81,12 +81,20 @@ class ConfigTranslationListUITest extends WebTestBase {
     $block_machine_name = Unicode::strtolower($this->randomName(16));
     $this->drupalPlaceBlock('system_powered_by_block', array('machine_name' => $block_machine_name));
 
-    // Get the Block listing.
-    $this->drupalGet('admin/structure/block');
+    // Check if the block is translatable.
+    if (config_translation_has_translatable('block.block.stark.' . $block_machine_name)) {
 
-    $translate_link = 'admin/structure/block/manage/stark.' . $block_machine_name . '/translate';
-    // Test if the link to translate the block is on the page.
-    $this->assertLinkByHref($translate_link);
+      // Get the Block listing.
+      $this->drupalGet('admin/structure/block');
+
+      $translate_link = 'admin/structure/block/manage/stark.' . $block_machine_name . '/translate';
+      // Test if the link to translate the block is on the page.
+      $this->assertLinkByHref($translate_link);
+
+      // Test if the link to translate actually goes to the translate page.
+      $this->drupalGet($translate_link);
+      $this->assertRaw('<th>Language</th>');
+    }
   }

Ok, we don't have the same translatability check on any of the other entities. In the operations alter hook for example.... There are two things we should note about this:

1. Blocks should be translatable in core. We should not need this workaround if the blocks are properly written in core.

2. We should do the translatability checking anyway on all entities and have specific test coverage creating non-translatable entities for that. I think this point should be a followup.

So that the blocks are not translatable seems to be a bug to me. Don't work around that!

Thank you. I'm agree with you regarding the blocks workaround, I didn't do it at hook_entity_operation_alter because it hasn't work for blocks yet #2027857: Blocks operations cannot be altered. So checking translatability for all entities using that hook and providing tests for that, could be a different issue, depending on the "fixing the hook" issue. Should we open a issue for that? Or should we postpone the current issue?

I've removed the blocks translatability workaround so the test fail for this translate page example. Anything else I should do for the moment?

I've forgotten to switch it to needs review

#21: config_translation-check_translate_links_work-2029029-21.patch queued for re-testing.

Sent for retest due to #2031255: Update for new languages as config entities.

It would be great to:

- comment out the block thing because it is a core related fail; add a @todo on the core patch (if there is one) that deals with it
- add a commented out language test (language has a bug with operations altering too, see the code and @todo added in #2031207: Create EntityMapper for language config entity)

So we would have two @todo's added with this. That is fine for now.

I've just commented the block tests function.

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.phpundefined
@@ -78,6 +74,7 @@ class ConfigTranslationListUITest extends WebTestBase {
   protected function doBlockListTest() {
+    /*
     // Add a test block, any block will do.
     // Set the machine name so the translate link can be built later.
     $block_machine_name = Unicode::strtolower($this->randomName(16));
@@ -89,6 +86,10 @@ class ConfigTranslationListUITest extends WebTestBase {

@@ -89,6 +86,10 @@ class ConfigTranslationListUITest extends WebTestBase {
     $translate_link = 'admin/structure/block/manage/stark.' . $block_machine_name . '/translate';
     // Test if the link to translate the block is on the page.
     $this->assertLinkByHref($translate_link);
+
+    // Test if the link to translate actually goes to the translate page.
+    $this->drupalGet($translate_link);
+    $this->assertRaw('<th>Language</th>');*/

Do not comment out the whole method. Just comment the new part. The old part works. Also ensures once we remove the ugly altering for it, it still works.

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationListUITest.phpundefined
@@ -135,6 +136,10 @@ class ConfigTranslationListUITest extends WebTestBase {
+
+    // Test if the link to translate actually goes to the translate page.
+    $this->drupalGet($translate_link);
+    $this->assertRaw('<th>Language</th>');

I looked hard if we can have something more useful to assert for, but does not seem to be. We should test for '

' though (make language translate as applicable).

Same at all places where this is applied.