Problem/Motivation

Entity operations delegates URI generation to entities at all times and therefore generate links to the original language variant of the entity at all times. This is a problem both for content and config entities. (1) for config entities operations always switch language to the original language of that particular entity (2) for content entities, operations links on translations are never generated, translations also return links to the original language variant. This results in the following confusing/incorrect UX:

Config entity operations:

Content entity operations:

This may cause unintended data loss with the delete operations for example if the user does not read the confirmation form carefully. Trying to delete a translation of a content entity will lead to deleting the whole entity and only the confirmation form stops the user.

Additionally, the content entity edit form natively handles translations, but it is not accessible by users having only translation permissions, hence the related edit links are not displayed in that case.

Proposed resolution

  1. Config entity operations should avoid explicitly forcing a language and should fall back on the request language.
  2. Content entity operations should use the proper language of the translation to be handled. This works for edit and delete links.
  3. Delete on the original language still deletes the whole entity, for that to not happen, we need #2443991: Allow default_langcode field value to be changed, not to be resolved here.
  4. However edit/delete on content entities will be accessible only by users having update access on the entity. So additionally, make the Content Translation module provide two more operations (and the corresponding Views entity link handlers): "edit translation" and "delete translation", which will point to the translation form and the translation deletion form provided by CT. These will be accessible only by users having translation permissions and not having update access on the selected entity.

Remaining tasks

Discuss. Write tests.

User interface changes

  1. Config entity operations will keep the current page language in the links (instead of original language of entity)
  2. Content entity operations point to the right translation instead of always to the original language of entity
  3. Translators who don't have node admin permissions can still edit and delete translations thanks to new operations

API changes

None foreseen at the moment.

CommentFileSizeAuthor
#79 2476563-entity-operations.interdiff.77-79.txt930 bytespenyaskito
#79 2476563-entity-operations-79.patch17.64 KBpenyaskito
#7 2476563.patch6.94 KBamateescu
#11 log-node_admin-1.pdf46.72 KBplach
#11 log-node_admin-2.pdf53.97 KBplach
#22 2476563-22.patch6.08 KBamateescu
#22 interdiff.txt2.47 KBamateescu
#36 2476563-36.patch3.58 KBamateescu
#39 2476563-39.patch3.68 KBGábor Hojtsy
#39 interdiff.txt2.28 KBGábor Hojtsy
#40 2476563-40.patch13.61 KBGábor Hojtsy
#40 interdiff.txt9.93 KBGábor Hojtsy
#44 2476563-44.patch16.6 KBGábor Hojtsy
#44 interdiff.txt2.99 KBGábor Hojtsy
#46 2476563-46.patch5.87 KBGábor Hojtsy
#46 interdiff.txt11.93 KBGábor Hojtsy
#48 2476563-48.patch6.98 KBGábor Hojtsy
#48 interdiff.txt1.12 KBGábor Hojtsy
#49 interdiff.txt3.28 KBGábor Hojtsy
#49 2476563-49-test-only.patch3.28 KBGábor Hojtsy
#49 2476563-49.patch10.26 KBGábor Hojtsy
#51 2476563-51.patch12.95 KBGábor Hojtsy
#51 interdiff.txt2.69 KBGábor Hojtsy
#53 2476563-53.patch14.12 KBGábor Hojtsy
#53 interdiff.txt3.96 KBGábor Hojtsy
#58 2476563-58.patch14.17 KBpenyaskito
#62 2476563-entity-operations.interdiff.58-62.txt6.47 KBpenyaskito
#62 2476563-entity-operations-62.patch17.57 KBpenyaskito
#68 2476563-entity-operations.interdiff.63-68.txt2.69 KBpenyaskito
#68 2476563-entity-operations-68.patch17.53 KBpenyaskito
#73 2476563-entity-operations.interdiff.68-73.txt1.9 KBpenyaskito
#73 2476563-entity-operations-73.patch17.64 KBpenyaskito
#77 2476563-entity-operations-77.patch17.56 KBpenyaskito
#77 2476563-entity-operations.interdiff.73-77.txt2 KBpenyaskito
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

stefan.r’s picture

Just quoting what @plach said in the parent issue to provide some context on what we want here:

Fix operation links to deal with translations: For regular links like view or edit, we just need to use the translation language to trigger a language switch, this would make the user reach the related page in the expected language, while on monolingual sites this would have no effect. The deletion link is a bit trickier: I think the best solution would be make Content Translation swap the plugin class and replace it with a specialized one able to distinguish between original and translation: in the former case the link route would stay the same, in the latter it would change the route of CT's translation deletion page.

