Problem/Motivation

Repeatedly calling admin/content where there are 10 items of content listed (for example) causes key_value_expire to grow by 2 rows and 2MB for each request. Apache Bench testing for me caused the table to grow to 1.5GB, and a disk-full incident (on a VM with small virtual disk) & wrecked the db. Since 1000 requests equates to 2GB for this table, if a bot were to have access to a form which also causes storage to balloon with repeated requests, that may cause the db on a live site to become unstable.

Steps to reproduce: new git install of Drupal core. Create an authenticated user with permission to access the Content Overview page. Use devel_generate to make 10 pieces of content. Log in as that authenticated user, copy its cookie, and access the page repeatedly using Apache Bench (a tutorial on passing a session cookie to AB is here http://ezra-g.com/blog/20080229/benchmarking-authenticated-drupal-users-...). Check how much the key_value_expire has grown per page request.

Proposed resolution

Implement Serializable on the ViewExectuable object to decrease the footprint when it's serialized. Implement __sleep methods on ViewUi and View (storage) so the executable object is not serialized with them either (we can just create a new one in those instances pretty easily).

Remaining tasks

Tests

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because Drupal should be able to manage its memory/storage usage in a sane manner.
Issue priority Critical because runaway $form_state storage in the {key_value} table can create a a DOS situation with some database configurations.
Prioritized changes The main goal of this issue is scalability and addressed a critical bug.
Disruption None; no API change, no BC breaks.
CommentFileSizeAuthor
#115 2252763-114.patch1.44 KBdamiankloip
#107 interdiff-2252763-107.txt4.48 KBdamiankloip
#107 2252763-107.patch24.83 KBdamiankloip
#105 interdiff-2252763-105.txt720 bytesdamiankloip
#105 2252763-105.patch24.54 KBdamiankloip
#102 interdiff-2252763-102.txt3.42 KBdamiankloip
#102 2252763-102.patch24.33 KBdamiankloip
#100 interdiff-2252763-100.txt879 bytesdamiankloip
#100 2252763-100.patch23.38 KBdamiankloip
#98 interdiff-2252763-98.txt1.74 KBdamiankloip
#98 2252763-98.patch23.41 KBdamiankloip
#95 interdiff-92-95.txt967 bytesmartin107
#95 2252763-95.patch22.24 KBmartin107
#92 interdiff-2252763-92.txt1.61 KBdamiankloip
#92 2252763-92.patch22.24 KBdamiankloip
#91 interdiff-2252763-91.txt765 bytesdamiankloip
#91 2252763-91.patch20.63 KBdamiankloip
#87 interdiff-2252763-87.txt2.42 KBdamiankloip
#87 2252763-87.patch20.37 KBdamiankloip
#84 interdiff.txt709 bytesdashaforbes
#84 2252763-76.patch19.74 KBdashaforbes
#76 interdiff-2252763-76.txt1.55 KBdamiankloip
#76 2252763-76.patch19.74 KBdamiankloip
#74 interdiff-2252763-74.txt1.04 KBdamiankloip
#74 2252763-74.patch19.83 KBdamiankloip
#70 interdiff-2252763-70.patch2.67 KBdamiankloip
#70 2252763-70.patch18.79 KBdamiankloip
#65 2252763-65.patch17.66 KBdamiankloip
#63 interdiff-2252763-62.txt601 bytesdamiankloip
#63 2252763-62.patch17.65 KBdamiankloip
#61 interdiff-2252763-61.txt10.24 KBdamiankloip
#61 2252763-61.patch17.06 KBdamiankloip
#59 interdiff.txt2.48 KBdawehner
#59 2252763-59.patch8.8 KBdawehner
#56 interdiff-2252763-56.txt1.24 KBdamiankloip
#56 2252763-56.patch8.14 KBdamiankloip
#55 interdiff-2252763-55.txt5.26 KBdamiankloip
#55 2252763-55.patch8.39 KBdamiankloip
#53 2252763-53.patch7.96 KBmartin107
#50 interdiff-2252763-50.txt1.23 KBdamiankloip
#50 2252763-50.patch7.87 KBdamiankloip
#49 interdiff-2252763-49.txt7.01 KBdamiankloip
#49 2252763-49.patch7.81 KBdamiankloip
#46 interdiff-2252763-46.txt1.52 KBdamiankloip
#46 2252763-46.patch3.94 KBdamiankloip
#43 interdiff-2252763-43.txt1.9 KBdamiankloip
#43 2252763-43.patch3.73 KBdamiankloip
#40 interdiff-2252763-40.txt2.47 KBdamiankloip
#40 2252763-40.patch2.69 KBdamiankloip
#39 2252763-39.patch1.09 KBdamiankloip
#33 interdiff.txt401 bytesskipyT
#33 2252763-33.patch12.59 KBskipyT
#31 interdiff.txt7.4 KBskipyT
#31 2252763-31.patch12.41 KBskipyT
#28 2252763-28.patch10.09 KBskipyT
#23 interdiff.txt1.29 KBskipyT
#23 2252763-23.patch804 bytesskipyT
#18 2252763-18.patch809 bytesdamiankloip
#17 2252763-kve-after.txt3.65 KBdamiankloip
#17 2252763-kve-before.txt195.05 KBdamiankloip
#17 2252763.patch9.94 KBdamiankloip
#6 key_value_expire.txt4.2 MBJohn_B
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

John_B’s picture

Component: user.module » base system
John_B’s picture

Issue summary: View changes
John_B’s picture

Issue summary: View changes
catch’s picture

Which form is causing the data to balloon that much? Is it per-session or every request?

Could you dump the data being cached and attach it as a .txt?

Berdir’s picture

We've noticed that before on the node add form, but I'm not sure why it would happen on admin/content, possibly the views form?

We have other, related issues to save less in the key value store.

John_B’s picture

FileSize
4.2 MB

On repeating the experiment, the page requests to admin/content are consistently producing 2 rows and .35MB per request. So a lot smaller but arguably still and issue. A dump of the table after 10 requests is attached.

tim.plunkett’s picture

Wim Leers’s picture

#7: that's similar, but this is a bigger problem, because it occurs immediately, not due to keeping things around for too long.

I was able to reproduce this. Create 5 nodes, visit admin/content, look at key_value_expire's newly added form_state row … and that's 225 KiB. With 10 nodes, it's 263 KiB. It looks like things are being serialized that shouldn't be serialized.

damiankloip’s picture

This is kind of the same issue? This will only be a problem if things also stay around for too long? There is one issue IMO - table size getting out of control easily.

damiankloip’s picture

This will happen on any form with an #ajax element. ajax_process_form() will set $form_state['cache'] = TRUE. So each request with a new form ID etc... will get cached just from the GET request to retrieve the form.

John_B’s picture

The point is probably obvious to core specialists, but FWIW I checked that repeated identical hits run with this type of test on a D7 webform or ajax webform do not lead to any D7 cache table growing.

damiankloip’s picture

Yeah, I think this is a D8 specific issue :/

Wim Leers’s picture

Title: key_value_expire balloons on repeated requests » Views exposed filter form causes enormous form state cache entries
Component: base system » views.module
Issue tags: +VDC

#9: No it's not the same problem. The problem is that each individual cache entry is *enormous*. A single form_state cache entry containing >250 KiB of data… that's surely not intentional? AFAICT, the problem is that far too many objects are being serialized in Views exposed filter forms. If we could reduce the number of things being serialized, then the problem should go away! :)

catch’s picture

I think the fact that ajax-enabled forms generate a new cache entry for every page hit is as much of a problem as the cache size. Even if we make the cache size smaller, a sufficient number of hits still runs the risk of filling up the database. As it stands I could set up a browser auto-refresh plugin and take some sites down just with that.

Feels like we ought to be able to reduce this to one entry per-session per-form, or possibly better than that. MIght want to split this into two issues?

damiankloip’s picture

If we could reduce the number of things being serialized, then the problem should go away! :)

