@dawehner thinks this might be a duplicate of an existing issue, but I wanted to make sure to document it until we confirm.

Steps to reproduce

  1. Install 8.x Standard.
  2. Create a new view of users with a page display and path users-by-role.
  3. Add a contextual filter for the user role.
    role_contextual_filter.png
  4. Go to users-by-role/administrator. You get the fatal:
    Fatal error: Call to a member function label() on a non-object in /Applications/MAMP/htdocs/d8git/core/modules/user/lib/Drupal/user/Plugin/views/argument/RolesRid.php on line 23
    
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

Would be cool IMO to paste the view yaml into the bug report so we can skip creating the view. Hope we get there one day.

Anonymous’s picture

Assigned: Unassigned »
Status: Active » Needs review
FileSize
601 bytes

It looks like the value used in the call to entity_load is an array when it should be a simple value. Additionally, if the value couldn't be found, entity_load returns FALSE, so the call to ->label() was failing in those cases.

tstoeckler’s picture

Status: Needs review » Needs work

The patch makes it return a boolean, though. I think the code should be something like

if ($entity = entity_load(...)) {
  return $entity->label();
}
Anonymous’s picture

Status: Needs work » Needs review
FileSize
627 bytes

Good catch! Rerolled.

Anonymous’s picture

Changed last return value for clarity.

Status: Needs review » Needs work
Issue tags: -VDC, -Configurables

The last submitted patch, drupal-1995868-05-Role-Contextual-Filter-WSOD.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +Configurables
tstoeckler’s picture

Looks good to me, but I don't know Views enough to RTBC myself.

andymartha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
50.94 KB
38.72 KB
49.37 KB

On a standard download of Drupal 8, the steps outlined in the issue summary by xjm produced a white screen of death for me. See screenshot.

After applying patch drupal-1995868-05-Role-Contextual-Filter-WSOD.patch from #5 by JoshuaRogers to a fresh Drupal 8 install, the path correctly gave me the view in Drupal instead of the WSOD. See screenshot with no perceived detriment in functionality. Good job!

Role-Contextual-Filter-before1.png

Role-Contextual-Filter-before2.png

Role-Contextual-Filter-after.png

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We need a test for this...

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
8.16 KB

Let's phpunit that!

dawehner’s picture

Issue tags: +PHPUnit
katbailey’s picture

Just a couple of comments about the test as I was chatting with dawehner in irc about it...

+++ b/core/modules/user/tests/Drupal/Tests/user/Views/Argument/RolesRidTest.phpundefined
@@ -0,0 +1,99 @@
+    // NULL forces to disable mocking on any method by default.

I'd prefer a comment here about what we're actually doing, rather than about how PHPUnit behaves, e.g. "Create a stub role storage controller that replaces the attachLoad method."

+++ b/core/modules/user/tests/Drupal/Tests/user/Views/Argument/RolesRidTest.phpundefined
@@ -0,0 +1,99 @@
+
+    $container = new ContainerBuilder();
+    $container->set('plugin.manager.entity', $entity_manager);
+    \Drupal::setContainer($container);

Why is this needed? I thought our PHPUnit tests could be a container-free zone. Wah.

dawehner’s picture

FileSize
1.96 KB
8.39 KB

Why is this needed? I thought our PHPUnit tests could be a container-free zone. Wah.

Sadly not at the moment but I think there is no reason to not inject it all the time?

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -363,7 +363,7 @@ public function createDuplicate() {
-    return entity_get_info($this->entityType);
+    return \Drupal::entityManager()->getDefinition($this->entityType());

Nice!

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/argument/RolesRid.phpundefined
@@ -19,8 +22,46 @@
+   * The role entity storage controller
+   */
+  protected $roleStorageController;

Missing @var

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/argument/RolesRid.phpundefined
@@ -19,8 +22,46 @@
+   * @{inheritdoc}

{@inheritdoc

+++ b/core/tests/Drupal/Tests/UnitTestCase.phpundefined
@@ -7,6 +7,9 @@
+use Drupal\Core\Config\Config;
+use Drupal\Core\Config\ConfigFactory;

Where are these used?

dawehner’s picture

FileSize
1.39 KB
8.58 KB

Where are these used?

Ups.

dawehner’s picture

Assigned: » Unassigned
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks great!

xjm’s picture

xjm’s picture

Just realized there was never a test-only patch uploaded here. Attached should expose the coverage.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5bf482f and pushed to 8.x. Thanks!

ParisLiakos’s picture

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