Value of last_render_text leaks into the next Dropbutton

Come on, this is at least major

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1207,6 +1207,7 @@ public function isValueEmpty($value, $empty_zero, $no_skip_empty = TRUE) {
+    $this->last_render_text = NULL;
 

... I'm pretty convinced that this is the wrong fix. There is a usecase of using the token to itself in D7 for things like the mathfield. We maybe need to store the row index of the last rendered text, so we can compare that for the usecase of the dropbutton.

This sounds like a very similar bug to the one that would be caused in #2473873: Entity operations lack cacheability support, resulting in incorrect dropbuttons, but the fix sounds very different. Is it possible it's a duplicate?

NW based on #3.

So let's say I have two nodes, node 3 and node 4. I can edit node 3 but only view node 4. Is the bug here that node 4 shows an edit dropbutton that links to node 3, even immediately after a full cache clear? Clear steps to reproduce and a test case would definitely help evaluate this issue.

@xjm that's what I'm experiencing, yes. The Edit link in views works properly, the operations dropdown doesn't (although the delete button is correctly suppressed).

Added STR

failing test

And here's a fix that uses the 'We maybe need to store the row index of the last rendered text, so we can compare that for the usecase of the dropbutton' approach from #3

+++ b/core/modules/views/tests/src/Kernel/Handler/FieldDropbuttonTest.php
@@ -0,0 +1,146 @@
+          return $view->field[$field]->advancedRender($row) . PHP_EOL;
...
+    $this->assertEquals(trim((string) $dropbutton_output[1]), '');

the PHP_EOL can go in next pass, it was for easier debug, so this trim can go too

+++ b/core/modules/views/src/Plugin/views/field/Links.php
@@ -97,4 +105,20 @@ protected function getLinks() {
+      foreach (array_filter($this->options['fields']) as $field) {
+        $this->view->field[$field]->last_render_text = '';
+      }

This will mean it only works for the first row.. test and fix coming

This is better

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1181,7 +1188,10 @@ public function advancedRender(ResultRow $values) {
   +    if (!empty((string) $this->last_render)) {
   

   As said in IRC, "0" is a total valid rendered value in views.

2. +++ b/core/modules/views/src/Plugin/views/field/Links.php
   @@ -62,7 +63,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -    foreach ($this->options['fields'] as $field) {
   +    foreach (array_filter($this->options['fields']) as $field) {
   

   Is this one needed as part of the patch?

Fixes #20

Is there a good reason we can't keep this contained to Links/Dropbutton and not get FieldPluginBase involved at all? This passes the test locally, but maybe I'm missing some of the logic behind this.

Less changes in base class are always better. I guess we want to apply some manual testing though?

Less changes in base class are always better

My thoughts exactly.

Manual testing with #22 works too. See screenshots. Also added screenshots to the IS.

Before

After

Awesome! everyone seems happy here let's fix it then.

@Lendude but might not other plugins have the same problem and isn't the generic fix in #21 better?

@alexpott If other plugins have this problem, then hmm maybe, but this is just happening because the dropdown plugin reuses generated values from other field plugins. That is not a pattern that is wide spread (to say the least), so if we can leave it of the base class and make the plugin that wants to do this work a little harder, that sounds acceptable to me.

If this wasn't possible without changes to the base class, sure, put it in, but making all field handlers in all Views track data that currently has one known use case (and there the same effect can be achieved without this data), sounds like data we can do without for now.

So, in short, I don't really see a use case here that warrants a generic fix over a specific fix. Feel free to disagree obviously :)

Well won't \Drupal\contextual\Plugin\views\field\ContextualLinks have the same problem? Also as \Drupal\views\Plugin\views\field\FieldPluginBase sets this value should it clear it?

#2532200: Adding contextual links via Views fields does not work \Drupal\contextual\Plugin\views\field\ContextualLinks That thing is so broken, I have no idea if that would ever be bothered by this.

Also as \Drupal\views\Plugin\views\field\FieldPluginBase sets this value should it clear it?

Discussed this with @alexpott on IRC, this sounds like a better way to fix this, make FieldPluginBase clean up its own mess. Setting to NW for that.

+++ b/core/modules/views/src/Plugin/views/field/Links.php
@@ -97,4 +98,19 @@ protected function getLinks() {
+    // Clear out the previous rendered values if they are from a different
+    // row.
+    foreach (array_filter($this->options['fields']) as $field) {
+      if ($this->view->field[$field]->lastRenderIndex !== $values->index) {
+        $this->view->field[$field]->last_render_text = '';
+      }
+    }

So I think, moving this logic from #21 into FieldPluginBase instead of having it on Links.

So something like this then.

Interdiff is against #21 for clarity.

I came across this exact issue myself on my current project.

This patch looks clean, simple, and gets the job done.

I wonder about the difference between last_render_text and last_render...

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -102,6 +102,13 @@
+  public $lastRenderIndex;

Can be protected now - i think. Which is nice.

Fixed #32

I wonder about the difference between last_render_text and last_render...

One contains just the main output (last_render_text), and one contains the output after all the altering logic.

Feedback from #32 has been addressed, so back to RTBC.

Committed and pushed 14afb77 to 8.3.x and c0a0e44 to 8.2.x. Thanks!

diff --git a/core/modules/views/tests/src/Kernel/Handler/FieldDropbuttonTest.php b/core/modules/views/tests/src/Kernel/Handler/FieldDropbuttonTest.php
index 53c9564..b789bd6 100644
--- a/core/modules/views/tests/src/Kernel/Handler/FieldDropbuttonTest.php
+++ b/core/modules/views/tests/src/Kernel/Handler/FieldDropbuttonTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\views\Kernel\Handler;
 
 use Drupal\Core\Render\RenderContext;
-use Drupal\KernelTests\KernelTestBase;
 use Drupal\simpletest\ContentTypeCreationTrait;
 use Drupal\simpletest\NodeCreationTrait;
 use Drupal\simpletest\UserCreationTrait;

Unused use fixed on commit.