Make Views bulk operations entity translation aware

Raising to critical as per #2473989-21: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual.

Downgrading, per #2473989-51: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual. This is no longer in the critical path once #2485159: Switch admin/content from showing one translation per row to one node per row is fixed.

Let's test it for a change.

Update IS

Why just delete? The rest is also problematic. Is there another issue?

Promoting to critical based on discussion on the D8 criticals call with @alexpott and @catch in agreement. This is even worse than the #2486177: Deleting an entity translation from the UI deletes the whole entity issue because you can delete even more things unintentionally.

@dawehner: I can see how the delete is critical. It is not *that* big of an issue if you publish all languages of something (that you did not intend) vs. to remove all languages of something that you did not intend. Either way all of the operations need to be resolved. If we don't consider operations other than delete critical, we need yet one more issue for those :)

Should we expand this again to all bulk operations then?

Sorry, I thought the other operations were already ok, but actually they are only if we act on individual translations, which will need to do anyway to fix deletion.

VBO has always had an action-independent confirm step (and a per-action "skip confirmation" option).
Guessing that wasn't brought into core for simplicity, but to me it still makes perfect sense to have it.

Of course, that would mean figuring out the API for how an action can affect the confirm step, that's something that this issue needs, but VBO never had.

To see how this works (or not) now, I created a site with 3 languages and an article translated to all 3 and a page in a single language. After selecting all in the table, I wanted to unpublish them:

The result is all originals are unpublished but translations are not. Then I selected both published and wanted to publish them "again". Then of course the original translation of the article was published. (The page that I did not select remained unpublished).

Clearly operations are only operating on the original language of each entity, even when I only pick a translation to act on.

I think the first step to resolve this would be to handle on the VBO level that each row has their own language. Something like this attached. I looked at how Node.php does this for linking with the result rows, and $this->aliases is undocumented on field plugins, so I am not sure what would that one do, so this may or may not work.

