Provide a classed object for TempStore metadata

+++ b/core/lib/Drupal/Core/TempStore/PrivateTempStore.php
@@ -155,7 +155,7 @@ public function set($key, $value) {
-   * @return mixed
+   * @return \Drupal\Core\TempStore\Lock
    *   An object with the owner and updated time if the key has a value, or
    *   NULL otherwise.

+++ b/core/lib/Drupal/Core/TempStore/SharedTempStore.php
@@ -215,7 +215,7 @@ public function set($key, $value) {
-   * @return mixed
+   * @return \Drupal\Core\TempStore\Lock
    *   An object with the owner and updated time if the key has a value, or
    *   NULL otherwise.

Can it still return NULL?

Ok, I made a pass through this but need to look again with fresh eyes (hopefully tomorrow). For testing, just test locking for views, yes?

1. +++ b/core/lib/Drupal/Core/TempStore/Lock.php
   @@ -0,0 +1,112 @@
   +   * @var int
   +   */
   +  private $owner;
   

   Since there is getOwnerId and getOwner, seems like this variable (and elsewhere) should change to ownerId.

2. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -48,12 +49,11 @@ class ViewUI implements ViewEntityInterface {
       */
   -  public $lock;
   +  protected $editingLock;
   

   Unclear why this is called $editingLock.

3. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1351,4 +1351,53 @@ public function addCacheTags(array $cache_tags) {
   +    if ($name === 'lock') {
   +      @trigger_error('Using the "lock" public property of a View is deprecated in Drupal 8.7.0 and will not be allowed in Drupal 9.0.0. Use \Drupal\views_ui\ViewUI::setLock() instead. See https://www.drupal.org/node/3025869.', E_USER_DEPRECATED);
   +      if ($value instanceof \stdClass && property_exists($value, 'owner') && property_exists($value, 'updated')) {
   +        $value = new Lock($value->owner, $value->updated);
   

   Ah, it's to not conflict with old $lock? And owner (instead of ownerId) is to keep consistent with old naming.

#1748022: Make cache()->get() return a classed CacheItem object is very similar and has a ton of discussion around BC, might be interesting.

Thanks for the changes in #12! Much better. :) Does anything need to be done for @Berdir's comment?

One thing that the other issue did was to extend from \stdClass to pass explicit checks against that. But no idea how to do that in a BC way, e.g. detect checks against that, and if that's in any way necessary here.

Cache items are probably used 50x (random guess) more often than this thing, so it's far less likely that something breaks due to that. I think for cache items we at least had explicit test coverage for that.

I tested the patch from #12 by:

1) Created a view
2) Created view as user 1 and saved
3) Edited the view as user 2 and didn't save
4) Loaded the view as user 1 and it showed it was locked
5) Broke the lock and changes were gone
6) Repeated the above to get the lock as user 2 and broke the lock

Appears to be working as expected.

The code looks ok, the view locking passes manual testing, and tests are passing. Marking this RTBC.

+1 to the idea of making a Lock value object.

+++ b/core/lib/Drupal/Core/TempStore/Lock.php
@@ -0,0 +1,112 @@
+  /**
+   * Wraps the entity type manager.
+   *
+   * @return \Drupal\Core\Entity\EntityTypeManagerInterface
+   *   The entity type manager.
+   */
+  private function entityTypeManager() {
+    return \Drupal::entityTypeManager();
+  }

