Abstract the code from block visibility for user role check to be a condition plugin. I already have code for this and will clean it up and post it shortly with tests attached.

Eclipse

Parent Issue:

#1743686: Condition Plugin System

CommentFileSizeAuthor
#77 interdiff.txt4.51 KBtim.plunkett
#77 userrole-1921540-77.patch11.5 KBtim.plunkett
#75 interdiff.txt2.02 KBtim.plunkett
#75 userrole-1921540-75.patch11.08 KBtim.plunkett
#69 interdiff.txt985 bytestim.plunkett
#69 userrole-1921540-69.patch11.09 KBtim.plunkett
#65 interdiff.txt618 bytestim.plunkett
#65 userrole-1921540-65.patch11.73 KBtim.plunkett
#61 interdiff.txt5.55 KBtim.plunkett
#61 userrole-1921540-61.patch11.76 KBtim.plunkett
#58 user-role-2269401-58.patch11.52 KBtim.plunkett
#58 interdiff.txt1.04 KBtim.plunkett
#56 interdiff.txt1.99 KBtim.plunkett
#56 user-role-2269401-56.patch10.48 KBtim.plunkett
#51 user-role-1921540-51.patch8.63 KBtim.plunkett
#51 interdiff.txt6.86 KBtim.plunkett
#39 1921540-39.patch11.29 KBEclipseGc
#39 1921540-interdiff.txt2.42 KBEclipseGc
#32 1921540-32.patch11.55 KBEclipseGc
#32 1921540-interdiff.txt9.85 KBEclipseGc
#28 condition-user-1921540.28.interdiff.txt1.78 KBlarowlan
#28 condition-user-1921540.28.patch10.64 KBlarowlan
#25 1921540-25.patch10.37 KBEclipseGc
#25 1921540-interdiff.txt1.06 KBEclipseGc
#24 1921540-24.patch10.35 KBEclipseGc
#24 1921540-interdiff.txt570 bytesEclipseGc
#22 1921540-21.patch10.35 KBEclipseGc
#17 condition-user-1921540.17.interdiff.txt761 byteslarowlan
#17 condition-user-1921540.17.patch10.42 KBlarowlan
#13 condition-user-1921540.13.interdiff.txt1.18 KBlarowlan
#13 condition-user-1921540.13.patch10.37 KBlarowlan
#9 condition-user-1921540.9.interdiff.txt2.09 KBlarowlan
#9 condition-user-1921540.9.patch10.34 KBlarowlan
#7 condition-user-1921540.7.interdiff.txt1.05 KBlarowlan
#7 condition-user-1921540.7.patch10.25 KBlarowlan
#5 1921540-5.patch9.35 KBEclipseGc
#5 1921540-interdiff.txt1.34 KBEclipseGc
#4 1921540-interdiff.txt6.65 KBEclipseGc
#3 1921540-3.patch9.36 KBEclipseGc
#1 1921540.patch8.8 KBEclipseGc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

Status: Active » Needs review
FileSize
8.8 KB

Writing tests for this turned into a total pain in the butt because anonymous users are still just stdClass objects. The plugin can't even consider working with that because typed_data() (upon which it depends) can't deal with that. As such, I wrote a bit of code in the setup() method that actually make the anonymous user a User class object for the sake of this test. This will fail in the wild, so we need to get core actually using a User entity for anonymous users. I hear there are a number of issues out there that are attempting to do this already. I'll see if I can find them, or someone who knows what they are.

In addition to that wtf, I also found that EntityWrapper returns NULL on 0 $ids, so I had to add an additional check to that that passes 0. I've not gotten fago to weigh in on that specific change, but we'll see how testbot likes it I guess.

