Make ViewsForm stop marking itself as needing to be cached

Honestly I would assume this to be an indication of insufficient test coverage more than anything

#1473276: Views Form is not cached, can lead to data loss and corruption added this.

#1473276-5: Views Form is not cached, can lead to data loss and corruption is amazing. Per @dawehner:

i'm just wondering whether this might cause problems for websites which uses views forms not as a backend tool but on the frontpage or something similar. They will see that each request creates a new entry in the form_cache table, which isn't that nice.

It looks like we'll need different approaches for various types of form to solve #2263569: Bypass form caching by default for forms using #ajax.. Bumping this to critical, but also tagging 'Needs issue summary update'.

@Wim:

I'm available to work on this if you need to focus on something else, but I'll need directions (updated IS at least) and early reviews :)

Updated IS would be good, but in a nutshell, see #4. I think the issue is that the row checkboxes for VBO are keyed by row number, so if the view in $form_state isn't persisted, then it is rerun on submission and the row number of a given entity can change (e.g., if an entity was added or removed in the meantime), and you'd operate on the wrong entity. The fix is probably to key the checkboxes on entity id or uuid. But, how to handle Views that display non-entities, such as watchdog entries?

That ship also sailed with #2252763: Views exposed filter form causes enormous form state cache entries as that rebuilds things anyway so we do not serialize all the data. That makes turning this off almost moot from that point of view.

Let me take a look at what we did there and report back here. I think that would help quite a bit.

#7: Quite easy, we now _have_ a permanent and unique row identifier:

Use the new cache keys from the views row caching instead of the Row ID => done :).

#9, I am not sure we can use that, as recalculating it would result in a new ID anyway? And we also can't rely on loading the cached row to get what we need.

Talked with damian about it and we thought about simply using entity ID + revisions ID + langcode, given that the views bulk operations are based upon entities only anyway.

This is a rough first patch, but I think we should do something like this:

- Generate our own return values on the bulk op checkboxes containing the langcode, ID, and VID if applicable. This will be a string with these values in
- Use this key instead to load the entity that we should load and action
- The rest can remain unchanged as we only need the change the mechanism to load entities we want to perform bulk actions on

The methods to create the IDs are not ideal, but it will be messy either way unless we can call out to the entity itself to get this. Entities already have a shit load of stuff on anyway so this would be a drop in the ocean I guess... :)

1. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,47 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +    return implode('-', $key_parts);
   

   Is there any risk in using "-" as separator?

2. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,47 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +  /**
   +   * @param string $bulk_form_key
   +   */
   +  protected function loadEntityFormBulkFormKey($bulk_form_key) {
   

   What about adding a @return :P

#11: Yes, we can, the row-cache-id is unique by design (else caching would not work).

However entity_type, entity_id, revision_id, langcode works for me.

Edit: #13 looks great to me!

At least for views we don't need the entity type but I agree for a general functionality, we should add it.

However entity_type, entity_id, revision_id, langcode works for me.

Not sure that will work.

If I submit a bulk delete operation, entity id 2 has vid 2.

Someone updates entity ID 2, so it moves to revision id 3.

Now the ID won't match.

But is there any need to have the revision ID? Can't update revisions in place as such.

edit: except for deleting revisions..

#15, Glad that approach seems feasible :)

I can add the entity_type to the new bulk key, we don't really need it though. Do you see another use for that?

row-cache-id would not work, as it uses the row index to create the ID. So that would essentially be the same as what we have now. The order of your view could change, and you would not be able to load the item anyway. Aside from that, we should not use caching based code here IMO.

I can add the entity_type to the new bulk key, we don't really need it though. Do you see another use for that?

I see a usecase for it in case we would have a generic functionality which would be part of the EM.

For a quick moment I thought \Drupal\Core\Entity\Entity::getConfigTarget could be useful, but it turns out, not at all.

Fixed the tests and added a couple of quick docblocks. So questions:

- Do we want to add the entity_type to the key? We always know the entity type form the bulk form
- Not sure if there is a cleaner way to implement the id and/or the loading of the entity from the ID parts?

Also added back Wim's patch from #1.

#22: The clearest way would be to have a EntityLoaderSerializer() or such that given data of an entity can ensure it can be loaded again.

That would be useful in many other parts of core. However I think its overkill for here and a simple delimiter based format works fine.

Is security a concern - that we need to - expose the IDs to change to the user?

e.g. Do we sufficiently check that I cannot just change the user-input key to some entity id I do not have access to?

#23, Access is already checked in the code from before:

// Skip execution if the user did not have access.
if (!$action->getPlugin()->access($entity, $this->view->getUser())) { ...

But maybe we should hash the checkbox return_values?

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -344,4 +361,53 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
+  protected function loadEntityFormBulkFormKey($bulk_form_key) {

Did you mean loadEntity*From*BulkFormKey()?

Certainly did! :)

1. +++ b/core/modules/node/tests/src/Unit/Plugin/views/field/NodeBulkFormTest.php
   @@ -51,6 +51,12 @@ public function testConstructor() {
   +    $entity_manager = $this->getMock('Drupal\Core\Entity\EntityManagerInterface');
   +    $entity_manager->expects($this->once())
   +      ->method('getStorage')
   +      ->with('action')
   +      ->will($this->returnValue($entity_storage));
   
   +++ b/core/modules/user/tests/src/Unit/Plugin/views/field/UserBulkFormTest.php
   @@ -51,6 +51,12 @@ public function testConstructor() {
   +    $entity_manager = $this->getMock('Drupal\Core\Entity\EntityManagerInterface');
   +    $entity_manager->expects($this->once())
   +      ->method('getStorage')
   +      ->with('action')
   +      ->will($this->returnValue($entity_storage));
   

   do you mind using quickly

   ->willReturn($entity_storage)</code??
   </li>
   
   <li>
   <code>
   +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,53 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +  protected function calculateEntityBulkFormKey(EntityInterface $entity) {
   ...
   +  protected function loadEntityFromBulkFormKey($bulk_form_key) {
   

   Fabianx asked for a more generic approach. I think we should not try to tackle that as part of this issue. I remember the following issue: #1983100: Provide a LoadableInterface for Typed Data objects and its endless discussions. Let's be pragmatic and fix this issue. If we want to have a generic implementation, that should be its own thing, especially because we already have some serialization format for config dependencies.

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -344,4 +361,53 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
+   * Calculates a bulk form key.
+   *
+   * @return string
+   */
...
+  /**
+   * Loads an entity based on a bulk form key.
+   *
+   * @param string $bulk_form_key
+   *
+   * @return \Drupal\Core\Entity\EntityInterface
+   */

nitpick NR2: Can we have a bit more documentation? Sorry. We should explain why we have such a method

In general though I really like the patch as it is!

1. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,53 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +  protected function calculateEntityBulkFormKey(EntityInterface $entity) {
   ...
   +  }
   

   Can we change the signature not be entity-dependent? E.g.:

   protected function calculateBulkFormKey(ResultRow $row) {
     $entity = $this->getEntity($row);
     //...
   }
   

   This way if in the future we want to implement VBO also for for non-entity stuff we are not forced to introduce BC layers and pollute the public (protected :) API.

2. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,53 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +  /**
   +   * Loads an entity based on a bulk form key.
   +   *
   +   * @param string $bulk_form_key
   +   *
   +   * @return \Drupal\Core\Entity\EntityInterface
   +   */
   +  protected function loadEntityFromBulkFormKey($bulk_form_key) {
   

   Same here, can we remove "entity" from the method name and return a row?

Other than that this is looking good to me.

This way if in the future we want to implement VBO also for for non-entity stuff we are not forced to introduce BC layers and pollute the public (protected :) API.

Yeah doesn't looks like a totally bad idea! Here is an issue summary.

Updated the issue summary.

Other than that this is looking good to me.

So, a complete rewrite of the patch then... :)

#30.1 - Every other expectation in both of those tests uses "->will($this->returnValue($entity_storage))", so this seems more consistent for now.
#30.2 - This seems to go against your comment in #33, or you mean this issue should not tackle an EntityLoaderSerializer approach?
#30.3 - Added some docs

I am also not sure about test coverage, we have bulk form test coverage, and probably trying to replicate stuff changing between a form being loaded and a form being submitted my be tricky? We could maybe unit test the bulk form class.

#32.1, #32.2: Hmm, I am not sure actually. BulkForm is completely coupled to entities anyway, so I don't see any gain from this? Also, other problems with that approach are: If you create a unique ID for a row, similar to the row cache ID, you need to find it again. So you need to execute the view on submission, and then calculate this value for each row again to find your entity. But then, once you do this, you are still not guaranteed that the entity you wanted to act on is in the re executed view. So that is a similar problem as we already have now. Thoughts?

Here is the doc update for now.

#32.1, #32.2: Hmm, I am not sure actually. BulkForm is completely coupled to entities anyway, so I don't see any gain from this? Also, other problems with that approach are: If you create a unique ID for a row, similar to the row cache ID, you need to find it again. So you need to execute the view on submission, and then calculate this value for each row again to find your entity. But then, once you do this, you are still not guaranteed that the entity you wanted to act on is in the re executed view. So that is a similar problem as we already have now. Thoughts?

We talked about that part. While writing a method with a generic signature for the $row =>ID part is pretty straightforward, its entirely not fair to do the loading side. You can't pass in arbitrary things, but its just bound onto entities for now. You know, in case we have a better idea in the future, we can just adapt the internal implementation ... this is why we have a protected method here.

#30.2 - This seems to go against your comment in #33, or you mean this issue should not tackle an EntityLoaderSerializer approach?

Yeah I think we totally should not. If someone cares, feel free to open up a dedicated issue for that.

The issue summary is now up to date, we don't need test coverage. Sheep it

Nice work seems very elegant solution. RTBC++
#1986606: Convert the comments administration screen to a view would need a re-roll after that. It would be great if we could have change notice for this. A lot of contrib module can inherit BulkForm plugin.

1. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -7,9 +7,14 @@
    use Drupal\Core\Entity\EntityStorageInterface;
   ...
   +use Drupal\Core\Language\LanguageInterface;
   

   EntityStorageInterface and LanguageInterface are now not used.

2. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,59 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +   * submitting a bulk form. This key allow the entity for the row to be loaded
   

   allows

3. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,59 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +   * @return string
   +   *
   

   Needs a description

4. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,59 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +   * @see loadEntityFromBulkFormKey
   

   This needs to be fully namespaced for api.d.o

5. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,59 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +  protected function calculateEntityBulkFormKey(EntityInterface $entity) {
   

   Missing @param doc

6. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -344,4 +361,59 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
   +   * @param string $bulk_form_key
   +   *
   +   * @return \Drupal\Core\Entity\EntityInterface
   

   @param and @return both need descriptions.

Working on those now. Also marking for D8MI.

I think this addresses all @alexpott's concerns from #37.

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -365,12 +363,17 @@ protected function drupalSetMessage($message = NULL, $type = 'status', $repeat =
+   * @param EntityInterface $entity

Let's use a FQCN

Sure. Also found another docs typo.

Thank you! Back to RTBC

This issue addresses a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed 6eb89b5 and pushed to 8.0.x. Thanks!

Amazing. Let's add test coverage for the multilingual improvement that this issue did in #2484037: Make Views bulk operations entity translation aware.

RTBC + 1 - extremely efficient patch for a difficult problem, congrats Damian!