I agree with catch, and what I was saying in #10 - This affects any ajax enabled form. So that is definitely not a problem just pertaining to views. Making the cache entries smaller will definitely not make the problem go away, just dilute it slightly.

Created another issue as this seems to be just for the size: #2263569: Bypass form caching by default for forms using #ajax.

catch’s picture

Version: 8.0-alpha11 » 8.x-dev
damiankloip’s picture

Status: Active » Needs review
FileSize
9.94 KB
195.05 KB
3.65 KB

So, one option is to pass around the view id and display id instead. The downside is we then need to load the view set the display, and init handlers each time. The other option is to look at improving the DependencySerialization implementation as ViewExecutable just extends DependencySerialization currently, which does not serialize much as it is only looking for direct object properties.

Example form state KVE entries attached also for before and after. You can see the difference just from the file sizes. That's about a 98% decrease! I have used the admin/content view with one node.

damiankloip’s picture

FileSize
809 bytes

With the DependencySerialization approach. Bear in mind these are both rough patches atm!

The last submitted patch, 17: 2252763.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: 2252763-18.patch, failed testing.

Status: Needs work » Needs review

dawehner queued 18: 2252763-18.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: 2252763-18.patch, failed testing.

skipyT’s picture

FileSize
804 bytes
1.29 KB

