Updated: Comment #0

Problem/Motivation

To get ready for getting in core, address any @todo's.

When looking at the translation table, there is an edit on the original source.

In ConfigTranslationController.php

    foreach ($languages as $language) {
      if ($language->id == $original_langcode) {
        $page['languages'][$language->id]['language'] = array(
          '#markup' => '<strong>' . t('@language (original)', array('@language' => $language->name)) . '</strong>',
        );

        // @todo The user translating might as well not have access to
        // edit the original configuration. They will get a 403 for this
        // link when clicked. Do we know better?
        $operations = array();
        $operations['edit'] = array(
          'title' => t('Edit'),
          'href' => $path,
        );
        $page['languages'][$language->id]['operations'] = array(
          '#type' => 'operations',
          '#links' => $operations,
        );
      }

Proposed resolution

Do not show the edit for the content (which is different than the edit on translations). How?

Or ?? rename the edit on the source to: edit source. This might help translators realize there is a different level of permissions when they try it and get a denied message.

Remaining tasks

  • Discuss

User interface changes

TBD

API changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Or figure out if we can get an access check on the edit route via the routing system.

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.33 KB

Initial patch...

vijaycs85’s picture

FileSize
18.77 KB

Working as expected. Confirmed by a new user and assigned the role 'translator' which has just 'translation configuration' permission. Here is the screenshot of this user viewing site information page.

vijaycs85’s picture

Issue tags: +Needs tests

If we agree this approach, we might need to have some test cases to make sure we don't have 403 edit.

Gábor Hojtsy’s picture

Looks good. Not sure how long will the menu_* API will last though. Is this not deprecated?

1. We should put the two if()s on one line IMHO.

2.

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -72,14 +72,16 @@ class ConfigTranslationController implements ControllerInterface {
+        // Only allow users with permission to edit origin configuration.

origin => original :)

vijaycs85’s picture

Thanks for the review @Gábor Hojtsy. No menu_get_item() is not deprecated (Yet :))

#5.1 NOT FIXED: throwing 'undefined variable' error
#5.2 - FIXED.

Gábor Hojtsy’s picture

I do not see how merging the two its behave differently if joined by &&. It would be great to have the tests back running (I assume something committed made the tip of the branch fail) and then see a version with merged ifs.

vijaycs85’s picture

@Gábor Hojtsy, When I try to do assignment and check in same if condition, getting fatal error:

        if ($router_item = menu_get_item($path) && $router_item['access']) {

But assigning it above works fine.

        $router_item = menu_get_item($path);
        if (isset($router_item) && $router_item['access']) {

Regading test, Since we have the 'edit' as tab (can see in the screenshot at #3), not sure how to assert just edit in table. However the best fix is to remove 'edit' tab as well. I tried and couldn't get where that tab coming from.

So both patch will have 1 fail.

Status: Needs review » Needs work

The last submitted patch, 2035433-origin-translation-edit-8.patch, failed testing.

Gábor Hojtsy’s picture

Both our mapper constructors have arguments to set if they need edit tabs added, and the menu hook in the module adds the tab as needed. In case core already has the tab, it does not add one. The edit tab is added with inheritance for access I believe, but I don't have code access to check ATM. So it should already be hidden if no access, no?

vijaycs85’s picture

@Gábor Hojtsy, i have checked hook_menu and no where we are setting addEditTab and it is by default FALSE. doesn't look like it is mapper's addEditTab

Gábor Hojtsy’s picture

I don't get this, sorry. Unfortunately don't have time to look deeper soonish.

Gábor Hojtsy’s picture

So I managed to catch some time to look at this after all :) The Edit tab is added in:

function config_translation_menu() {
   // ...
    if ($group->needsEditTab()) {
      // For pages that do not have a default tab, we need a default local task
      // on this level, so that the translate tab will show up.
      // @todo This is not very compatible with possible other alters. Look
      //   into other clever ways to make this happen,
      //   http://drupal.org/node/2011332.
      $items[$path . '/edit'] = array(
        'title' => 'Edit',
        'type' => MENU_DEFAULT_LOCAL_TASK,
        'weight' => -100,
      );
    }
   // ...
}

I don't think its even possible to have the Translate tab and NOT have the Edit tab, because if there is only one tab, that needs to be the default tab, and on editing site information, the translate tab will obviously not be the default one. Also since the edit tab does not have any access info, it is just inheriting it (see code above). So it should automatically disappear if there is no access(?). Maybe the menu system does not make tabs disappear if they are the default tab, even if you don't have access? That may be a menu system bug. I don't see how we would be able to make it go away anyway? We practically have no idea about the parent access setup, so we need to rely on it being inherited. If its not inherited proper, that is a menu system bug.

YesCT’s picture

Noting that the issue seen in the comments in #13 is closed, we took out the comment, and decided not to do a alter for it.

YesCT’s picture

FileSize
198.07 KB
147.38 KB

without admin (edit) permissions on settings/config, it's almost impossible to navigate to the page where a translator might want to go to translate config.

withoutadminpermission.png

If someone with permission to translate does have the url given to them by their boss or something, they see:

(with no patch)
editsshow.png
The edit tab does show.

@Gábor Hojtsy says this is menu system bug.

I stopped Crell and talked with him. He had some idea about not using local tasks (tabs) and instead making a parallel sibling menu item, just for translate. And in the admin/config pages, show the translate link if the translator user did not have edit, but had translate permission. And hide the translate link if the user had edit permission. But then... we got confused and Crell said we should talk to @pwolanin

It's an existing problem in d7 that if a default tab has more restrictive permissions than another tab, it is still shown.

At this point, in order for a translator to be able to find stuff to translate, they need edit (admin) permissions.

Maybe we could add a different default tab when the user does not have permission? Is that even possible? Like a "View" tab.

With the patch applied... I can still see the edit operation on English... hm.

pwolanin’s picture

pwolanin’s picture

Oh, maybe I'm misunderstanding the issue

in D8 now we can basically have multiple "default" tabs now - by having more than one root tab appear on the same route.

YesCT’s picture

can we make translate a root tab? does that make sense?
or, when we add translate, also add a view root tab?

Gábor Hojtsy’s picture

@YesCT: I don't think we can make multiple default/root tabs, at least that was not possible in Drupal 7. How would Drupal pick which one to show as default when both are accessible. We also cannot make it a root tab conditionally, as we set the menu in a generic way and then access turns out per user.

To make these pages discoverable for those not having edit permissions to originals, we can/should do an index page with all translatables. That page may run pretty big, it would need to list all block placements, views, content types, vocabularies, menus, etc. Maybe we need something like the new (currently in core) blocks UI where you have a filter to look for your stuff. (Or like the modules page or tests page which allows you to filter down).

YesCT’s picture

So. #2085907: Ensure all configuration entities have an Edit/Configure tab by default added edit tabs in core. We dont add them. We cannot hide them. core should hide tabs users do not have permission to see. #2095137: When default tabs are more restrictive than other tabs, users with lower permissions see default tab and get permission denied

Also, it would be nice if core added edit tabs on config pages that are not config entities. (They should be not shown if there are no other tabs, similar to in 2085907.) #2095117: Menu system should provide a default tab if none exists

What status is this? closed wont fix (because it will be fixed otherwhere?)

Gábor Hojtsy’s picture

Status: Needs work » Closed (works as designed)

IMHO works as designed because core does not have the solution to hide those tabs... So *our* stuff works as designed. If there is a bug it is not in our control.

Gábor Hojtsy’s picture

Issue summary: View changes

I dont think api changes will be needed. taking out the default text from the issue summary template.