@dawehner do you agree with this approach? It's just the edit and delete operations that we need to address, I don't think there are any other ones?

plach’s picture

Issue tags: +language-content

If we are ok with the proposed plan I can work on this.

catch’s picture

How do you delete the actual entity if the delete links are for translations?

What happens if you delete the source translation?

stefan.r’s picture

Guessing for now we can't do this, in the parent issue there was talk of being able to delete the source translation without deleting the entity as a non-critical followup outside of the scope of this issue and the parent. In such case we'd probably have to ask the user which language to reassign the node to.

dawehner’s picture

I would have expected to have 2 operations available: "delete %entity" and "delete %entity translation in %language",
but I see the point that the user might thing, delete points to deleting the translation for itself.

For the bulk operation case we already will need to ask the user directly, whether to remove the entity or the translation for the entities which will be removed,
see \Drupal\node\Form\DeleteMultiple. Given that I guess we want to have the exact same behaviour for the delete route as well.

Given that translations are a thing which exists outside of the context of content_translation (they are part of entity API),
I think adapting the generic form, is one way forward.

The deletion link is a bit trickier: I think the best solution would be make Content Translation swap the plugin class and replace it with a specialized one able to distinguish between original and translation: in the former case the link route would stay the same, in the latter it would change the route of CT's translation deletion page.

About which plugin do we talk about? \Drupal\views\Plugin\views\field\EntityOperations?

The code which adds the entity operation links is the following in \Drupal\Core\Entity\EntityListBuilder::getDefaultOperations

    if ($entity->access('delete') && $entity->hasLinkTemplate('delete-form')) {
      $operations['delete'] = array(
        'title' => $this->t('Delete'),
        'weight' => 100,
        'url' => $entity->urlInfo('delete-form'),
      );
    }

Can't we use the alter hook available above? $this->moduleHandler->alter('entity_operation', $operations, $entity);
in order to change the existing delete url info object?

amateescu’s picture

Title: Make entity operations translation aware » [PP-1] Make entity operations translation aware
Priority: Major » Critical
Status: Active » Postponed
Issue tags: +D8 Accelerate
FileSize
6.94 KB

This patch fixes the entity operations handler to be translation aware, but, like #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual, it depends on #2428103: String formatter should link to its translation. which fixes $entity->urlInfo() to return the correct url.

As proposed in #2473989-40: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual, this is the actual critical issue and that one should be demoted to major.

webchick’s picture

Issue tags: +blocker
Gábor Hojtsy’s picture

I think ideally operations and bulk operations should either work in an entity level mode or a translation level mode (configured on the view?!). Exposing both entity and translation level operations in the dropdown sounds like a bit too much. You would have two of each actions? Also if you filter a view to original languages of entities only, you would only get one item per entity and then what should be in the operations? We should either let the view configure what level the operations should affect or make it configurable AND exposable. I can easily imagine use cases where you exclusively want entity level operations only (where the page makes it clear you are acting on entities) and where you only want translation level operations.

@catch: When you delete the original translation of an entity, the entity API should have provisions already to pick another original language for that entity I believe.

dawehner’s picture

Added a subissue for the bulk operations side of the problem space #2484037: Make Views bulk operations entity translation aware

plach’s picture

FileSize
46.72 KB
53.97 KB

@catch: When you delete the original translation of an entity, the entity API should have provisions already to pick another original language for that entity I believe.

I'm really not sure about that, at least I don't recall ever coding such logic.

Posting a couple of chat logs for future reference.

webchick’s picture

Issue summary: View changes

Updating issue summary to make it clear that this is blocked on #2428103: String formatter should link to its translation..

Gábor Hojtsy’s picture

@plach: oh, what happens now if you delete the original language translation of something then? You get refused with an exception?

amateescu’s picture

what happens now if you delete the original language translation of something then? You get refused with an exception?

Nope, the entity and all its translations are deleted :/

stefan.r’s picture

And to make this even more confusing, depending on which part of the user interface you use, in some places the button/option is hidden on the source language, in other places it deletes the whole entity (even if you only clicked the "Delete (this translation)" button), and in other places it throws an error, if I remember correctly.

I couldn't find any code that would allow us to pick another default language for an entity, ContentEntityBase::removeTranslation() explicitly disallows this as well:

    if ($langcode == $this->defaultLangcode) {
      $message = 'The specified translation (@langcode) cannot be removed.';
      throw new \InvalidArgumentException(SafeMarkup::format($message, array('@langcode' => $langcode)));
    }