rerolled the patch and added the current user also in the wakeup.

skipyT’s picture

Status: Needs work » Needs review
damiankloip’s picture

We need the current user to be set though?

damiankloip’s picture

Sorry, you have added it. That interdiff is very misleading... :)

So I think in an ideal work, we would go with my first approach. I.e. don't serialize views. wdyt?

Status: Needs review » Needs work

The last submitted patch, 23: 2252763-23.patch, failed testing.

skipyT’s picture

Status: Needs work » Needs review
FileSize
10.09 KB

Rerolled the first patch. Which is taking out the views from the from_state.

Status: Needs review » Needs work

The last submitted patch, 28: 2252763-28.patch, failed testing.

skipyT’s picture

Assigned: Unassigned » skipyT

I rerolled yesterday the patch, but it isn't worked. It seems we have many tests failing because the exposed form isn't working anymore. Checking it.

skipyT’s picture

Status: Needs work » Needs review
FileSize
12.41 KB
7.4 KB

I modified the code to work with the exposed forms. We still need the sleep function to get out the views from form state. It seems the callback object is storing the views and we need to get rid of that. but we need the callback objects for ajax forms.

Status: Needs review » Needs work

The last submitted patch, 31: 2252763-31.patch, failed testing.

skipyT’s picture

Status: Needs work » Needs review
FileSize
12.59 KB
401 bytes

i tried to resolve the fatal errors. for now I have a huge hack for this. but lets see what we get at the tests.

Status: Needs review » Needs work

The last submitted patch, 33: 2252763-33.patch, failed testing.

xjm’s picture

Issue tags: +Triaged D8 critical
larowlan’s picture

We need an issue summary update here - specifically the 'proposed resolution'

dawehner’s picture

Well, right we need a solution for this issue.

skipyT’s picture

Assigned: skipyT » Unassigned
damiankloip’s picture

FileSize
1.09 KB

I like the serialzation of the view approach too, this works in the sense that the view takes up hardy any space now, but more data is put in there that is not there when the entire view is cached.

So this will not really work at the moment, but this is an idea.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
2.47 KB

With more things added to help keep the state.

dawehner’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -2201,4 +2201,45 @@ public function calculateDependencies() {
+    // Rebuild the exposed raw input.
+    $exclude = array('submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', 'reset');
+    foreach ($this->exposed_data as $key => $value) {
+      if (!in_array($key, $exclude)) {
+        $this->exposed_raw_input[$key] = $value;
+      }
+    }
+

Could we extract that into a helper method?

Status: Needs review » Needs work

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
1.9 KB

ok, let's see how much better this does.

I haven't looks at extracting that into a helper method yet, one problem that could arise from this is that in ViewsExposedForm::submitForm the $exclude array is passed to the exposed form plugin first (exposedFormSubmit) method where the $exclude array can be altered... So not sure if we need to use the same logic here too, probably. Otherwise there could be differences. Or we just keep the raw input for now as-is and be done with it? :) Or we add a getExposedRawInput() method that does this for us?

Status: Needs review » Needs work

The last submitted patch, 43: 2252763-43.patch, failed testing.

damiankloip’s picture

ok, so no views forms work, no biggie ;)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
1.52 KB

