Problem/Motivation

\Drupal\views\ViewExecutable::$style_plugin and \Drupal\views\ViewExecutable::$rowPlugin
should have corresponding methods, one could call.

Once we do that, we can also mark the public properties as deprecated

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#53 interdiff_50-52.txt14.84 KBrahulkhandelwal1990
#52 2719173-52.patch21.59 KBrahulkhandelwal1990
#50 interdiff_49-50.txt526 bytesrahulkhandelwal1990
#50 2719173-50.patch2.85 KBrahulkhandelwal1990
#49 interdiff_48-49.txt1.88 KBvarshith
#49 2719173-49.patch2.19 KBvarshith
#48 interdiff_41-48.txt1.97 KBrahulkhandelwal1990
#48 2719173-48.patch2.19 KBrahulkhandelwal1990
#41 2719173-41.patch2.67 KBmrweiner
#34 2719173-34.patch2.67 KBhardikpandya
#34 interdiff_31-34.txt555 byteshardikpandya
#31 2719173-31.patch2.71 KBaburrows
#24 2719173-24.patch2.71 KBZeiP
#21 interdiff-2719173-19-21.txt1.2 KBVinay15
#21 2719173-21.patch2.72 KBVinay15
#19 interdiff-2719173-17-19.txt4.29 KBVinay15
#19 2719173-19.patch2.72 KBVinay15
#17 interdiff-2719173-13-17.txt1.87 KBVinay15
#17 update-function-docs-2719173-17.patch2.09 KBVinay15
#13 add-2719173-13.patch2.03 KBzaurav
#8 add-2719173-8.patch1.73 KBzaurav
#3 add-2719173-3.patch1.78 KBzaurav
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

zaurav’s picture

Assigned: Unassigned » zaurav

Working on this at the DrupalConNOLA Sprint

zaurav’s picture

rakesh.gectcr’s picture

Status: Active » Needs review

@zaurav

Next time Upload the patch , Please update the status to review 'Needs review'.

mikeker’s picture

Status: Needs review » Needs work

@zaurav, thank for working on this!