but I see the point that the user might thing, delete points to deleting the translation for itself.

@dawehner I guess we can avoid that by using different verbiage?

stefan.r’s picture

@gabor:

I think ideally operations and bulk operations should either work in an entity level mode or a translation level mode (configured on the view?!). Exposing both entity and translation level operations in the dropdown sounds like a bit too much. You would have two of each actions? Also if you filter a view to original languages of entities only, you would only get one item per entity and then what should be in the operations?

Yes that would make sense. Or alternatively, just to get back to what was discussed in #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual: As "operations" is just edit and delete, and edit is always one language at a time we can just have 3 operations on the UI level (once we allow people to delete separate translations without deleting the whole entity):

edit (french) / delete (french) / delete (all languages)

...where it doesn't matter if the view is filtered by language or not.

Similarly, for bulk operations we could have a dropdown with:

  • publish french
  • publish all languages
  • unpublish french
  • unpublish all languages
  • delete french
  • delete all languages
  • ...

Or even two dependent dropdowns where the leftmost one is a list of bulk operations and the rightmost one says "all languages", and optionally "french" (if we're filtering by that and translation-level operations are available for the selected bulk operation on the left).

And in case of the deletion of a an original language version, in the confirmation screen we can ask the user for every individual affected entity what the new default language should be (with dropdowns or radios).

dawehner’s picture

publish french
publish all languages
unpublish french
unpublish all languages

I had the hope that we could make them more intelligent so depending on the context (source language vs. non source language) do a different action.

stefan.r’s picture

The assumption here was that it shouldn't matter whether something is in a source language or not.

Can you clarify with an example?

dawehner’s picture

Well, you go to admin/content and see both the source and the translatations, so if you delete for example the translation it should just remove the translation. This is what I think about contextual different behaviour of the links. Similar could be applied to publish/unpublish, but IMHO this is not a critical issue, or at least less of a.

stefan.r’s picture

@dawehner I think we're actually on the same page here and I may just not have explained correctly. I was talking outside of the current possibilities of Drupal and outside of just the scope of this issue. Basically my point was to let this behave like Drupal 7, and for every operation / bulk operation have the option to apply them on either on the translation level, to all different translations and the source translation, or on the entity level. In the UI, for the latter 2 options all we would say is "all languages".

In such case it really shouldn't matter whether we're looking at the source translation for an entity or a translated version, as the behavior will be the same regardless, and there would be no need for the list of operations to be different.

So even if we have a language filter with the following options:

  • all languages
  • source translations only
  • english
  • french
  • spanish

And we filter by "french", and we select 3 french rows (where 1 is a french non-source translation and 2 are french source translations), we'd still want to have a bulk operation that allows for unpublishing everything, ie. the 3 selected french rows and all their translations, as well as having a bulk operation that unpublishes just the selected versions. Similarly for deletion.

@Gabor's point seemed to be that the list of bulk operations would get too long, but then we could still split the dropdown into 2 dropdowns, 1 for the name of the bulk operation and 1 for whether it applies to just the language we're filtering on or to all languages.

webchick’s picture

amateescu’s picture

Issue tags: -blocker
FileSize
6.08 KB
2.47 KB

I was working on finishing this before it got demoted, so here's a patch that fixes the "criticalness" of this issue: deleting an entity translation results in deleting the whole entity. By the way, this happens even in normal site operation (e.g. going to the delete form of an entity translation), not only through views, so maybe we should bump it back since it's still an unexpected data loss problem?

Still depends on #2428103: String formatter should link to its translation. so leaving postponed for the moment.

stefan.r’s picture

We discussed this on IRC and that interdiff in #22 should get its own critical issue :)

plach’s picture

Issue summary: View changes

Updated the IS summary with the proposed solution for this particular issue I discussed with @amateescu and @dawehner in IRC.

Can we move the discussion about bulk operations to #2484037: Make Views bulk operations entity translation aware? I agree that the UX implied by both issues should be consistent, but it's a bit confusing that we are mainly discussing the other issue here.

plach’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes
stefan.r’s picture

Filed a followup to discuss removal of source translations in #2485499: Allow source translations to be removed

plach’s picture

Thanks :)

webchick’s picture

Status: Postponed » Needs review

#2428103: String formatter should link to its translation. got committed, this should be unblocked now.

webchick’s picture

Title: [PP-1] Make entity operations translation aware » Make entity operations translation aware
webchick’s picture

