Refactoring ViewStorageInterface to ViewEntityInterface

I've included a patch which adds:

getBaseTableName();
getBaseFieldName();
getDescription();
getTags();
getCoreVersion();
getViewExecutable();
getModuleName();

I didn't think including setters for any property made sense since most of these properties are set implicitly by the ViewExecutable through the system and shouldn't be modified individually, certainly not through the View entity directly I think?

Not sure how this patch could fail since it only modifies documentation and adds code that isn't called yet (new methods), can anyone explain that or is this just a (temporary) test-bot failure?

#1: view-entity-expand-view-with-methods-2030667-1.patch queued for re-testing.

I think it was the use Drupal\views\Drupal...
Anyway, we've also discussed renaming the interface, might as well do that here.

Oh! I realized why the other patch failed. We needed to add this to ViewUI.

Taggin.g

+++ b/core/modules/views/lib/Drupal/views/ViewInterface.php
@@ -0,0 +1,129 @@
+   * @return int

That is a string.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
@@ -15,12 +15,12 @@
@@ -141,10 +141,10 @@ class ViewUI implements ViewStorageInterface {

I think we should extend the test coverage of ViewUI then.

.

Updated issue summary.

8: vdc-2030667-8.patch queued for re-testing.

This patch is a year old so it needs a reroll

working on this

ok this will not get back green but need to get some feedback from testbot.

notable changes

1. removed getViewExecutable and used getExecutable that is on head instead.

2. ViewExecutable __construct was wrong, the interface declared parameters that wasn't implemented, the dochead needs still to be updated for that.

3. newDisplay in View entity, not sure if it is really needed but let it stay.

ok so hopefully this will be green now. and if it is then time to check if it do what we are aiming for.

1. The class variable $id is not set protected.
2. You have replaced ViewStorageInterface with ViewInterface. Why? It is not a rename action. The old one does not get deleted. Why?
3. The ViewInterface is missing the definition of the method duplicateDisplayAsType().
4. The EntityStorageInterface is not implemented for the views module.

5. +++ b/core/modules/views/src/Entity/View.php
   @@ -461,4 +465,46 @@ class View extends ConfigEntityBase implements ViewStorageInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getBaseTableName() {
   +    return $this->base_table;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getBaseFieldName() {
   +    return $this->base_field;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDescription() {
   +    return $this->description;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getTags() {
   +    return $this->tag;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCoreVersion() {
   +    return $this->core;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getModuleName() {
   +    return $this->module;
   +  }
   +
   
   +++ b/core/modules/views/src/ViewInterface.php
   @@ -0,0 +1,129 @@
   +  /**
   +   * Returns the name of the base table this view will use.
   +   *
   +   * @return string
   +   *   The name of the base table for this view.
   +   */
   +  public function getBaseTableName();
   +
   +  /**
   +   * Returns the name of the base field to use.
   +   *
   +   * @return string
   +   *   The name of the base field for this view.
   +   */
   +  public function getBaseFieldName();
   +
   +  /**
   +   * Returns the description of the view, which is used only in the interface.
   +   *
   +   * @return string
   +   *   The description of this view used in the interface.
   +   */
   +  public function getDescription();
   +
   +  /**
   +   * Returns the "tags" of a view.
   +   *
   +   * The tags are stored as a single string, though it is used as multiple tags
   +   * for example in the views overview.
   +   *
   +   * @return string
   +   *   A single string of multiple tags added to this view.
   +   */
   +  public function getTags();
   +
   +  /**
   +   * Returns the core version this view was created for.
   +   *
   +   * @return int
   +   *   A drupal core version string such as DRUPAL_CORE_COMPATIBILITY (8.x)
   +   */
   ...
   +  /**
   +   * Returns the name of module implementing this view.
   +   *
   +   * @return string
   +   *   The name of the module implementing this view.
   +   */
   +  public function getModuleName();
   

   Why do you add these methods. They are never used, so please remove them.

6. +++ b/core/modules/views/src/ViewInterface.php
   @@ -0,0 +1,129 @@
   +  /**
   +   * Returns a reference to the executable version of this view.
   +   * If none was set previously, it is created before returning it.
   +   *
   +   * @return \Drupal\views\ViewExecutable
   +   *   A reference to the executable version of this view.
   +   */
   +  public function getExecutable();
   

   This method is not used and is not implemented by the class View.

7. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1168,4 +1168,66 @@ class ViewUI implements ViewStorageInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function addDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
   +    return $this->storage->addDisplay($plugin_id, $title, $id);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function newDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
   +    return $this->storage->newDisplay($plugin_id, $title, $id);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getBaseTableName() {
   +    return $this->storage->getBaseTableName();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getBaseFieldName() {
   +    return $this->storage->getBaseFieldName();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDescription() {
   +    return $this->storage->getDescription();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getTags() {
   +    return $this->storage->getTags();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCoreVersion() {
   +    return $this->storage->getCoreVersion();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getViewExecutable() {
   +    return $this->storage->getViewExecutable();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getModuleName() {
   +    return $this->storage->getModuleName();
   

   Why are these added? If $this->storage is the class View, then they inherit the methods from View. Also they are never used, so please remove them.

ok so everything in comment #21 except for 21.1 for now. and 21.4 I don't understand that sentence.

The issue needs a better issue summary and beta evaluation.

1. +++ b/core/modules/node/tests/src/Unit/Plugin/views/field/NodeBulkFormTest.php
   @@ -62,7 +62,7 @@ class NodeBulkFormTest extends UnitTestCase {
   -    $storage = $this->getMock('Drupal\views\ViewStorageInterface');
   +    $storage = $this->getMock('Drupal\views\ViewInterface');
   

   That itself makes sense now ... this is a relict from earlier times BUT the issue summary / beta evaluation should describe how much problems are potentially caused by renaming the interface (small, given that the interface name is not simple to fix).

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -461,4 +447,46 @@ class View extends ConfigEntityBase implements ViewStorageInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCoreVersion() {
   +    return $this->core;
   +  }
   +
   

   Can we open up a follow up to kill this ? I think its not something you really need anymore ... it used to be used for version updates between 6 and 7.

3. +++ b/core/modules/views/src/ViewInterface.php
   @@ -53,4 +53,23 @@
   +  /**
   +   * Adds a new display handler to the view, automatically creating an ID.
   +   *
   +   * @param string $plugin_id
   +   *   (optional) The plugin type from the Views plugin annotation. Defaults to
   +   *   'page'.
   +   * @param string $title
   +   *   (optional) The title of the display. Defaults to NULL.
   +   * @param string $id
   +   *   (optional) The ID to use, e.g., 'default', 'page_1', 'block_2'. Defaults
   +   *   to NULL.
   +   *
   +   * @return string|bool
   +   *   The key to the display in $view->display, or FALSE if no plugin ID was
   +   *   provided.
   +   */
   +  public function addDisplay($plugin_id = 'page', $title = NULL, $id = NULL);
   

   In an ideal world the View should not have these methods but rather that logic should live in a separate place, but sure, this is out of scope of this issue.

I thought that I had removed all not used functions from the View class but apparently only in the interface. done that in this and also protected the property id.

Renaming of the Class from ViewStorageInterface to ViewInterface is as far as I can see a clear API change and hence it shouldn't be done in this face we are know.

The effort to do the renaming and the effects of it seems to be small, I guess it wouldn't be a huge problem to change it back either. I think it wuld be best that we let committers take a look and let them decide.

@filijonka: Good work!

In the class Entity/View you change the docblock for a number of public methods to inheritdoc. Can you do the same for the public method getExecutable().

For the rest of the patch is it RTBC from me.

waiting for a decision from view maintainers on this issue before going on.

@filijonka
Please update the issue summary according to our conversation.

ok let's hope we got this right

That itself looks fine!

It would be nice if the summary could contain a justification for the interface rename. I know @dawehner and @damiankloip want to do this and I don;t think this is a disruptive change since everything outside of views that wants to typehint on a view would want a ViewExecutable - which is exactly what all the views plugins do.

1. +++ b/core/modules/user/src/Tests/Views/AccessRoleTest.php
   @@ -9,7 +9,7 @@
   -use Drupal\views\ViewStorageInterface;
   +use Drupal\views\ViewEntityInterface;
   
   +++ b/core/modules/views/src/Plugin/views/wizard/WizardInterface.php
   @@ -54,7 +54,7 @@
       *   The current state of the wizard form.
   

   Just remove the use - this is not used here.

2. +++ b/core/modules/views/tests/modules/views_test_data/views_test_data.views_execution.inc
   @@ -7,7 +7,7 @@
   -use Drupal\views\ViewStorageInterface;
   +use Drupal\views\ViewEntityInterface;
   

   Remove the use - it's not used.

3. +++ b/core/modules/views_ui/views_ui.module
   @@ -10,7 +10,7 @@
   -use Drupal\views\ViewStorageInterface;
   +use Drupal\views\ViewEntityInterface;
   

   Just remove ViewStorageInterface - it is not actually used in the file :)

Updated the IS with the justification of the interface rename.

Novice TODO:
- 3 small updates to the current patch. See the comment #33 from @alexpott.

I'm trying to remove useless 'use'

It all looks good to me.
All the changes that are wanted by @alexpott are implemented.
Back to RTBC.

ViewsStorage/EntityInterface is not used in external typehints - ViewsExecutable is.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed efbaa69 and pushed to 8.0.x. Thanks!

thanks @daffie for updating the IS.