+++ b/core/modules/views/src/ViewExecutable.php
@@ -214,15 +214,22 @@ class ViewExecutable implements \Serializable {
+  private $style_plugin;
...
+  private $rowPlugin;

These should be protected so that they can be overridden by child classes. Other than that, it looks great.

The last submitted patch, 3: add-2719173-3.patch, failed testing.

mikeker’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -214,15 +214,22 @@ class ViewExecutable implements \Serializable {
-  public $style_plugin;
+  private $style_plugin;
...
-  public $rowPlugin;
+  private $rowPlugin;

Heh, completely spaced on this... These need to remain public for now, but marked as @deprecated (as you have done). Since we shipped 8.0 with these as public methods, it's an API change to change that.

zaurav’s picture

Assigned: zaurav » Unassigned
Status: Needs work » Needs review
FileSize
1.73 KB

@mikeker, thank you for helping me.

2nd try.

mikeker’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/ViewExecutable.php
@@ -221,6 +224,10 @@ class ViewExecutable implements \Serializable {
+   * ¶

Looks like a trailing whitespace got in the last change...

dawehner’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -847,7 +854,7 @@ public function newDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
-  public function getStyle() {
+  public function getStylePlugin() {

Let's not rename this function ...

mikeker’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -214,6 +214,9 @@ class ViewExecutable implements \Serializable {
+   * @deprecated in Drupal 8.0.x and will be removed before 9.0.0. Use

@@ -221,6 +224,10 @@ class ViewExecutable implements \Serializable {
+   * @deprecated in Drupal 8.0.x and will be removed before 9.0.0. Use

Sorry, missed this the first time. Technically, we're deprecating this in 8.2.x.

The last submitted patch, 8: add-2719173-8.patch, failed testing.

zaurav’s picture

Implemented changes from #9 #10 #11.

Unsure what to return in the case of $rowPlugin empty.

mikeker’s picture

Status: Needs work » Needs review
Issue tags: +neworleans2016

@zaurav, Don't forget to set the status to Needs Review so the testbot tests the new patch.

Tagging for NOLA sprints. I mentored @zaurav on this issue.

Mile23’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -214,6 +214,9 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated in Drupal 8.0.x and will be removed before 8.2.x. Use
    
    @@ -221,6 +224,10 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated in Drupal 8.0.x and will be removed before 8.2.x. Use
    
    @@ -846,6 +853,8 @@ public function newDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
    +   * @deprecated in Drupal 8.0.x and will be removed before 8.2.x.
    

    I'm pretty sure we can't deprecate for 8.2.x, and need to target 9.0.0, particularly for a getter method.

    Also I doubt we'd really be *removing* these properties. Maybe say we'll be removing access to them.

  2. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -221,6 +224,10 @@ class ViewExecutable implements \Serializable {
        * The current used row plugin, if the style plugin supports row plugins.
    
    @@ -856,6 +865,35 @@ public function getStyle() {
    +  /**
    +   * Gets the current row plugin.
    

    The accessor doc line should match the property doc line, with 'Gets...' at the beginning.

  3. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -856,6 +865,35 @@ public function getStyle() {
    +   * @return \Drupal\views\Plugin\views\row\RowPluginBase
    +   *   The current row plugin.
    +   */
    +  public function getRowPlugin() {
    +    if (!isset($this->rowPlugin)) {
    +      return;
    +    }
    

    Since we can return NULL, then @return should be \Drupal\views\Plugin\views\row\RowPluginBase|null and explain when the method will return NULL.

Vinay15’s picture

Assigned: Unassigned » Vinay15
Vinay15’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
1.87 KB

Hi Paul, I have worked on your suggestions and updated the patch.

dawehner’s picture

Thank you for picking up the issue!

  1. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -214,6 +214,9 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated as of Drupal 8.0.x and access will be removed in Drupal 9.0.0.
    
    @@ -221,6 +224,9 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated as of Drupal 8.0.x and access will be removed in Drupal 9.0.0.
    
    @@ -846,6 +852,8 @@ public function newDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
    +   * @deprecated as of Drupal 8.0.x and access will be removed in Drupal 9.0.0.
    

    Should be '8.2.x' I think :)

  2. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -856,6 +864,34 @@ public function getStyle() {
       /**
    +   * Gets the current style plugin.
    +   *
    +   * @return \Drupal\views\Plugin\views\style\StylePluginBase
    +   *   The current style plugin.
    +   */
    +  public function getStylePlugin() {
    +    if (!isset($this->style_plugin)) {
    +      $this->initStyle();
    +    }
    +
    +    return $this->style_plugin;
    +  }
    

    Let's getStyle() call to getStylePlugin()

  3. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -856,6 +864,34 @@ public function getStyle() {
    +    if (!isset($this->rowPlugin)) {
    +      return;
    +    }
    +
    +    return $this->rowPlugin;
    

    This should initialize the row plugin otherwise, IMHO, so use $display->getPlugin('row'). Once this is done, \Drupal\views\Plugin\views\style\StylePluginBase::init could call out to just $view->getRowPlugin()

Vinay15’s picture

Hi Daniel, I have tried to update the patch with your suggestions. Please correct me if I am wrong.

mikeker’s picture

Status: Needs review » Needs work

@Vinay15, thank you for your work on this issue!

  1. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -214,6 +214,9 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated as of Drupal 8.0.x and access will be removed in Drupal 8.2.x.
    +   *   Use getStylePlugin() instead to get this property.
    
    @@ -221,6 +224,9 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated as of Drupal 8.0.x and access will be removed in Drupal 8.2.x.
    +   *   Use getRowPlugin() instead to get this property.
    

    Went the wrong way on this one. :) When @dawehner said it should be 8.2.x, he meant it should read "@deprecated as of Drupal 8.2.x and access will be removed in Drupal 9.0.0."

    We can't actually change the access during the 8.x cycle as that would break backwards compatibility. So we mark as deprecated (as of 8.2.x) and change access to protected in the 9.x branch.

  2. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -846,8 +852,20 @@ public function newDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
    +   * @deprecated as of Drupal 8.0.x and access will be removed in Drupal 8.2.x.
    

    Similar to the above, but should read "@deprecated as of Drupal 8.2.x and will be removed in Drupal 9.0.0."

    In this case the old method will be removed -- in the previous cases the property will remain but will go from public to protected.

Sorry for the nitpicks, but getting the version numbers correct is important when deprecating things.

Vinay15’s picture

Hey Mike, thanks a lot for correcting me.. :) I have updated the patch.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ZeiP’s picture

Re-rolling against 8.4.x.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -213,6 +213,9 @@ class ViewExecutable implements \Serializable {
    +   * @deprecated as of Drupal 8.2.x and access will be removed in Drupal 9.0.0.
    

    The version numbers need updating again, sadly.

  2. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -845,8 +851,20 @@ public function newDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
    +   * @deprecated as of Drupal 8.2.x and will be removed in Drupal 9.0.0.
    

    Docs should say what to use instead. I know it's in the source 2 lines lower, but on api.d.org, you only see the source if you click the 'show source' button.

Eli-T’s picture

Issue tags: +DistributedSprintUK18
aburrows’s picture

Working on this at DistributedSprintUK18

aburrows’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

Latest patch rerolled with 8.7x added

Status: Needs review » Needs work

The last submitted patch, 31: 2719173-31.patch, failed testing. View results

joachim’s picture

#28.2 still needs fixing.

hardikpandya’s picture

Assigned: Vinay15 » Unassigned
Status: Needs work » Needs review
FileSize
555 bytes
2.67 KB

Status: Needs review » Needs work

The last submitted patch, 34: 2719173-34.patch, failed testing. View results

shubhangi1995’s picture

Assigned: Unassigned » shubhangi1995
joachim’s picture

Latest patch looks good, but I don't think it'll make it into 8.7.x, so the comments will need changing to say 8.8.x.

vacho’s picture

If the patch deprecate the variable $rowPlugin why the new code use it?

 /**
   * Gets the current row plugin.
   *
   * @return \Drupal\views\Plugin\views\row\RowPluginBase
   *   The current row plugin.
   */
  public function getRowPlugin() {
    if (!isset($this->rowPlugin)) {
      $this->rowPlugin = $this->display_handler->getPlugin('row');
    }

    return $this->rowPlugin;
  }
joachim’s picture

Because it's going to remain as an internal property. The point of this issue is to add accessors for these properties so that in D9 they can be changed to protected. The class itself can and should still refer to it.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mrweiner’s picture

Update per #37 to update references from 8.7.x to 8.8.x

Status: Needs review » Needs work

The last submitted patch, 41: 2719173-41.patch, failed testing. View results

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

surabhi-gokte’s picture

Assigned: shubhangi1995 » Unassigned
rahulkhandelwal1990’s picture

Created a new patch and resolved Custom Commands Failed for 9.3.x in #41

varshith’s picture

Formatting fixes and fix issue link in doc.

rahulkhandelwal1990’s picture

Added one missing file from last patch(#41)

rahulkhandelwal1990’s picture

Status: Needs work » Needs review
rahulkhandelwal1990’s picture

FileSize
21.59 KB

Getting below phpunit issues after this patch
1x: The "PHPUnit\TextUI\ResultPrinter" class is considered internal This class is not covered by the backward compatibility promise for PHPUnit. It may change without further notice. You should not use it from "Drupal\Tests\Listeners\HtmlOutputPrinter".
1x in DrupalListener::endTest from Drupal\Tests\Listeners

1x: The "Drupal\Tests\Listeners\DrupalListener" class implements "PHPUnit\Framework\TestListener" that is deprecated Use the `TestHook` interfaces instead.
1x in DrupalListener::endTest from Drupal\Tests\Listeners

1x: The "Drupal\Tests\Listeners\DrupalListener" class uses "PHPUnit\Framework\TestListenerDefaultImplementation" that is deprecated The `TestListener` interface is deprecated.
1x in DrupalListener::endTest from Drupal\Tests\Listeners

Seems these are deprecation issues due to phpunit version. See https://www.drupal.org/project/drupal/issues/3125809

rahulkhandelwal1990’s picture

FileSize
14.84 KB

Interdiff of #52

Status: Needs review » Needs work

The last submitted patch, 52: 2719173-52.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.