Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#15 | withoutadminpermission.png | 147.38 KB | YesCT |
#15 | editsshow.png | 198.07 KB | YesCT |
#8 | 2035433-origin-translation-edit-8-test_only.patch | 1.19 KB | vijaycs85 |
#8 | 2035433-origin-translation-edit-8.patch | 2.52 KB | vijaycs85 |
#6 | 2035433-origin-translation-edit-6.patch | 1.33 KB | vijaycs85 |
Comments
Comment #1
Gábor HojtsyOr figure out if we can get an access check on the edit route via the routing system.
Comment #2
vijaycs85Initial patch...
Comment #3
vijaycs85Working 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.
Comment #4
vijaycs85If we agree this approach, we might need to have some test cases to make sure we don't have 403 edit.
Comment #5
Gábor HojtsyLooks 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.
origin => original :)
Comment #6
vijaycs85Thanks 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.
Comment #7
Gábor HojtsyI 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.
Comment #8
vijaycs85@Gábor Hojtsy, When I try to do assignment and check in same if condition, getting fatal error:
But assigning it above works fine.
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.
Comment #10
Gábor HojtsyBoth 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?
Comment #11
vijaycs85@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
Comment #12
Gábor HojtsyI don't get this, sorry. Unfortunately don't have time to look deeper soonish.
Comment #13
Gábor HojtsySo I managed to catch some time to look at this after all :) The Edit tab is added in:
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.
Comment #14
YesCT CreditAttribution: YesCT commentedNoting 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.
Comment #15
YesCT CreditAttribution: YesCT commentedwithout 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.
If someone with permission to translate does have the url given to them by their boss or something, they see:
(with no patch)
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.
Comment #16
pwolanin CreditAttribution: pwolanin commentedWe need to take advantage of this change: #2046737: Add a method to the AccessManager that only needs a route name and parameters
Comment #17
pwolanin CreditAttribution: pwolanin commentedOh, 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.
Comment #18
YesCT CreditAttribution: YesCT commentedcan we make translate a root tab? does that make sense?
or, when we add translate, also add a view root tab?
Comment #19
Gábor Hojtsy@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).
Comment #20
YesCT CreditAttribution: YesCT commentedSo. #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?)
Comment #21
Gábor HojtsyIMHO 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.
Comment #21.0
Gábor HojtsyI dont think api changes will be needed. taking out the default text from the issue summary template.