Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The UI displays Translate operation links for untranslatable entities, which results in a HTTP 404.
Steps to reproduce
Proposed resolution
Check if an entity is translatable before displaying the Translate link.
Remaining tasks
Determine if this is still a problem.
User interface changes
Links may be hidden if they are not applicable.
API changes
None
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#43 | 2140925.patch | 880 bytes | DieterHolvoet |
#42 | 2140925.patch | 1.47 KB | DieterHolvoet |
#33 | drupal_2140925_33.patch | 1.56 KB | Xano |
#27 | config_translation_entity_operation_alter-2140925-27.patch | 1.59 KB | webflo |
Comments
Comment #1
XanoComment #3
Xano1: drupal_2140925_1.patch queued for re-testing.
Comment #4
tstoecklerThis makes a lot of sense. Instead of checking for the existence of 'config_prefix', however, I think we should simply check for $entity instanceof ConfigEntityInterface. Thoughts?
Comment #5
XanoAs it has been decided that config entity storage isn't swappable after all, every entity type needs to implement ConfigEntityInterface and have a config prefix, so either solution should work. Checking for ConfigEntityInterface is cleaner, but may not communicate how the code works as much as checking for the prefix does.
Comment #6
XanoWait, no. We can't just do either. Not all config entities are translatable. We need the config prefix to construct the filename of the config entity, which is needed to check if the entity itself is translatable.
I would very much like to put this piece of code in a method somewhere, because #1839516: Introduce entity operation providers needs it too.
Comment #8
tstoecklerSorry, I didn't look closely enough before. This should be $mapper->hasTranslatable() (which checks for the appropriate config names itself). Therefore the instanceof check should be sufficient.
Comment #9
tstoecklerOh wait. We don't have $mapper. Hmm... Well we should certainly add a method to ConfigMapperManager to get the mapper for an entity. I'll see if I can cook something up tomorrow.
Comment #10
XanoComment #11
tstoecklerOhh, it's so wrong that we use the entity type as the plugin ID. We changed to making the plugin ID equal the base route name, but apparently forgot entity mappers.
This is now missing the instanceof check. Even better than that, however, we should simply be checking whether the mapper exists. E.g. if field UI module is disabled, for fields you wouldn't get a proper mapper, but simply FALSE. (I guess hook_operations_info() wouldn't be run on fields in that case, but still...)
Comment #12
XanoComment #13
tstoecklerLooks good. Xano pointed out that createInstance() doesn't just return FALSE, but throws an exception. That's why we need to go the detour of checking getDefinition() first.
Comment #15
Xano12: drupal_2140925_12.patch queued for re-testing.
Comment #17
tstoeckler12: drupal_2140925_12.patch queued for re-testing.
Comment #19
tstoecklerSo @webflo smartly points out that this cannot work, as the mapper does not know about the specific entity, but only about the entity type. We need to initialize the entity-specific information with setEntity() first. Tests++
Comment #20
Gábor HojtsyIdeally this code would do the same access checking that we have on the route or at least consider that possibility. Eg if the config is in undefined language, it is not translatable either. That may have UX implications, eg. confusion to users that some entities in a list have a translate buttons, others don't. So all considered with UX this patch may be better at the end.
Comment #21
XanoWe should move the access check to
hook_entity_access()
so it's reusable through entities' access controllers. Once #1839516: Introduce entity operation providers is in, this is a requirement for operation link access control anyway.Comment #22
Gábor HojtsySounds like that would need some refactoring, since not all config are entities, so we need to delegate the access checking through the config mapper then which can rely on entity access in the entity mapper then :)
Comment #23
XanoWell, this issue is about the entity operation only. Does this access check go wrong elsewhere for non-entities too?
Comment #24
Gábor HojtsyNo. But to factor out the access checking in the route access class to an entity access hook, we'll need to then re-integrate it in the route access class (via the config mapper).
Comment #25
XanoI assume you are talking about
\Drupal\config_translation\Access\ConfigTranslationOverviewAccess
and/or\Drupal\config_translation\Access\ConfigNameCheck
? I am not sure what the problem is you are describing, but can't it be solved by using multiple access requirements on routes? One can be the entity access check, whereas the other points to these classes.Comment #26
Gábor HojtsyRight, we need a combination of access checks, which is what I am saying. The route access depends on the entity access as well as dynamic path components (eg target language of the translation). So all I am saying it needs to delegate parts of the access check to avoid code duplications.
Comment #27
webflo CreditAttribution: webflo commentedThe plugin id for field instance is not field_instance (thanks to config_translation_config_translation_info). Fiel instance translation depends on the parent entity type.
Comment #28
Gábor HojtsyLooks like this may not be very extensible then... Eg. view modes and form modes would also depend on the parent entity?
Comment #29
tstoecklerSo, I was about to RTBC this, when I saw the discussion above:
@Xano/@Gábor Hojtsy: So what's the proposed solution, that is not really clear to me from the above?
Comment #30
tstoecklerI just reviewed #1839516: Introduce entity operation providers and now understand #20 - #26. I will post an updated patch that implements entity access if no-one beats me to it.
Comment #31
Xano27: config_translation_entity_operation_alter-2140925-27.patch queued for re-testing.
Comment #33
XanoRe-roll.
Comment #35
Gábor HojtsySo now the block and field translation links that are looked for in the test are not found.
Comment #42
DieterHolvoet CreditAttribution: DieterHolvoet as a volunteer commentedRe-roll
Comment #43
DieterHolvoet CreditAttribution: DieterHolvoet as a volunteer commentedRe-rolled patch isn't working anymore, I updated the patch to check access using the url object.
Comment #50
quietone CreditAttribution: quietone at PreviousNext commentedIs this still a problem? All discussion here happened for Drupal 8. Has this been fixed in the meantime?
The issue summary does not have specific steps to reproduce so it is not clear what entities have this problem. Adding tag for steps to reproduce.
Since we need more information to move forward with this issue, I am setting the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks
Comment #51
quietone CreditAttribution: quietone at PreviousNext commentedSince no additional information has been supplied I am closing this issue.
If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").
Thanks!