Issue summary: View changes

Sigh. One more. Sorry for the noise. :(

The last submitted patch, 7: 2476563.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2476563-22.patch, failed testing.

amateescu’s picture

webchick’s picture

Woohoo #2486177: Deleting an entity translation from the UI deletes the whole entity is in. :) Would still love to see this one fixed, though. It's pretty nasty.

amateescu’s picture

Component: entity system » views.module
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
3.58 KB

I wrote this patch a few days ago but I didn't get to post it due to high fever and stuff :( It works but it needs tests, so if anyone wants to work on it, please do.

Edit: (cont.) because I won't be able to do much for a few more days.

webchick’s picture

Oh no. :( Feel better!!

Status: Needs review » Needs work

The last submitted patch, 36: 2476563-36.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
2.28 KB

#2504663: Entity operations links tied to original entity language, ignore both views row and UI languages is a duplicate, so trying to integrate progress from there here.

1. Add missing phpdoc to cosntructor.
2. Use the methods on the trait for less code :)
3. Keep comment on why no call to parent::query().

Gábor Hojtsy’s picture

Title: Make entity operations translation aware » Entity operations links tied to original entity language, ignore everything else
Component: views.module » base system
Issue tags: +sprint
FileSize
13.61 KB
9.93 KB

The problem also spans way beyond the views operations. It applies to all config entities. I opened #2504663: Entity operations links tied to original entity language, ignore both views row and UI languages based on what @webchick found in the listing of views config entities. Since that is considered a duplicate, integrating those changes here too.

My idea for that is basically all config entity operations need to ignore the language of the entity. We can either fix this directly in the entity URL methods (making them behave differently for config entites) OR each getDefaultOperations() method (which is what I did). This solves the problem of "I'm looking at my menus admin page in Spanish but actually going to edit a menu appears in English, French, etc. base on the menu itself and not my UI language".

The last submitted patch, 39: 2476563-39.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: 2476563-40.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes

Dramatically updated issue summary. Hopefully brought over all concerns that were originally there.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
16.6 KB
2.99 KB

Fixing all the tests.

amateescu’s picture

Thanks, Gábor!

We can either fix this directly in the entity URL methods (making them behave differently for config entites) OR each getDefaultOperations() method

Given that users are interacting with config entities in a different way than with content entities (i.e. admin interface for config vs. user-facing interface for content), I think it's ok to hardcode a different behavior in the config entity URL methods :)

Gábor Hojtsy’s picture

FileSize
5.87 KB
11.93 KB

@amateescu: that would mean a direct change in ConfigEntityBase::urlInfo(). Here you go.

Status: Needs review » Needs work

The last submitted patch, 46: 2476563-46.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.98 KB
1.12 KB

Updating test expectations then.

Gábor Hojtsy’s picture

Test for the config entity changes and test only version for them.

The last submitted patch, 49: 2476563-49-test-only.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
12.95 KB
2.69 KB

Expanding on FieldEntityOperationsTest with language variance, no entity translations yet.

Status: Needs review » Needs work

The last submitted patch, 51: 2476563-51.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
14.12 KB
3.96 KB

Adding Spanish translations to all entities in the test. Now this will not yet work of course because the view works off of the entity_test table, not entity_test_data. But if I try to make it work off of the data table, there is of course no data table. But if I make the EntityTest entity define a data table, its views data altering code gets confused, etc.

So not sure how best to make this work without making a round of unrelated changes for entity_test :( We can make this test/view use nodes instead?

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Let's test that too.

Status: Needs review » Needs work

The last submitted patch, 53: 2476563-53.patch, failed testing.

jhodgdon’s picture

We need to revive this issue.

Gábor Hojtsy’s picture

Issue tags: -sprint

I agree this should be revived but I was unsuccessful so far to find someone to help refocus the tests. Removing from sprint as a consequence.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: +sprint
FileSize
14.17 KB

Rerolling first.

Status: Needs review » Needs work

The last submitted patch, 58: 2476563-58.patch, failed testing.

The last submitted patch, 58: 2476563-58.patch, failed testing.

fran seva’s picture

I'm working on test

penyaskito’s picture

Sorry, I had work in progress which I didn't uploaded as I intended to finish this today.
This replace the view to use a node. There are still some issues with destination which you, Fran, can try to fix.

Status: Needs review » Needs work

The last submitted patch, 62: 2476563-entity-operations-62.patch, failed testing.

fran seva’s picture

ok @penyaskito. I'm going to try to fix it.

The last submitted patch, 62: 2476563-entity-operations-62.patch, failed testing.

isntall’s picture

The patch in comment #62 has been doing odd things on the qa.d.o testbots and has been cancelled.

penyaskito’s picture

Yeah, sorry, my verbosity was too high and the testbot wasnt happy with it.
I'm back to work on this during the extended sprints.

penyaskito’s picture

Now should be ready :D

Status: Needs review » Needs work

The last submitted patch, 68: 2476563-entity-operations-68.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/views/src/Tests/Handler/FieldEntityOperationsTest.php
@@ -29,30 +31,66 @@ class FieldEntityOperationsTest extends HandlerTestBase {
+    // Create Page content type.
+    if ($this->profile != 'standard') {
+      $this->drupalCreateContentType(array(
+        'type' => 'article',
+        'name' => 'Article'
+      ));

Article

The last submitted patch, 68: 2476563-entity-operations-68.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/views/src/Tests/Handler/FieldEntityOperationsTest.php
@@ -29,30 +31,66 @@ class FieldEntityOperationsTest extends HandlerTestBase {
+          $parts = explode('/', $operation['url']->toString());

Talking to @penyaskito, looks like we need to substring-remove the base path from the URL or something along those lines before checking.

penyaskito’s picture

Let's remove the base path then. Thanks @webflo for assisting with permissions stuff.

Gábor Hojtsy’s picture

Issue tags: -Needs tests

Yay, thanks for the updated tests! Lets get this in :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/config/src/Tests/ConfigEntityListMultilingualTest.php
    @@ -0,0 +1,68 @@
    +    // Ensure that operations for editing the Hungarian entity appear English.
    

    appear in?

  2. +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
    @@ -120,8 +135,38 @@ public function render(ResultRow $values) {
    -    // There is nothing to ensure or add for this handler, so we purposefully do
    -    //   nothing here and do not call parent::query() either.
    +    // There is nothing to ensure or add for this handler other than language,
    +    // so we purposefully do not call parent::query().
    +    if ($this->languageManager->isMultilingual()) {
    +      $this->getEntityTranslationRenderer()->query($this->query, $this->relationship);
    +    }
    

    Aren't we adding something here now?

  3. +++ b/core/modules/views/src/Tests/Handler/FieldEntityOperationsTest.php
    @@ -29,30 +31,67 @@ class FieldEntityOperationsTest extends HandlerTestBase {
    +    if ($this->profile != 'standard') {
    

    Why would the test run with two different profiles?

  4. +++ b/core/modules/views/src/Tests/Handler/FieldEntityOperationsTest.php
    @@ -29,30 +31,67 @@ class FieldEntityOperationsTest extends HandlerTestBase {
    +    ConfigurableLanguage::createFromLangcode('hu')->save();
    +    ConfigurableLanguage::createFromLangcode('es')->save();
    +    $this->rebuildContainer();
    

    Creating the language doesn't rebuild the container?

  5. +++ b/core/modules/views/src/Tests/Handler/FieldEntityOperationsTest.php
    @@ -29,30 +31,67 @@ class FieldEntityOperationsTest extends HandlerTestBase {
    +      foreach($entity->getTranslationLanguages() as $language) {
    

    Missing space.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
2 KB
17.56 KB

1. Fixed.
2. Right, we don't call parent::query but we are relying on TranslationLanguageRenderer. I'm not sure how to make this comment useful though.
3. Removed the check for profile.
4. Adding a language with the API doesn't rebuild the container, we need to do it.
5. Fixed.

Needs work still for 2.

jhodgdon’s picture

Ah, I hear someone calling my name "We need a better comment, Jennifer!" :)

So my suggestion would be to write something like:

Do not call parent::query(), because we do not want the default query behavior for Views fields. Instead, let the entity translation renderer provide the correct query behavior.

penyaskito’s picture

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK. This patch was RTBC in #75, marked needs work in #76, and I believe all the concerns of @catch were addressed, so back to RTBC on that basis.

Gábor Hojtsy’s picture

Yay, let's get this in :)

webchick’s picture

Assigned: Unassigned » catch

Back to catch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2476563-entity-operations-79.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Looks like a server problem on the test bot, sigh.

Gábor Hojtsy’s picture

Issue tags: +rc target

Should get in before RC, its pretty bad.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 1ae0ed8 on 8.0.x
    Issue #2476563 by Gábor Hojtsy, penyaskito, amateescu: Entity operations...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, amazing! Thanks everyone for making this happen :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.