Eclipse

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityWrapper.phpundefined
@@ -67,7 +67,8 @@ public function __construct(array $definition, $name = NULL, ContextAwareInterfa
-    return $id ? entity_load($this->entityType, $id) : NULL;
+    // If $id is numeric, trust that it is correct.
+    return $id || is_numeric($id) ? entity_load($this->entityType, $id) : NULL;

I don't get this change, is it just allowing 0?

Also, assuming this stays, can it please become
if ($id || is_numeric($id)) { return entity_load($this->entityType, $id); }

and skip the ternary and the redundant return NULL?

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
@@ -0,0 +1,100 @@
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

These don't technically implement the interface, they override ConditionPluginBase

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
@@ -0,0 +1,100 @@
+  public function buildForm(array $form, array &$form_state) {
...
+    return parent::form($form, $form_state);

parent::buildForm(). Maybe an indication of missing test coverage? Idk if that's overkill to test

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
@@ -0,0 +1,100 @@
+  public function validateForm(array &$form, array &$form_state) {

This should call parent::validateForm() just for best practice, and in case something important is added upstream

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
@@ -0,0 +1,100 @@
+  public function submitForm(array &$form, array &$form_state) {
...
+    parent::submit($form, $form_state);

parent::submitForm()?

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
@@ -0,0 +1,100 @@
+    return (array_intersect(array_filter($this->configuration['roles']), array_keys($user->roles))) ? TRUE : FALSE;

I'd think return (bool) array_intersect(...); would read better.

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    parent::setUp();
+    $this->installSchema('system', 'sequences');
+    $this->installSchema('system', 'cache_path');
+    $this->installSchema('user', 'users');
+    $this->installSchema('user', 'role_permission');
+    $this->installSchema('user', 'users_data');
+    $this->installSchema('user', 'users_roles');
+    $this->installSchema('field', 'field_config');
+    $this->installSchema('field', 'field_config_instance');
+    $anonymous = new User((array) drupal_anonymous_user(), 'user');

Also, just to make this readable, a blank line above and below all the installSchema calls would be nice.

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    $this->installSchema('system', 'sequences');
+    $this->installSchema('system', 'cache_path');
+    $this->installSchema('user', 'users');
+    $this->installSchema('user', 'role_permission');
+    $this->installSchema('user', 'users_data');
+    $this->installSchema('user', 'users_roles');
+    $this->installSchema('field', 'field_config');
+    $this->installSchema('field', 'field_config_instance');

Have you tried removing these one by one now that you have a passing test? I know I've had to add some of these tables while writing a test, because they're needed by error handling.

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    $anonymous = new User((array) drupal_anonymous_user(), 'user');
+    $anonymous->save();
+    db_update('users')->fields(array('uid' => 0))->condition('uid', $anonymous->id())->execute();
+    $authenticated = new User(array('name' => $this->randomName(), 'bundle' => 'user', 'roles' => array(DRUPAL_AUTHENTICATED_RID => DRUPAL_AUTHENTICATED_RID)), 'user');
...
+    $anonymous = entity_load('user', 0);
+    $authenticated = user_load(2);

These could be assigned as protected properties, so you're not calling this later. Also, entity_load and user_load? Pick one! (Or neither, as I just said)

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    db_update('users')->fields(array('uid' => 0))->condition('uid', $anonymous->id())->execute();

It doesn't appear why this is needed, it needs a comment or an @todo or something

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    $authenticated = new User(array('name' => $this->randomName(), 'bundle' => 'user', 'roles' => array(DRUPAL_AUTHENTICATED_RID => DRUPAL_AUTHENTICATED_RID)), 'user');
+    $authenticated->save();

This one seems like it could still use entity_create()?

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    $manager = new ConditionManager();

Any reason not to do $manager = $this->container->get('plugin.manager.condition');?

+++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
@@ -0,0 +1,99 @@
+    $this->assertEqual('The user is not a member of anonymous or authenticated', $condition->summary());
+  }

Missing blank line before end of class

EclipseGc’s picture

FileSize
9.36 KB
+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityWrapper.phpundefined
@@ -67,7 +67,8 @@ public function __construct(array $definition, $name = NULL, ContextAwareInterfa
-    return $id ? entity_load($this->entityType, $id) : NULL;
+    // If $id is numeric, trust that it is correct.
+    return $id || is_numeric($id) ? entity_load($this->entityType, $id) : NULL;

I don't get this change, is it just allowing 0?

Also, assuming this stays, can it please become
if ($id || is_numeric($id)) { return entity_load($this->entityType, $id); }

and skip the ternary and the redundant return NULL?

Yes this is just allowing 0 ids to work. I don't really care what the code ends up looking like, but I figured I'd keep it as close to what is in core currently until fago has a chance to weigh in here. I just want to make sure this is the optimal way to proceed on this issue.

...parent::buildForm(). Maybe an indication of missing test coverage? Idk if that's overkill to test...

Yes it's missing coverage, and thanks for spotting my parent calls in all of these. Fixed in this patch.

I'd think return (bool) array_intersect(...); would read better.

OK

These could be assigned as protected properties, so you're not calling this later. Also, entity_load and user_load? Pick one! (Or neither, as I just said)

Yup, done.

It doesn't appear why this is needed, it needs a comment or an @todo or something

Non-0 id users get the authenticated role, 0 ids get the anonymous role. If you want to test anonymous user entities, you have to move the id to 0.

This one seems like it could still use entity_create()?

A.) Fago specifically hates people using entity_create() that way (I don't have an opinion).
B.) I had issues when I tried that. They may have been elsewhere in the code, so you may be correct, but I ultimately settled on this as being the easiest. I think drupal_anonymous_user() should probably be doing exactly this itself.

Any reason not to do $manager = $this->container->get('plugin.manager.condition');?

I always setup my classes and any dependencies they have manually in the test. It makes it very obvious everything that's happening.

Eclipse

EclipseGc’s picture

FileSize
6.65 KB

and the interdiff, sorry.

EclipseGc’s picture

FileSize
1.34 KB
9.35 KB

ok, another pass to take care of stuff I misunderstood from tim.

Status: Needs review » Needs work

The last submitted patch, 1921540-5.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
10.25 KB
1.05 KB

This should fix failing tests.

Status: Needs review » Needs work

The last submitted patch, condition-user-1921540.7.patch, failed testing.

larowlan’s picture

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

So drupal_anonymous_user() no returns a User object, not a StdClass.
*This* should fix the last fail.

larowlan’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
@@ -0,0 +1,101 @@
+      '#title' => t('Show block for specific roles'),

Should this reference blocks? I don't think so

podarok’s picture

Issue tags: +VDC

#10 agree
This one can be used not only for blocks visibility, but for views?

EclipseGc’s picture

Conditions are intended to be use case generic. So anyone wanting to reuse them should be able to do so. I'm working on getting some generic UI components into core.

Eclipse

larowlan’s picture

This patch changes the #title/#description to be more generic.

The last submitted patch, condition-user-1921540.13.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

#13: condition-user-1921540.13.patch queued for re-testing.

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

The last submitted patch, condition-user-1921540.13.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
10.42 KB
761 bytes

Fixes fails

larowlan’s picture

bump

EclipseGc’s picture

Issue tags: -VDC

#17: condition-user-1921540.17.patch queued for re-testing.

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

The last submitted patch, condition-user-1921540.17.patch, failed testing.

EclipseGc’s picture

Status: Needs work » Needs review
Issue tags: +conditions, +Blocks-Layouts, +scotch

Merged to head, this should apply.

Eclipse

EclipseGc’s picture

FileSize
10.35 KB

oops

Status: Needs review » Needs work

The last submitted patch, 1921540-21.patch, failed testing.

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
570 bytes
10.35 KB

Plugin annotation class got moved.

EclipseGc’s picture

FileSize
1.06 KB
10.37 KB

moved the buildForm() call a little bit to conform with my expectations.

jhedstrom’s picture

Status: Needs review » Needs work

Functionally, this patch is working as advertised. I have just a few code-level nitpicks:

return $id || is_numeric($id) ? entity_load($this->entityType, $id) : NULL;

This reliance on order of operations is dangerous and makes readability difficult. I'd add parentheses around $id || is_numeric($id).

+++ b/core/modules/comment/comment.moduleundefined
@@ -1608,6 +1608,15 @@ function comment_preprocess_block(&$variables) {
+  $anonymous_fields = array('name', 'homepage');
+  if ($account->uid == 0) {
+    // Make sure that correct anonymous fields are set if provided.
+    foreach ($anonymous_fields as $field) {
+      if (!empty($comment->{$field}->value)) {
+        $account->{$field} = $comment->{$field}->value;
+      }
+    }

This block of code could use more comment clarification on what's going on. It took me a while to figure out if it was even meant to be part of this patch, or had slipped in from a merge.

EclipseGc’s picture

Assigned: EclipseGc » larowlan

@jhedstrom

Thanks, I appreciate the review.

@larowlan

I totally agree with jhedstrom here, and I think you did the bit to get the comment stuff working. Can you just revisit/document that further? I had the exact same reaction except that I knew it was part of this patch because of anonymous posting.

Thanks!

Eclipse

larowlan’s picture

Status: Needs work » Needs review
FileSize
10.64 KB
1.78 KB
EclipseGc’s picture

I'm ++ to this, but wrote too much of it to rtbc.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #28 addresses my concerns, and the testbot is happy!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -VDC, -scotch
  1. +++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityWrapper.phpundefined
    @@ -77,7 +77,9 @@ public function getValue() {
    -    return $id ? entity_load($this->entityType, $id) : NULL;
    +    // If $id is numeric, trust that it is correct, this allows it to work for
    +    // anonymous users ($id = 0).
    +    return ($id || is_numeric($id)) ? entity_load($this->entityType, $id) : NULL;
       }
    

    This still doesn't seem to have been cleaned up for @tim.plunkett's feedback, and is still about as clear as mud to me. However, I'd suggest cleaning it up even further... why don't we stop babysitting entity_load() and just let it tell us whether or not there's a dang entity? That's its job, after all.

  2. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1608,6 +1608,18 @@ function comment_preprocess_block(&$variables) {
     function comment_prepare_author(Comment $comment) {
    

    I'm still hazy as to why this change is a part of the patch. Can we explain this on issue, please? Because the fact that we need to do this here makes it seem like something else somewhere isn't right.

  3. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1608,6 +1608,18 @@ function comment_preprocess_block(&$variables) {
    +  // For an anonymous commenter, name and homepage are stored in the comment,
    +  // to make them available for rendering in the author object we need to
    +  // transpose these values into the $account object.
    

    Comma splice. :) Let's make this two sentences.

  4. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1608,6 +1608,18 @@ function comment_preprocess_block(&$variables) {
    +  if ($account->uid == 0) {
    

    I'm nervous about hardcoding the anonymous UID like this, but then, that's all over core and has been forever, so definitely not in scope here.

    The loose typing check with 0 also makes me nervous.

  5. +++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
    @@ -0,0 +1,102 @@
    +    $form['roles'] = array(
    +      '#type' => 'checkboxes',
    

    I was going to comment on the scalability of this, but if a site has a zillion roles, it's already way more screwed by the permissions page. ;)

  6. +++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
    @@ -0,0 +1,102 @@
    +      '#description' => t('Evaluate to TRUE if the user has one of the selected role(s). If you select no roles, the condition will evaluate to TRUE for all users.'),
    

    We can lose the parens around the "s" in "role(s)" because we're already saying "one of". But actually, I think we can take the first sentence out of this description, since it's evident from the label.

  7. +++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
    @@ -0,0 +1,102 @@
    +        form_set_error('roles', t('You have chosen an invalid user role, please check your selection and try again.'));
    

    Comma splice. Let's make it two sentences. Or actually, the entire second part (after the comma) can go away, and the first part sounds rather accusatory. Let's work on this error message.

  8. +++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.phpundefined
    @@ -0,0 +1,102 @@
    +      return t('The user is @not a member of @roles or @last',
    +        array(
    +          '@roles' => $roles,
    +          '@last' => $last,
    +          '@not' => !empty($this->configuration['negate']) ? 'not' : ''
    ...
    +      array(
    +        '@roles' => $roles,
    +        '@not' => !empty($this->configuration['negate']) ? 'not' : ''
    +      )
    
    +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
    @@ -0,0 +1,131 @@
    +    // Summaries require an extra space due to negate handling in summary().
    +    $this->assertEqual('The user is  a member of authenticated', $condition->summary());
    

    What, seriously? Can't we just... not add the extra space?

    Well, actually, that's irrelevant. This is too clever for its own good. We're now passing the word "not" as a fixed value in all languages. This is a misuse of t(). Make two strings, for both of these, and then fix the goofy double-spaced assertion.

    Edit: The "@roles or @last" thing is also goofy. Either implode it with commas, or make a bulleted list.

  9. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
    @@ -0,0 +1,131 @@
    +  public static $modules = array('system', 'user', 'field');
    

    Missing a docblock.

  10. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
    @@ -0,0 +1,131 @@
    +  /**
    +   * Defines test information.
    +   */
    +  public static function getInfo() {
    ...
    +  /**
    +   * Sets up the tests.
    +   */
    +  protected function setUp() {
    

    There should be no docblocks here, per our standards, inconsistent though they be.

  11. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
    @@ -0,0 +1,131 @@
    +    // Setup an anonymous user for our tests.
    ...
    +    // Setup an authenticated user for our tests.
    

    Nitpick: "Set up" should be two words when used as a verb.

  12. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
    @@ -0,0 +1,131 @@
    +   * Tests conditions.
    

    This could do to be a little more specific. It's testing the user role condition, ne?

  13. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.phpundefined
    @@ -0,0 +1,131 @@
    +  public function testConditions() {
    

    Overall this test looks great. But maybe we should be checking custom roles as well?

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
9.85 KB
11.55 KB

ok, this addresses everything in #31 except #2 which is asking for on issue discussion I think.

Status: Needs review » Needs work

The last submitted patch, 1921540-32.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
+++ b/core/modules/comment/comment.moduleundefined
@@ -1602,12 +1602,27 @@ function comment_preprocess_block(&$variables) {
+  // For an anonymous commenter, name and homepage are stored in the comment.
+  // To make them available for rendering in the author object we need to
+  // transpose these values into the $account object.
+  $anonymous_fields = array('name', 'homepage');
+  if ($account->uid === 0) {
+    // Make sure that correct anonymous fields are set if provided.
+    foreach ($anonymous_fields as $field) {
+      if (!empty($comment->{$field}->value)) {
+        $account->{$field} = $comment->{$field}->value;
+      }
+    }

Oooh excellent. This actually makes things like anonymous Gravatar pictures possible, but we should load in more information like 'mail' and 'hostname'

+1 for having this change help ease contrib a little bit

fago’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.php
@@ -0,0 +1,96 @@
+ *       }
+ *     }

This misses configuration metadata. Core conditions should be good examples, so can we add that?

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.php
@@ -0,0 +1,96 @@
+      '#description' => t('If you select no roles, the condition will evaluate to TRUE for all users.'),

That's pretty un-interesting to me, more important would be to clarify how it behaves with multiple roles checked I think.

+ $anonymous_fields = array('name', 'homepage');

Ouch, we could really need a proper clean-up here, e.g. #1806514: Unify anonymous and registered users. However, I do not get on how this is related with this patch either?

shanethehat’s picture

@davereid - I'm not sure it will do the job for modules like gravatar (at least not in the current state).

function comment_prepare_author(Comment $comment) {
  // The account has been pre-loaded by CommentRenderController::buildContent().
  $account = $comment->uid->entity;

At this point, $account already contains a field for the user_picture that has passed through the field formatters.

EclipseGc’s picture

@fago

This misses configuration metadata. Core conditions should be good examples, so can we add that?

You mean in terms if identifying the configuration form elements with typed data? As I said during our work on conditions, I'm open to doing that since it helps Rules, but I'd prefer we do it on the node_type condition that's in core already and prove to ourselves that we have that working as desired before I try to do something "new" on a condition that's not in yet.

Ouch, we could really need a proper clean-up here, e.g. #1806514: Unify anonymous and registered users. However, I do not get on how this is related with this patch either?

Maybe larowlan can speak to that. It's been a while I don't recall which tests were failing without this, and he did the fix. Larowlan?

Eclipse

larowlan’s picture

Without that hunk several comment tests fail.
The crux of the issue is the author field on the comment is a 'entity' field type (in terms of the entity field api).
The code in that field type *did not* load an entity with id 0.
This patch changes that (the is_numeric() bit) and we now get an anonymous User object instead of nothing.
The existing code has

if (!$account) {
     $account = entity_create('user', array('uid' => 0, 'name' => $comment->name->value, 'homepage' => $comment->homepage->value));
   }

in other words in HEAD when the 'entity' field type for author on the Comment entity returns NULL for an anonymous user, we mock a user. Because this patch changes the behaviour of the entity field type to treat 0 as a valid id, we don't hit that hunk so need to intervene by adding those two properties to the passed object.
I hope that makes sense - if not - ping me on irc.

EclipseGc’s picture

FileSize
2.42 KB
11.29 KB

ok, the the stricter type checking is what's causing this to fail. I dug around in core for a bit, and all but one reference was some id == 0, so that's definitely the far more common. Whatever the case this is done through out core, so if it needs solving it's out of scope of this issue. I think this takes care of all feedback at this point.

Eclipse

fago’s picture

You mean in terms if identifying the configuration form elements with typed data?

Having metadata about required configuration, yes.

As I said during our work on conditions, I'm open to doing that since it helps Rules, but I'd prefer we do it on the node_type condition that's in core already and prove to ourselves that we have that working as desired before I try to do something "new" on a condition that's not in yet.

Well, but we've that already - it's just not implemented for NodeType as we missed the list class AFAIR.

E.g. from ExecutableInterface:

* - configuration: An array of configuration option definitions, keyed by
* option name. Each option definition is a typed data definition describing
* the configuration option. Check the typed data definition docs for details.

Howsoever, ok let's make sure that's there and tested in a separate issue.

fago’s picture

effulgentsia’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Condition/UserRole.php
@@ -0,0 +1,96 @@
+ *   context = {
+ *     "user" = {
+ *       "type" = "entity",
+ *       "constraints" = {
+ *         "EntityType" = "user"
+ *       }
+ *     }
+ *   }

How will this integrate with block caching? What I mean is, when this condition is used to determine block visibility, then we want to cache per role, not per user. But if the context is defined as a $user object, won't that require the SCOTCH controller to cache per user?

larowlan’s picture

Assigned: larowlan » Unassigned
pameeela’s picture

Is #1976758: Plugins miss metadata about configuration blocking this issue?

@EclipseGc are you able to respond to #42?

EclipseGc’s picture

@effulgentsia

How is that situation different than if someone were to use user role visibility on a block via the current approach? Likewise Conditions are for more then just blocks, we're just using the block visibility rules as our first set of condition plugins. I guess I'm just interested in how switching the logic portion of this to a plugin changes anything about the situation. We may indeed have a caching problem, but I think we'll have one regardless of whether we do this via plugins or not.

Right?

Eclipse

jibran’s picture

Issue tags: -conditions, -Blocks-Layouts

#39: 1921540-39.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1921540-39.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

adding parent issue link

kim.pepper’s picture

.

kim.pepper’s picture

Before I embark on an epic re-roll, I assume the parts of the patch that touch entity in #39 are no longer relevant?

tim.plunkett’s picture

Issue summary: View changes
Issue tags: -VDC +condition plugins

Inventing a new tag.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
8.63 KB

This reroll assumes #2268801: Update ConditionInterface to use recently added plugin interfaces has gone in, so it will fail for now.

Also typeddata doesn't allow anonymous users right now, will open up a separate issue for that.

tim.plunkett’s picture

Status: Needs review » Needs work

The last submitted patch, 51: user-role-1921540-51.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

51: user-role-1921540-51.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 51: user-role-1921540-51.patch, failed testing.

tim.plunkett’s picture

Component: other » plugin system
Status: Needs work » Needs review
FileSize
10.48 KB
1.99 KB

Based on the discussion in #2269401: The anonymous user cannot be validated because it has no name, I think this is the correct fix.

Status: Needs review » Needs work

The last submitted patch, 56: user-role-2269401-56.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
11.52 KB

Ah, need to adjust the test coverage to check the violations directly.

tim.plunkett’s picture

To clarify, this was the idea of @fago and @berdir from #2269401: The anonymous user cannot be validated because it has no name

kim.pepper’s picture

Status: Needs review » Needs work

Looking good. Some minor nitpicks.

  1. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.php
    @@ -0,0 +1,151 @@
    +class UserRoleConditionTest extends DrupalUnitTestBase {
    

    This is now deprecated.

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.php
    @@ -0,0 +1,151 @@
    +  public static function getInfo() {
    

    Comments.

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.php
    @@ -0,0 +1,151 @@
    +  protected function setUp() {
    

    Comments

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/Condition/UserRoleConditionTest.php
    @@ -0,0 +1,151 @@
    +    // Check a custom role
    

    Full stop.

tim.plunkett’s picture

hass’s picture

How can I reuse the condition in my modules? I'd like to replace _google_analytics_visibility_roles($account) and in Piwik the same. I need 'Add to the selected roles only' and 'Add to every role except the selected ones' conditions. I think this makes a lot of sense for core, too.

tim.plunkett’s picture

@hass please open a support request or ask a question on http://drupal.stackexchange.com, this is not the place.

Status: Needs review » Needs work

The last submitted patch, 61: userrole-1921540-61.patch, failed testing.

tim.plunkett’s picture

hass’s picture

Fago asked the same #1921544: Create a Current Path Condition. We cannot reuse the code. This is very bad and not OT and means CNW.

tim.plunkett’s picture

@hass Because that one did not use typeddata. You are OT.

fago’s picture

Had a short look at the patch - it generally looks great. One question though:

+++ b/core/modules/user/src/Plugin/Condition/UserRole.php
@@ -0,0 +1,109 @@
+      if (!in_array($role, array_keys($this->getRoleNames()))) {

Why is that validation necessary? This should be covered by form api.

tim.plunkett’s picture

FileSize
11.09 KB
985 bytes

HAH! Very good point, I never thought about that.

The last submitted patch, 69: userrole-1921540-69.patch, failed testing.

The last submitted patch, 69: userrole-1921540-69.patch, failed testing.

tim.plunkett’s picture

69: userrole-1921540-69.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs work » Needs review
fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/src/Plugin/Condition/UserRole.php
@@ -0,0 +1,87 @@
+      return $this->t('The user is not a member of @roles', array('@roles' => $roles));

This outputs just role ids? It should use the labels.

  1. +++ b/core/modules/user/src/Plugin/Condition/UserRole.php
    @@ -0,0 +1,87 @@
    +    return (bool) array_intersect(array_filter($this->configuration['roles']), $user->getRoles());
    

    There should be no array_filter necessasry here.

  2. +++ b/core/modules/user/src/Tests/Condition/UserRoleConditionTest.php
    @@ -0,0 +1,158 @@
    +      'name' => 'User Role Condition Plugin',
    +      'description' => 'Tests that the User Role Condition, provided by the user module, is working properly.',
    

    why is User Role Condition all starting upper case? It should either be the class name or regular lower case I guess?

  3. +++ b/core/modules/user/src/Tests/Condition/UserRoleConditionTest.php
    @@ -0,0 +1,158 @@
    +    $condition->setConfig('roles', array(DRUPAL_ANONYMOUS_RID => DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID => DRUPAL_AUTHENTICATED_RID));
    

    It's a bit weird to have associative arrays here. It should work with arbitrary keys. Having the form submit value with different keys is an implementation detail of the form anyway

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.08 KB
2.02 KB

#74.0 fixed
#74.1 fixed
#74.2 fixed

#74.3, this is how the NodeType condition works, and views, and just about every single configurable plugin (and form) in Drupal.

The expected structure for a selected "Authenticated user" is:

authenticated => authenticated
anonymous => 0

I'm going to keep that this way, and if you think it should change globally, we can do that. We *just* fixed NodeType to handle this properly in #2271939: NodeType condition does not evaluate properly.

Status: Needs review » Needs work

The last submitted patch, 75: userrole-1921540-75.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.5 KB
4.51 KB

Adjusting the assertions.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, changes look good.

I'm going to keep that this way, and if you think it should change globally, we can do that. We *just* fixed NodeType to handle this properly in #2271939: NodeType condition does not evaluate properly.

yeah, I think that's a weird coupling of configuration values and its form. As it's not an issue of this condition only, it should be handled in a separate issue and not hold on this one. This boils into the bigger problem of context vs configuration values and its metadata.

tim.plunkett’s picture

Priority: Normal » Major
Issue tags: +Page Manager

Because of the changes in ContextAwarePluginBase, this is a bit more important than your regular conversion.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5004fa8 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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