[Change notice[ Replace views_ui_cache_set with a method on the ViewUI object

How about this? I have also changed the tests slightly in CachedDataUITest, to also test the cancel button on the edit form works correctly.

Looking at the ViewUIConverter, as it already uses the tempstore, could we pass this into the ViewUI so we won't need to make a drupal_container() call in cacheSet. thoughts? We could also, have a better name for this method. Also, thoughts? :)

This conflicts pretty heavily with #1919142: Convert Views UI AJAX forms to use FormInterface, and I keep getting confused since I see code I've removed in that issue :)
I'll take a real look at this over the weekend.

OK, let's postpone this one on that. This is a pretty small patch anyway, so it can just be re rolled when that issue is in. Probably easier for all involved :)

Let's get this moving again, rerolled.

Should probably use Drupal::service() too.

$this->locked isn't a TempStore object, it's the result of TempStore::getMetadata(), which is either a stdClass or NULL.

Yeah, I guess we would have seen a few notices if we couldn't access that. I'll change the doc for the locked property.

#5: 1919700-5.patch queued for re-testing.

Changed the $locked property doc on ViewExecutable.

Should we maybe look at the content of the locked object as well?

I think these tests are ok how they are, as NULL will be returned if no entry is found for the current user. I'm not sure we need to test the tempstore owner etc.. and the user tempstore tests will be covering that already?

We can remove that call to views_get_view in the test though I think.

OK, Let's help dawehner sleep at night and change the tests :) Check the owner is equal to the lock owner, and that it is equal to NULL when it should be.

Good point, here we go.

#17: 1919700-17.patch queued for re-testing.

This all looks like fine clean-up, but that cacheSet() method name is confusing. I was originally going to ask "Why can't Views just do a normal cache()->set() like everyone else?" only to find out that the method has nothing at all to do with setting caches and has everything to do instead with interacting with the user tempstore.

I realize this is a "pre-existing condition" since the function name you're moving away from is called views_ui_cache_set(), but it'd be nice to clean that up as well, since we are in "clean-up" phase after all. :)

Here it is with setTempStore() instead.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -835,7 +835,7 @@ public function displayExposedForm($form, &$form_state) {
-    views_ui_cache_set($form_state['view']);
+    $form_state['view']->setTempStore();

+1, this is much nicer.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -64,7 +64,10 @@ class ViewUI implements ViewStorageInterface {
+   * If this view is locked it will contain the result of
+   * \Drupal\user\TempStore::getMetadata(). Which can be a stdClass or NULL.

This doesn't need to be two sentences (nor should it, grammatically speaking).

Also, good tests!

Ok, we can make that a comma instead.

Just spotted a typo in one of the asserts. Tiny change.

#26: 1919700-26.patch queued for re-testing.

+++ b/core/modules/views/lib/Drupal/views/Tests/UI/CachedDataUITest.phpundefined
@@ -31,26 +31,34 @@ public static function getInfo() {
+    // Cancel the view edit and make sure the cache is deleted.
+    $this->drupalPost(NULL, array(), t('Cancel'));
+    $this->assertEqual($temp_store->getMetadata('test_view'), NULL, 'User tempstore data has been removed.');
+    // Test we are redirected to the view listing page.
+    $this->assertUrl('admin/structure/views', array(), 'Redirected back to the view listing page.');
+    // The view should not be locked.
+    $this->assertEqual($temp_store->getMetadata('test_view'), NULL, 'The view is not locked.');

Seem to have a duplicate assertion of the view not being locked.

Good point, that last assertion is pointless there.

Rerolled, after unsaved view message text got changed.

Views UI has moved...

Indeed it has...

#33: 1919700-33.patch queued for re-testing.

Me and Alex talked about this earlier and decided it would be nice to have an isLocked() method that encapsulates this logic, and change the property to lock instead of locked.

Nice improvement!

Committed 34acf68 and pushed to 8.x. Thanks!

For more information as to why this issue was moved to the Drupal core project, please see issue #3030347: Plan to clean process issue queue

Moving back to the contributed Views issue queue and closing as outdated per https://www.drupal.org/project/views/issues/3030347#comment-13023447