(I also removed the unrelated changes from @dawehner's patch which I am sure crept in unintentionally. Only included the new thing in the interdiff).

#2486433: Make ViewsForm stop marking itself as needing to be cached makes the operations aware of language, so this one indeed only has the delete operations left then and/or adding confirm steps to all of them for extra safety. But at least following #2486433: Make ViewsForm stop marking itself as needing to be cached only the found translation would be affected by the operation. Technically we should postpone on that one I think but @dawehner said that @plach wanted to add test coverage for the language specific operations here. While that patch introduces loading the right entity variant, it does not test it specifically.

#2486433: Make ViewsForm stop marking itself as needing to be cached landed, so the non-delete operations should be fixed now I believe. Will test manually now. Needs tests for those as well as the delete operation to have confirmation properly because in that case, deleting the original language does not only delete that (unlike all other operations). I don't think confirmation for the rest of the operations is a requirement here then, because following #2486433: Make ViewsForm stop marking itself as needing to be cached the operations are doing the thing the user selected except the delete.

Then my change to load the entity in the language is duplicate, the bulk key already does that. So this is now only @dawehner's code rerolled.

Oh, manually testing #2486433: Make ViewsForm stop marking itself as needing to be cached did not fix the operations because the language is always the original one in the VBO result even for translation rows. So it works the same as before, even though now the id should be different per row. I set up the same scenario as above (3 article translations in the same article, 1 page). The article translations show up as "en-1-1" for all of them in the table instead of their own language :/ So needs fix for that too. And tests.

That is because this hunk is not getting the right language:

@@ -175,6 +186,8 @@ public function viewsForm(&$form, FormStateInterface $form_state) {
       // Render checkboxes for all rows.
       $form[$this->options['id']]['#tree'] = TRUE;
       foreach ($this->view->result as $row_index => $row) {
+        $entity = $this->getEntity($row);
+

Let's ensure in this issue that it does.

Working a bit on this

Not yet, sorry, I'll get back to this in a while, feel free to assign if you have tome to work on this.

For reals now

This should fix translation selection.

This implements individual translation deletion. Next steps:

• Generalize this code in a base class, so the user form does not need to repeat it.
• Fix the FIXME: there's a bad issue with lost translation object internal references, due to the temp store, I think.
• Verify this fixes also the other bulk operations.
• Provide additional test coverage.

Here are a few screenshots, I tried to mimic the single deletion form:

I'm done for today :)

+++ b/core/modules/node/src/Form/DeleteMultiple.php
@@ -102,11 +101,22 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          $items[] = $node->label();
...
-        return SafeMarkup::checkPlain($node->label());

Losing a checkPlain

Sorry, I uploaded the wrong patch, the interdiff was correct, though.

I got part way testing this, and looking into 3.
I think we can take out the checkplain. I tried to verify by taking out the checkplain and then making a node with the title <script>alert("xss!"); and looking at the message from deleting some of the nodes. and the title showed like that, and did not cause an alert. Is that enough proof it is ok to take out the checkplain?

[edit] I noticed something I will double check again later that is not a problem in head:
If make a node first, before enabling content translation, and then enable content translation, add a language and make another node.
The second node has translate as a contextual link in the all content listing, but the first node does not.

But clearing all the caches makes the translate contextual link show on the first node, and this is the same in head. Not introduced by this patch.

+++ b/core/modules/node/src/Form/DeleteMultiple.php
@@ -102,11 +104,43 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      elseif (!isset($items[$default_key])) {

Is this working dependent on the order of the results of $this->nodes?

because it assumes that the source langcode-node is processed first, so that translations are not separately added if the original was already selected (and thus all of its translations)?

On this again

AS pointed out by Gabor in today's critical issues call, we need an usability review here. Added the screenshots to the IS.

I'll try and review a bit later, but one thing right off the bat:

On the admin/content page, how would a user know that "test 2 EN" is a default translation versus "test 2 FR" is a regular translation? The rows look identical to me in the table, so the difference in behaviour between the two would be extremely surprising.

This should fix the user cancellation form. Actually it does not make any sense to display translations on the People view, since it's meant to administrate user accounts and not the content associated to them. Additionally the user name is not translatable, hence we'd show just straight duplicates. A dedicated custom view would be way better suited to deal with profile translations.

For these reasons I just added a filter on the default language and did not change the logic of the bulk cancellation form, which is meant to cancel accounts, again nothing to do with the field content attached to the user entities.

If we are ok with this solutions, these are the updated next steps:

• Generalize this code in a base class, so the user form does not need to repeat it. - still good to have, but not critical.
• Fix the FIXME: there's a bad issue with lost translation object internal references, due to the temp store, I think.
• Verify this fixes also the other bulk operations.
• Provide additional test coverage.

And obviously address any feedback from the UX team.

@webchick:

In #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual there's a patch to add the language column, we could add also the default translation column, if you think that would address your concerns.

@YesCT:

Is this working dependent on the order of the results of $this->nodes?

Yep, the unset call in the loop does the trick. You can verify it via manual testing by updating a translation so it comes before the original in the selection.

Nice work.

1. +++ b/core/modules/node/src/Form/DeleteMultiple.php
   @@ -118,12 +154,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $this->storage->loadMultiple(array_keys($delete_translations));
   

   Isn't that better than serializing entities in the form state? Getting away from that is a good thing IMO. Or you mean something else here?

2. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -294,7 +356,7 @@ public function viewsFormSubmit(&$form, FormStateInterface $form_state) {
   +        $entities[$bulk_form_key] = $entity;
   

   It's fine to use the entity ID again here as it's from the entities we have already loaded based on the bulk form key. The bulk form key is just for this plugin really.

3. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -63,19 +74,70 @@ class BulkForm extends FieldPluginBase {
   +  public function isCacheable() {
   ...
   +  public function getCacheContexts() {
   ...
   +  public function getEntityTypeId() {
   ...
   +  protected function getEntityManager() {
   ...
   +  protected function getLanguageManager() {
   ...
   +  protected function getView() {
   

   Can we keep the init() method above all of these please (Sorry :)).

I don't know all the implications of implementing CacheablePluginInterface here but it sounds dangerous for a form like this.

Per #48/#50, plach and I spoke on IRC and suggested adding a language column to the table (there was one on D7) with "default" denoted in that column for ones that are default translations. That'd look something like this:

+------------+-----+-------------------+-----+
| Title      | ... | Language          | ... |
+------------+-----+-------------------+-----+
| Hello      | ... | English (default) | ... |
| Bonjour    | ... | French            | ... |
+------------+-----+-------------------+-----+

Another option is to remove the "specialness" of default translations but this does not appear to be easy: #2485499: Allow source translations to be removed #2443991: Allow default_langcode field value to be changed Excerpt:

"Anyway, I'm pretty sure just changing the default_langcode field value is not enough, unless the entity is reloaded, because the internal data structures need to be adjusted to take the new default langcode into account."

Incidentally, I don't know that that needs to be done as part of this issue since it's a "pre-existing condition," but figured I'd mention it since it sounds like it might be easy to fix.

+++ b/core/modules/node/src/Form/DeleteMultiple.php
@@ -130,11 +132,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-          $items[$key] = SafeMarkup::checkPlain($node->label());
+          $items[$key] = $node->label();
...
-        $items[$key] = SafeMarkup::checkPlain($node->label());
+        $items[$key] = $node->label();

I can confirm with more certainty that this is totally fine.

For the record:

I checked (with help in IRC from @dawehner) that
core/lib/Drupal/Core/Template/TwigEnvironment.php:59
set to FALSE
(and a drush cr)

And a title (label) of <script>alert("xss!");</script>

with the change to not have checkPlain()

makes the alert happen.

And, changing
core/lib/Drupal/Core/Template/TwigEnvironment.php:59
back to TRUE
(and then a drush cr)

makes the alert not happen.
So the autoescape is enough for us.

I think we do not need to add language (or if something is the default/source translation) to the all content overview listing.
Note #1279624: Add translation filter to content listing admin page looked into that for a bit, and concluded that since it is a view, people can configure that if they want it. (at least that is what we concluded at the time)

also, for the translation tab for a piece of content, I remember us going around and around on what words to use for the source/default/original translation.

We ended up with: (Original language)

Note there is some complicated edge cases, where the language of the original translation can be changed, and then translation can have different sources...

I dont want this to get blocked on that discussion again as to if the "original" is really the original (it means the current *actual* language of the *the* node #1833180: decide on name of: Original content vs Original language vs Source language vs n/a in translations overview)

----

I think instead of changes to the content listing (which IMO should be a separate non-critical issue), we can help a bit by improving the wording on the delete confirmation... I'll try and get a patch right now.

here are some more (hopefully also clarifying words) which add why the multiple translations are removed in some cases (when the original node is deleted) and also the titles of each of the translations.

updated remaining tasks.

Not sure about #58, I tried to model the nested list on the single entity deletion form:

Now single and multiple deletion forms are inconsistent.

+++ b/core/modules/node/src/Form/DeleteMultiple.php
@@ -7,14 +7,16 @@
+use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Core\Entity\ContentEntityInterface;
+use Drupal\Core\Entity\ContentEntityTypeInterface;

I'm pretty sure we do not need the SafeMarkup use anymore. Not sure about the other changes to this use hunk.

---------

Did some manual testing on operations other than delete.

When selecting a translation without also selecting the original source, and picking remove from the front page, or promote to the front page, it works.

When selecting the original source, and not its translations, and picking remove from the front page, or promote to the front page, it works, effecting only the source original translation and not the other ones.

When selecting a translation and its original source, the remove from front page, or promote to front page, only effects the original source (and not the translation), even though it was also checked. Note, the message says the correct number of things was altered, but really not all of them were.

Reading the issue summary:
"If the default translation is selected for other operations but deletion, only operate on that translation."

So I think we need to alter the implementation of this.

=========

I think setting this to needs work for that, tests, and the fix me probably makes sense.

(I wont have time to work on this (for example writing tests) for at least a few hours, so that is up for grabs for someone to do.)

@plach I can see that point of view also. If we decide we want to not change it here, without also changing it on the single delete form, we can make a separate (normal) issue to change it on both after this critical goes in. It was an idea so that #53 did not have to be scoped into this issue. (which I still think is a separate issue either way.)

@damiankloip:

Isn't that better than serializing entities in the form state? Getting away from that is a good thing IMO. Or you mean something else here?

Good point. Serializing entities breaks internal translation references, so it's a bad idea for at least two reasons :)
The attached patch implements this suggestion and thus should fix the FIXME.

It's fine to use the entity ID again here as it's from the entities we have already loaded based on the bulk form key. The bulk form key is just for this plugin really.

It's not: if we don't use the bulk form key we cannot tell which translations were selected, because we get only one entity for each translation set. I removed the explicit reference in the user form code though.

I don't know all the implications of implementing CacheablePluginInterface here but it sounds dangerous for a form like this.

Good point again, I'll ask Wim or Fabian to chime in.

@webchick / @dawehner:

Incidentally, I don't know that that needs to be done as part of this issue since it's a "pre-existing condition," but figured I'd mention it since it sounds like it might be easy to fix.

Let's keep that for #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual: hiding the column on monolingual sites involves some non-trivial code and we also need test coverage for that.

@YesCT:

Did some manual testing on operations other than delete. [...]

Thanks, I'll tackle this next.

I can see that point of view also. If we decide we want to not change it here, without also changing it on the single delete form, we can make a separate (normal) issue to change it on both after this critical goes in. It was an idea so that #53 did not have to be scoped into this issue. (which I still think is a separate issue either way.)

Yep, we should either postpone this to a separate issue, or wait for the UX team to tell us how to reconcile the two forms.

Yeah I think we're all in "violent agreement" ;) that #53 can be handled in #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual. I just wanted to capture that the discussion happened, since the point came up here in review.

So then it sounds like what I should actually be UX reviewing is what comes after that screenshot, which are the confirm form and dsm() screenshots:

But essentially, same feedback: I do not understand how a user will understand why one of these triggers a "cascading delete" and others don't (and that's even assuming we fix the previous screen to include the language column in that other issue). I do like the idea of keeping symmetry with the other confirm form, but "The following content translations" is not enough of a clue stick IMO, because "Test IT" and "Test FR" are also content translations and do not trigger the "cascading delete."

So, can we do a similar thing, which is append "(Original translation)" or some such string to the node title in that case? Then it would be:

• Test 2 EN (Original translation) - The following content translations will be deleted:
  • English
  • French
• Page 3
• Page 1
• Test FR
• Test IT

That at least provides some bit of clue.

(Nitpick: Would be great if we could squeeze an "also" between "will" and "be" but can do that in a follow-up for all such forms.)

On this one, I feel that breaking these apart is much more confusing than not. First of all because I actually deleted four content translations, did I not? (English, French, Test FR, Test IT)

From a user POV, all I'm really deleting here are rows in a table. So can you just say "Deleted 7 items"? (Or "posts" or whatever the current language is.)

It's not: if we don't use the bulk form key we cannot tell which translations were selected, because we get only one entity for each translation set. I removed the explicit reference in the user form code though.

Ah, so the same entity is/could be referenced many times. I forgot this was already added to loadEntityFromBulkFormKey (to return the translation if needed). Nice!

@plach: I agree with the UX of the people form re #49. Looks like this was not commented on much by others in the meantime so wanted to jot down my support :) Makes sense given there are no translatable things in the user view unlike the content view where everything is translatable except the type by default at least.

@webchick:

Nitpick: Would be great if we could squeeze an "also" between "will" and "be" but can do that in a follow-up for all such forms.

This makes me suspect you didn't notice that all languages are listed, including the default translation language, not only the ones of the regular translations. Again this is the behavior currently implemented in the single deletion confirmation form. IMHO it's ok as it is right now, in fact if we listed only the translations, it would make more sense to list their titles, for consistency. But this would probably clutter the from, and may not be informative to a person having no familiarity with the related languages.

@plach: well, you know that, the user may not :) Maybe "This is the original content, so all translations will be deleted:" may be a shorter summary for the message? I think while it is still confusing that some rows work different (and that is due to currently not supporting removal of the default language variant to designate another default language variant), the message does not make it clear that all the translations are removed. Making that connection may make it easier to understand why that deletes more.

This should fix all the other operations, which were suffering from a loss of the translation internal references, similar to the one above.

I also fixed this, from #52:

I don't know all the implications of implementing CacheablePluginInterface here but it sounds dangerous for a form like this.

after discussing it with Fabian, Damian and Daniel (unfortunately Gandalf was missing ;)

@webchick's review still to be addressed.

Btw, this is why things were not working: the bulk form plugin always used EntityStorageInterface::loadRevision() to retrieve the selected entities, which, combined with #2503025: EntityStorageInterface::loadRevision() implementations are not statically cached, caused one entity translation to override the other.

Addressed #67 but kept separate log messages for entity and translation deletions, which IMO can be useful. I also skipped the nitpick, see #70.

Added test coverage for the various operations. Now working on a specific test for deletion. The interdiff and the test-only version are identical.

This completes test coverage and addresses #78. We should be done here, aside from addressing upcoming reviews.

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -178,15 +242,19 @@ public function getValue(ResultRow $row, $field = NULL) {
+    // Make sure we do not accidentally cache this form, see ::isCacheable().

Do you think we should mirror the @todo in isCacheable() here too? As this is just a failsafe, related to that, right?

Let's make sure @webchick has a chance to review this once more.

@dawehner:

1: Honestly I didn't. We definitely support deleting revisions although we currently have no UI to delete them in bulk. This would certainly make sense as a follow-up, together with a generalization making the logic available to any entity type.

@damiankloip:

Done :)

Looks like d.o removed the files with my crosspost with @plach's crosspost...

Went through the same steps as earlier, and confirmed the screenshots match expectations. I also fiddled around with e.g. setting "Sticky" on both an original and regular translation, and the settings took to only the intended rows, as expected. Awesome!! Finally, I tried creating my own content bulk operations view from scratch (in French, no less, which was a lot of "fun") and verified that the behaviour works fine there too.

I did find one small issue: I can't see anywhere in the patch that would cause this, but the Cancel button on the bulk operations delete confirm leads me back to "/content" (404), not "/admin/content". Not worth holding up a critical for this, though. Can we get a follow-up? (Maybe it's even a novice issue?)

Actually, I lied, I found another small issue: If I go to /fr/admin/structure/views, and hit "Edit" (well, "Modifier" ;)) on one of the views in that table, I'm taken to /admin/structure/views/view/content (it switches me back into English UI text translation), not /fr/admin/structure/views/view/content. (Actually, several places in the Views UI seemed to do this "popping back to English" behaviour, but I wasn't completely paying attention so needs more testing.) So it looks like we might still have some spots to clean-up? But at any rate, also not part of this issue.

It looks like code-wise this patch has been reviewed by multiple folks extremely knowledgeable about this area of the code. Therefore.....!

Committed and pushed to 8.0.x. YEAH! :D

Great work on this folks!!! :D

Opened #2504663: Entity operations links tied to original entity language, ignore both views row and UI languages, seems to be true of all operation links (not just in views). Also opened #2504671: /admin/content delete VBO cancel link goes to /content. Both reproduced.