Problem/Motivation

The ViewExecutable class still has a lot of public properties, let's remove some of them, which aren't actually needed:


  /**
   * The current page. If the view uses pagination.
   *
   * @var int
   */
  public $current_page = NULL;

  /**
   * The number of items per page.
   *
   * @var int
   */
  public $items_per_page = NULL;

  /**
   * The pager offset.
   *
   * @var int
   */
  public $offset = NULL;

  public function setCurrentPage($page) {}
  public function getCurrentPage($page) {}

  public function getItemsPerPage($page) {}
  public function setItemsPerPage($page) {}

  public function getOffset($page) {}
  public function setOffset($page) {}

Proposed resolution

Replace public with protected and search for all usages of those and replace it with the appropriate method call.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#10 interdiff.txt1.76 KBdawehner
#10 2359703-10.patch3.96 KBdawehner
#4 2359703-4.patch2.42 KBmichaellenahan

Comments

michaellenahan’s picture

michaellenahan commented
Assigned: Unassigned » michaellenahan
michaellenahan’s picture

michaellenahan commented

I'm confused!

/**
   * The current page. If the view uses pagination.
   *
   * @var int
   */
  public $current_page = NULL;

Should $current_page not properly be *private* scope?
It is not used anywhere else except in the ViewExecutable class, and it is wrapped by getter and setter functions.

  public function setCurrentPage($page) {}
  public function getCurrentPage($page) {}

Both of these are referenced by Test classes, so should they not remain as public scope?

dawehner’s picture

dawehner
Primary language German
commented

Well, we don't use protected in Drupal, because subclasses can't really access it. You never know, maybe someone comes up with a subclass of ViewExecutable and then you are kinda fucked when stuff is private instead of just protected.

michaellenahan’s picture

michaellenahan commented
StatusFileSize
new 2359703-4.patch2.42 KB

OK. So it's just the *properties* that get set to protected. I have to learn to read better. :)

I see now that it's the getter/setter methods which allow us to set the property to protected.

Here's the patch.

As a follow-up, do the following properties need getter/setter methods, so that they can be set to protected?
- $total_rows
- $attachment_before
- $attachment_after
- $exposed_data
- $exposed_raw_input

... or is it overkill.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review

Thank you for the work! Let's set the issue to needs review in order to figure out what the testbot thinks about it ...

- $total_rows
- $attachment_before
- $attachment_after
- $exposed_data
- $exposed_raw_input

We can work on so many things, do you want to create an additional issue for that?

Status: Needs review » Needs work

The last submitted patch, 4: 2359703-4.patch, failed testing.

Status: Needs work » Needs review

michaellenahan queued 4: 2359703-4.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2359703-4.patch, failed testing.

michaellenahan’s picture

michaellenahan commented

Not sure what is going on here.
Test fails because ViewExecutable::setDisplay() is called without an argument.
The patch makes no change to SetDisplay() though.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2359703-10.patch3.96 KB
new interdiff.txt1.76 KB

@michaellenahan
Well, the problem is not the debug message (that one is actually intended)
but the problem is that the property is not accessible anymore (as you marked it as protected).
Instead we should check for the default value of the method result. Sadly this opens up another bug, as the display handlers might not be defined
at some point in time.

Here is an interdiff which fixes the problem.

Do you want another issue?

michaellenahan’s picture

michaellenahan commented

Do you want another issue?

Yes please :)

michaellenahan’s picture

michaellenahan commented
Assigned: michaellenahan » Unassigned
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Well, the actual work was done without my work already.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a prioritized change as it reduces fragility by removing public access to properties that should not be and so is allowed as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 08bf45f and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views/src/Tests/ViewExecutableTest.php b/core/modules/views/src/Tests/ViewExecutableTest.php
index 775b740..478532d 100644
--- a/core/modules/views/src/Tests/ViewExecutableTest.php
+++ b/core/modules/views/src/Tests/ViewExecutableTest.php
@@ -20,7 +20,6 @@
 use Drupal\views\Plugin\views\pager\PagerPluginBase;
 use Drupal\views\Plugin\views\query\QueryPluginBase;
 use Drupal\views_test_data\Plugin\views\display\DisplayTest;
-use Symfony\Component\DependencyInjection\Container;
 use Symfony\Component\HttpFoundation\Response;
 
 /**

Unused use added.

  • alexpott committed 08bf45f on 8.0.x 
    Issue #2359703 by dawehner, michaellenahan: Remove public visibility...

Status: Fixed » Closed (fixed)

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