\Drupal\views_ui\ViewListBuilder

class ViewListBuilder extends ConfigEntityListBuilder {
  // ...

  /**
   * {@inheritdoc}
   */
  protected $limit;

  // ...

  public function __construct( ... ) {
    // ...
    $this->limit = FALSE;
  }

  // ...

}

The $limit variable is a property defined on the parent EntityListBuilder class and is inherited by ViewListBuilder. There is no need to redefine the variable in this subclass. Note that $limit is only used from within the code of the parent classes - there is no other usage of $limit inside ViewListBuilder.

I confess I can't think of a situation (in PHP) where this would cause an error in execution, but I feel this obscures what is going on in the code. I think it would make the code clearer if the override were removed.

This is also the case in 

core/modules/block/src/BlockListBuilder.php
core/lib/Drupal/Core/Config/Entity/DraggableListBuilder.php

which we should also clean up while we are at it.

CommentFileSizeAuthor
#14 3033986-14-useless-property-override.patch1.94 KBTR
#6 Useless-property-override-view-3033986-6.patch731 bytesrishabhthakur
useless-override.patch560 bytesTR
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR created an issue. See original summary.

TR’s picture

TR CreditAttribution: TR commented
Issue tags: +Novice

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.

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.

rishabhthakur’s picture

rishabhthakur CreditAttribution: rishabhthakur at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » rishabhthakur
rishabhthakur’s picture

rishabhthakur CreditAttribution: rishabhthakur at Srijan | A Material+ Company for Drupal India Association commented
FileSize
Useless-property-override-view-3033986-6.patch731 bytes

Updated patch to fix Notice error with protected variable

Status: Needs review » Needs work

The last submitted patch, 6: Useless-property-override-view-3033986-6.patch, failed testing. View results

KapilV’s picture

KapilV CreditAttribution: KapilV as a volunteer and at OpenSense Labs commented

protected $limit; Variable using for testing of multiple views.

<?php

namespace Drupal\Tests\views_ui\Functional;

use Drupal\views\Entity\View;
use Drupal\views\Views;

/**
* Tests the views list.
*
* @group views_ui
*/
class ViewsListTest extends UITestBase {

/**
* Modules to enable.
*
* @var array
*/
public static $modules = ['block', 'views_ui'];

/**
* {@inheritdoc}
*/
protected $defaultTheme = 'stark';

/**
* A user with permission to administer views.
*
* @var \Drupal\user\Entity\User
*/
protected $adminUser;

/**
* {@inheritdoc}
*/
protected function setUp($import_test_views = TRUE) {
parent::setUp($import_test_views);

$this->drupalPlaceBlock('local_tasks_block');
$this->drupalPlaceBlock('local_actions_block');
$this->adminUser = $this->drupalCreateUser(['administer views']);
$this->drupalLogin($this->adminUser);
}

/**
* Tests that the views list does not use a pager.
*/
public function testViewsListLimit() {
// Check if we can access the main views admin page.
$this->drupalGet('admin/structure/views');
$this->assertResponse(200);
$this->assertLink(t('Add view'));

// Count default views to be subtracted from the limit.
$views = count(Views::getEnabledViews());

// Create multiples views.
$limit = 51;
$values = $this->config('views.view.test_view_storage')->get();
for ($i = 1; $i <= $limit - $views; $i++) {
$values['id'] = 'test_view_storage_new' . $i;
unset($values['uuid']);
$created = View::create($values);
$created->save();
}
$this->drupalGet('admin/structure/views');

// Check that all the rows are listed.
$this->assertEqual(count($this->xpath('//tbody/tr[contains(@class,"views-ui-list-enabled")]')), $limit);
}

}
It's not Useless property.

TR’s picture

TR CreditAttribution: TR commented
Assigned: rishabhthakur » TR
Status: Needs work » Needs review

@rishabhthakur: Why did you remove that line of code? Your patch is clearly wrong and fails the core test.

@kapilkumar0324: Please don't post large blocks of unformatted code in the issue queue. It's hard to read and confuses the issue.

It's not Useless property.

No one said it's a useless property. My original post says it's a useless property OVERRIDE. The property is still needed and still exists in my original patch.

Both of you have totally misunderstood the original issue. I have triggered a re-test of that original patch and as you can see the patch still works with no test failures and no coding standards violations.

This issue is about elementary object-oriented programming. We have this situation:

class A {
  protected $limit;
}

class B extends A {
  protected $limit; // This is not needed.

  public function __construct() {
    $this->limit = FALSE; // This is absolutely needed.
  }
}

class B inherits the definition of $limit, it does not need to redeclare and override the $limit property. It's that simple.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Assigned: TR » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Agree with @TR here. Original patch https://www.drupal.org/files/issues/2019-02-18/useless-override.patch is RTBC. Confirmed that projected $limit lives in core/lib/Drupal/Core/Entity/EntityListBuilder.php, the grandparent of this class, so we don't need our own override of it.

Removing 'Novice' tag since apparently novices don't necessarily understand object oriented coding and inheritance, nor read issue summaries carefully. ;)

Cheers,
-Derek

rishabhthakur’s picture

rishabhthakur CreditAttribution: rishabhthakur at Srijan | A Material+ Company for Drupal India Association commented

Hi TR,
You're right. Sorry for the confusion :)
its clearly mention on title "Useless property override in ViewListBuilder"

KapilV’s picture

KapilV CreditAttribution: KapilV as a volunteer and at OpenSense Labs commented

Hi TR,
Sorry for the misunderstanding. You are right.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented

Doubt that this is not the only one in core. There are more to clean up. Needs follow-up or enlarge the scope?

TR’s picture

TR CreditAttribution: TR commented
FileSize
3033986-14-useless-property-override.patch1.94 KB

I found that 3 out of the 38 core list builders have this problem. These are:

core/modules/views_ui/src/ViewListBuilder.php
core/modules/block/src/BlockListBuilder.php
core/lib/Drupal/Core/Config/Entity/DraggableListBuilder.php

I'm fine with expanding this issue as long as we restrict the scope to just this one $limit property for classes that extend from the core EntityListBuilder, and don't try to find or solve all other useless property overrides in core. Here's a new patch.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Component: views_ui.module » entity system
Category: Bug report » Task
Issue summary: View changes

Doing all overrides of $limit sounds good as a scope I think. Re-checked that these are the only occurrences in core and they are (there are more in contrib).

Updated to task since this is not a bug, updated the title and IS to match the new direction.

Leaving RTBC cause the new patch looks good and it is nice that core now does the override of the pager setting in a consistent way.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Title: Useless property override in ViewListBuilder » Useless property override in multiple classes extending EntityListBuilder

really updated the title this time...

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Title: Useless property override in multiple classes extending EntityListBuilder » Remove useless 'limit' property override in multiple classes extending EntityListBuilder

Even more specific/accurate title. ;)

RTBC +1

Thanks!
-Derek

  • catch committed 3bcb729 on 9.1.x 
    Issue #3033986 by TR, rishabhthakur, dww, Lendude, jungle: Remove...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.9.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 3bcb729 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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