+++ b/core/modules/views_ui/src/Form/BreakLockForm.php
@@ -71,7 +71,7 @@ public function getQuestion() {
-    $account = $this->entityManager->getStorage('user')->load($locked->owner);
+    $account = $locked->getOwner();

It feels odd to move from service injection to service location. Are we sure we want to do this?

1. +++ b/core/lib/Drupal/Core/TempStore/Element/BreakLockLink.php
   @@ -0,0 +1,136 @@
   + * Usage example:
   + * @code
   + * $build['examples_lock'] = [
   + *   '#type' => 'lock',
   + *   '#label' => $this->t('example item'),
   + *   '#lock' => $tempstore->getMetadata('example_key'),
   + *   '#url' => \Drupal\Core\Url::fromRoute('examples.break_lock_form'),
   + * ];
   + * @endcode
   + *
   + * @RenderElement("break_lock_link")
   

   This usage example is not accurate; #type should be break_lock_link.

2. +++ b/core/modules/user/tests/src/Unit/PrivateTempStoreTest.php
   @@ -189,7 +189,7 @@ public function testGetMetadata() {
        $metadata = $this->tempStore->getMetadata('test');
   -    $this->assertObjectHasAttribute('owner', $metadata);
   +    $this->assertObjectHasAttribute('updated', $metadata);
   

   Why did this change? Seems like it could also assert that there is an ownerId attribute, in addition to updated.

3. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -888,7 +891,7 @@ public function cacheSet() {
   +    return is_object($this->lock) && ($this->lock->getOwnerId() != \Drupal::currentUser()->id());
   

   This seems like a good place to call $this->getLock(). Generally I see no reason for objects not to use their own APIs.

4. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1351,4 +1354,65 @@ public function addCacheTags(array $cache_tags) {
   +  public function getLock() {
   +    return $this->lock;
   +  }
   +
   +  /**
   +   * Sets a lock on this View.
   +   *
   +   * @param \Drupal\Core\TempStore\Lock $lock
   +   *   The lock object.
   +   *
   +   * @return $this
   +   */
   +  public function setLock(Lock $lock) {
   

   This stuff seems like it should be in a trait.

5. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1351,4 +1354,65 @@ public function addCacheTags(array $cache_tags) {
   +  /**
   +   * Unsets the lock on this View.
   +   *
   +   * @return $this
   +   */
   +  public function unsetLock() {
   

   Nit: Maybe removeLock() or clearLock() would be a clearer name for this method?

6. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1351,4 +1354,65 @@ public function addCacheTags(array $cache_tags) {
   +      if ($lock = $this->getLock()) {
   +        return (object) ['owner' => $lock->getOwnerId(), 'updated' => $lock->getUpdated()];
   +      }
   

   Seeing as how the 'owner' and 'updated' properties have BC, is there any reason not to simply return $lock from this?

Well, that's all I got, folks. Looks legit and nice to me.

+++ b/core/lib/Drupal/Core/TempStore/Element/BreakLockLink.php
@@ -0,0 +1,136 @@
+ * Usage example:
+ * @code
+ * $build['examples_lock'] = [
+ *   '#type' => 'break_lock_link',
+ *   '#label' => $this->t('example item'),
+ *   '#lock' => $tempstore->getMetadata('example_key'),
+ *   '#url' => \Drupal\Core\Url::fromRoute('examples.break_lock_form'),
+ * ];
+ * @endcode

CR should mention about new render element, it is useful for contrib/custom code

1. +++ b/core/lib/Drupal/Core/TempStore/Element/BreakLockLink.php
   @@ -0,0 +1,136 @@
   + * Provides a link to break a tempstore lock.
   + *
   + * Properties:
   + * - #label: The label of the object that is locked.
   + * - #lock: \Drupal\Core\TempStore\Lock object.
   + * - #url: \Drupal\Core\Url object pointing to the break lock form.
   + *
   + * Usage example:
   + * @code
   + * $build['examples_lock'] = [
   + *   '#type' => 'break_lock_link',
   + *   '#label' => $this->t('example item'),
   + *   '#lock' => $tempstore->getMetadata('example_key'),
   + *   '#url' => \Drupal\Core\Url::fromRoute('examples.break_lock_form'),
   + * ];
   + * @endcode
   + *
   + * @RenderElement("break_lock_link")
   + */
   +class BreakLockLink extends RenderElement implements ContainerFactoryPluginInterface {
   

   Nice addition; this makes it much cleaner

2. +++ b/core/lib/Drupal/Core/TempStore/Lock.php
   @@ -0,0 +1,72 @@
   +final class Lock {
   ...
   +  private $ownerId;
   

   It makes sense for these to be final and private under current policy since this is just a value object that will be constructed inline, and therefore inherently not swappable.

3. +++ b/core/lib/Drupal/Core/TempStore/Lock.php
   @@ -0,0 +1,72 @@
   +  public function __get($name) {
   
   +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -48,12 +49,14 @@ class ViewUI implements ViewEntityInterface {
   +   * For backwards compatibility, public access to this property is provided by
   +   * ::__set() and ::__get().
   

   +1 for adding this BC layer. @tim.plunkett and I discussed this in Slack awhile back but I forgot to document on the issue.

4. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1351,4 +1355,63 @@ public function addCacheTags(array $cache_tags) {
   +  public function __set($name, $value) {
   +    if ($name === 'lock') {
   +      @trigger_error('Using the "lock" public property of a View is deprecated in Drupal 8.7.0 and will not be allowed in Drupal 9.0.0. Use \Drupal\views_ui\ViewUI::setLock() instead. See https://www.drupal.org/node/3025869.', E_USER_DEPRECATED);
   +      if ($value instanceof \stdClass && property_exists($value, 'owner') && property_exists($value, 'updated')) {
   +        $value = new Lock($value->owner, $value->updated);
   +      }
   +      $this->setLock($value);
   +    }
   +    else {
   +      $this->{$name} = $value;
   +    }
   +  }
   

   The reason this part of the BC layer is only provided on the Views UI form is that setting it isn't that useful generically; we just have to let Views access it still because everything is public there. (The two properties in the getter BC layer are the actual values in the value object.)

5. +++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
   @@ -90,6 +91,47 @@ public function testIsLocked() {
   +    $lock = new Lock(2, (int) $_SERVER['REQUEST_TIME']);
   ...
   +    $lock = new Lock(1, (int) $_SERVER['REQUEST_TIME']);
   

   I was a bit alarmed on the hardcoded UIDs of 2 and 1 here -- seems brittle and hardcoding serial IDs in a test runner has caused us criticals before -- but the test already has:

       $lock = (object) [
         'owner' => 2,
         'data' => [],
         'updated' => (int) $_SERVER['REQUEST_TIME'],
       ];
   

   (etc.)

   So we don't need to fix that here.

6. +++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
   @@ -90,6 +91,47 @@ public function testIsLocked() {
   +    // A view_ui without a lock object is not locked.
   +    $this->assertFalse($view_ui->isLocked());
   

   False value is FALSE!

All this looks good to me now. Committed and pushed to 8.7.x and published the CR. Thanks!