OK, so it was an issue with the form handlers expecting the view to be executed already. If it was previously executed, we should re execute it when we unserialize I think?

Status: Needs review » Needs work

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

dawehner’s picture

  1. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -2201,4 +2201,53 @@ public function calculateDependencies() {
    +   * {@inheritdoc}
    +   */
    +  public function unserialize($serialized) {
    +    list($storage_id, $current_display, $args, $current_page, $exposed_input, $exposed_data, $dom_id, $executed) = unserialize($serialized);
    +
    

    We certainly should explain why we implement it.

  2. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -2201,4 +2201,53 @@ public function calculateDependencies() {
    +    $this->setCurrentPage($current_page);
    

    What about setItemsPerPage, setOffset, set AjaxEnabled etc.?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
7.81 KB
7.01 KB

Some more changes.

damiankloip’s picture

And with the storage back to ID.

The last submitted patch, 49: 2252763-49.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 50: 2252763-50.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
7.96 KB

I can see a few flaws which are easy to fix - but before I start to make changes, the patch needs a reroll.

No conflicts just auto merging.

Status: Needs review » Needs work

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
8.39 KB
5.26 KB

Bring back the other tests, and trying out something else with ViewUI. Will still not work.

damiankloip’s picture

This.

The last submitted patch, 55: 2252763-55.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 56: 2252763-56.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.8 KB
2.48 KB

Just tried to understand what damian was talking about. So what about updating
the storage on the ViewUI object, if needed? Not sure, whether this quick fix is sane here, at least things don't fatal anymore.

Status: Needs review » Needs work

The last submitted patch, 59: 2252763-59.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
17.06 KB
10.24 KB

mh, not sure. That will still load the storage object again which I think is a problem. If we are using the view UI object I think we only want to store and use that. Something like this maybe:

Status: Needs review » Needs work

The last submitted patch, 61: 2252763-61.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
17.65 KB
601 bytes

ok let's try this then. Should bring the exceptions down.

Status: Needs review » Needs work

The last submitted patch, 63: 2252763-62.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
17.66 KB

Reroll after some changes in the ViewUI class. Should be same fails.

Status: Needs review » Needs work

The last submitted patch, 65: 2252763-65.patch, failed testing.

martin107’s picture

Looking at the failing test FieldUITest, locally I get a smaller set of fails.. Retesting just to see where we stand.

Status: Needs work » Needs review

martin107 queued 65: 2252763-65.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2252763-65.patch, failed testing.

damiankloip’s picture

FileSize
18.79 KB
2.67 KB
catch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: interdiff-2252763-70.patch, failed testing.

The last submitted patch, 70: 2252763-70.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
19.83 KB
1.04 KB

It doesn't make sense why just those two assertions would fail.. Looking at the test, those assertions don't seem to make sense anyway, so not sure what was broken before? If a node it set published FALSE, and then made sticky, it should not be published then?

dawehner’s picture

Great work!

They indeed doesn't make sense anymore at all. Its odd that they did not fail but after #2172017: Bulk operations does not respect entity access its just the logic step to remove this stuff.

  1. +++ b/core/modules/views/src/Entity/View.php
    @@ -433,4 +433,13 @@ public function isInstallable() {
    +    unset($keys[array_search('executable', $keys)]);
    

    Its just ridiculous that PHP doesn't have a method for that.

  2. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
    @@ -157,13 +159,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    $exclude = array('submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', '', 'reset');
    +    $exclude = array('submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', 'reset');
         /** @var \Drupal\views\Plugin\views\exposed_form\ExposedFormPluginBase $exposed_form_plugin */
    

    Haha, we excluded the empty string, this is brutal.

  3. +++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
    @@ -150,7 +150,6 @@ public function renderExposedForm($block = FALSE) {
    -    $form_state->set('exposed_form_plugin', $this);
    

    Can you quickly explain why this helped so much?

  4. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -26,7 +27,7 @@
    -class ViewExecutable {
    +class ViewExecutable implements \Serializable {
    

    There was afaik a reason why we implemented __sleep and not the \Serializable interface

  5. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -2284,4 +2285,55 @@ public function calculateDependencies() {
    +    if ($this->storage instanceof ViewUI) {
    +      $storage = $this->storage;
    +    }
    +    else {
    +      $storage = $this->storage->id();
    +    }
    

    It would be great if we document quickly why we have to do that.

  6. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -173,10 +173,10 @@ class ViewUI implements ViewEntityInterface {
    -    if (!isset($executable)) {
    -      $executable = Views::executableFactory()->get($this);
    +
    +    if (isset($executable)) {
    +      $this->executable = $executable;
         }
    -    $this->executable = $executable;
       }
     
       /**
    @@ -259,7 +259,7 @@ public function standardSubmit($form, FormStateInterface $form_state) {
    
    @@ -259,7 +259,7 @@ public function standardSubmit($form, FormStateInterface $form_state) {
         $display_id = $form_state->get('display_id');
         if ($revert) {
           // If it's revert just change the override and return.
    -      $display = &$this->executable->displayHandlers->get($display_id);
    +      $display = &$this->getExecutable()->displayHandlers->get($display_id);
           $display->optionsOverride($form, $form_state);
    

    +1

damiankloip’s picture

FileSize
19.74 KB
1.55 KB

Thanks Daniel.

1. Yes, utterly ridiculous, I want one of those. Maybe Drupal should have a helper?
2. Yes, I know. wat?!?
3. We want to clean up the form state, this was just here out of convenience. Also, if you are creating a new exectuable things get a little bit crazy/broken with old references
4. I went with Serialzable on the ViewExectuable only so I could return the values I wanted to (pretty much for storage ID)
5. Remove that, now it is just always the ID only.
6. :) much better

damiankloip’s picture

Issue summary: View changes

Will do some tests shortly. Should be quite easy.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Lovely!

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

Ups, tests are missing. Well, you could argue that we have quite some implicit test coverage.

jhedstrom’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -17,6 +17,7 @@
+use Drupal\views_ui\ViewUI;

Is this needed? I don't see it used anywhere, and it would also create a dependency on the views_ui module?

damiankloip’s picture

Assigned: Unassigned » damiankloip

...create a dependency on the views_ui module? - HA :)

But yes, we don't need it anymore, thanks! I will roll that into the next patch.

Fabianx’s picture

Status: Needs review » Needs work

CNW per #80

larowlan’s picture

Issue tags: +DrupalSouth

Tagging as a task for DrupalSouth sprint

dashaforbes’s picture

Status: Needs work » Needs review
FileSize
19.74 KB
709 bytes

Removed use statements from ViewExecutable.php

damiankloip’s picture

This is needs work for test, that I am working on. Removing all of those use statements is out of scope here. Sorry!

damiankloip’s picture

.

damiankloip’s picture

FileSize
20.37 KB
2.42 KB

Here are some quick tests, as dawehner already mentioned, we have implicit coverage everywhere for this. Look at all the fails that have been fixed over the (many) previous patch iterations :)

With a quick test:

$form_state before: 175789 bytes
$form_state after: 18154 bytes

Not bad!

dawehner’s picture

+++ b/core/modules/views/src/Tests/ViewExecutableTest.php
@@ -436,4 +436,24 @@ public function testValidate() {
+  public function testSerialization() {

So we test serialize -> unserialize but do you think testing the serialized content for itself is something we should try out? So for example $this->assertFalse(strpos($serialized, '"executable"') !== FALSE)?

damiankloip’s picture

Mehhh. I am not sure that is worth it. We know the object will get serialized. or do you mean check there is not a executable property when serialized in another object? Either way, I would be inclined to say maybe not worth it?

I could add a similar test for the viewUI object maybe?

dawehner’s picture

Mehhh. I am not sure that is worth it. We know the object will get serialized. or do you mean check there is not a executable property when serialized in another object? Either way, I would be inclined to say maybe not worth it?

Well, for now nothing ensures that the bug of this issue is actually fixed right? There is no test covering the fact that we don't waste all that memory.

damiankloip’s picture

Issue tags: -Needs tests
FileSize
20.63 KB
765 bytes

OK, so we talked about this, we are serializing the actual executable, so let's check the ViewStorage is not in the serialized output.

damiankloip’s picture

FileSize
22.24 KB
1.61 KB

And a quick test for the ViewUI object.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It looks great now! Factor 10 is great!

swentel’s picture

Extreme nitpicks.

  1. +++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
    @@ -113,4 +114,32 @@ public function testIsLocked() {
    +   * Tests serialization if the ViewUI object.
    

    if => of

  2. +++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
    @@ -113,4 +114,32 @@ public function testIsLocked() {
    +    // Make sure the ViewExecutable clas is not found in the serialized string.
    

    clas => class

martin107’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
22.24 KB
967 bytes

Fixed.

mpdonadio’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Related issues: +#2341447: Possible Denial of Service Attack with {cache_form}

Back to RTBC.

Added Beta Eval and a related issue. Because of the DOS possibility, this could potentially also get the security tag.

Fabianx’s picture

RTBC + 1, Great work!

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
23.41 KB
1.74 KB

Spoke to Alex on IRC. Let's just remove the setPublished() calls we do not need anymore, and amend the comment.

re-RTBC please :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

The test change makes sense.

damiankloip’s picture

FileSize
23.38 KB
879 bytes

Oops, we can remove the save() calls too. Leaving as is.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views_ui/src/ViewUI.php
@@ -173,10 +173,10 @@ class ViewUI implements ViewEntityInterface {
   public function __construct(ViewEntityInterface $storage, ViewExecutable $executable = NULL) {
     $this->entityType = 'view';
     $this->storage = $storage;
-    if (!isset($executable)) {
-      $executable = Views::executableFactory()->get($this);
+
+    if (isset($executable)) {
+      $this->executable = $executable;
     }
-    $this->executable = $executable;
   }

We can completely remove $executable from the constructor. Talked about this with @damiankloip in IRC.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
24.33 KB
3.42 KB

Ok, let's try this. Only using the storage executable. If you need to set it, set it on the storage object first.

alexpott’s picture

#100 10 files changed, 174 insertions, 54 deletions.
vs
#102 10 files changed, 163 insertions, 64 deletions

Nice. Less code to maintain :) and no chance of ViewUI having an executable out of sync with its storage.

Status: Needs review » Needs work

The last submitted patch, 102: 2252763-102.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
24.54 KB
720 bytes

Status: Needs review » Needs work

The last submitted patch, 105: 2252763-105.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
24.83 KB
4.48 KB

Drupal alter wanting to pass the object by reference and creating warnings. Let's use an $executable variable. We also might as well replace the many many calls in the same method with the variable!

Fabianx’s picture

Nice :)

Is there anything more needed or can this go back to a final review for RTBC?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The changes made by @damiankloip are looking great!

tim.plunkett’s picture

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -873,14 +863,14 @@ public function cacheSet() {
-      unset($executable->current_display);
+      $executable->current_display = NULL;

Classic.

RTBC +1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed d1c0322 on 8.0.x
    Issue #2252763 by damiankloip, skipyT, martin107, dawehner, dashaforbes...
Fabianx’s picture

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -580,11 +570,11 @@ public function renderPreview($display_id, $args = array()) {
-    $this->executable->destroy();
+    $errors = $this->getExecutable()->validate();
+    $executable->destroy();

stray $this->getExecutable() for follow-up ...

Wim Leers’s picture

Great work!

damiankloip’s picture

FileSize
1.44 KB

Thanks all! Here is that small follow up,there were a couple actually...whoops.

webchick’s picture

Let's get a follow-up issue for those, so they can do a proper testbot run, etc.

martin107’s picture

opened up #2449079: ViewsUI executable cleanup

test bot is on the case.

Status: Fixed » Closed (fixed)

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