Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
Comment | File | Size | Author |
---|---|---|---|
#77 | interdiff.txt | 4.51 KB | tim.plunkett |
#77 | userrole-1921540-77.patch | 11.5 KB | tim.plunkett |
#75 | interdiff.txt | 2.02 KB | tim.plunkett |
#75 | userrole-1921540-75.patch | 11.08 KB | tim.plunkett |
#69 | interdiff.txt | 985 bytes | tim.plunkett |
Comments
Comment #1
EclipseGc CreditAttribution: EclipseGc commentedWriting 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
Comment #2
tim.plunkettI 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?
These don't technically implement the interface, they override ConditionPluginBase
parent::buildForm(). Maybe an indication of missing test coverage? Idk if that's overkill to test
This should call parent::validateForm() just for best practice, and in case something important is added upstream
parent::submitForm()?
I'd think
return (bool) array_intersect(...);
would read better.Also, just to make this readable, a blank line above and below all the installSchema calls would be nice.
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.
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)
It doesn't appear why this is needed, it needs a comment or an @todo or something
This one seems like it could still use entity_create()?
Any reason not to do
$manager = $this->container->get('plugin.manager.condition');
?Missing blank line before end of class
Comment #3
EclipseGc CreditAttribution: EclipseGc commentedYes 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.
Yes it's missing coverage, and thanks for spotting my parent calls in all of these. Fixed in this patch.
OK
Yup, done.
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.
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.
I always setup my classes and any dependencies they have manually in the test. It makes it very obvious everything that's happening.
Eclipse
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedand the interdiff, sorry.
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedok, another pass to take care of stuff I misunderstood from tim.
Comment #7
larowlanThis should fix failing tests.
Comment #9
larowlanSo drupal_anonymous_user() no returns a User object, not a StdClass.
*This* should fix the last fail.
Comment #10
larowlanShould this reference blocks? I don't think so
Comment #11
podarok#10 agree
This one can be used not only for blocks visibility, but for views?
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedConditions 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
Comment #13
larowlanThis patch changes the #title/#description to be more generic.
Comment #15
larowlan#13: condition-user-1921540.13.patch queued for re-testing.
Comment #17
larowlanFixes fails
Comment #18
larowlanbump
Comment #19
EclipseGc CreditAttribution: EclipseGc commented#17: condition-user-1921540.17.patch queued for re-testing.
Comment #21
EclipseGc CreditAttribution: EclipseGc commentedMerged to head, this should apply.
Eclipse
Comment #22
EclipseGc CreditAttribution: EclipseGc commentedoops
Comment #24
EclipseGc CreditAttribution: EclipseGc commentedPlugin annotation class got moved.
Comment #25
EclipseGc CreditAttribution: EclipseGc commentedmoved the buildForm() call a little bit to conform with my expectations.
Comment #26
jhedstromFunctionally, 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)
.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.
Comment #27
EclipseGc CreditAttribution: EclipseGc commented@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
Comment #28
larowlanComment #29
EclipseGc CreditAttribution: EclipseGc commentedI'm ++ to this, but wrote too much of it to rtbc.
Comment #30
jhedstromPatch in #28 addresses my concerns, and the testbot is happy!
Comment #31
xjmThis 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.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.
Comma splice. :) Let's make this two sentences.
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.
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. ;)
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.
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.
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.
Missing a docblock.
There should be no docblocks here, per our standards, inconsistent though they be.
Nitpick: "Set up" should be two words when used as a verb.
This could do to be a little more specific. It's testing the user role condition, ne?
Overall this test looks great. But maybe we should be checking custom roles as well?
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedok, this addresses everything in #31 except #2 which is asking for on issue discussion I think.
Comment #34
Dave ReidOooh 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
Comment #35
fagoThis misses configuration metadata. Core conditions should be good examples, so can we add that?
That's pretty un-interesting to me, more important would be to clarify how it behaves with multiple roles checked I think.
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?
Comment #36
shanethehat CreditAttribution: shanethehat commented@davereid - I'm not sure it will do the job for modules like gravatar (at least not in the current state).
At this point, $account already contains a field for the user_picture that has passed through the field formatters.
Comment #37
EclipseGc CreditAttribution: EclipseGc commented@fago
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.
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
Comment #38
larowlanWithout 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
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.
Comment #39
EclipseGc CreditAttribution: EclipseGc commentedok, 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
Comment #40
fagoHaving metadata about required configuration, yes.
Well, but we've that already - it's just not implemented for NodeType as we missed the list class AFAIR.
E.g. from ExecutableInterface:
Howsoever, ok let's make sure that's there and tested in a separate issue.
Comment #41
fagook, opened #1976758: Plugins miss metadata about configuration.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedHow 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?
Comment #43
larowlanComment #44
pameeela CreditAttribution: pameeela commentedIs #1976758: Plugins miss metadata about configuration blocking this issue?
@EclipseGc are you able to respond to #42?
Comment #45
EclipseGc CreditAttribution: EclipseGc commented@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
Comment #46
jibran#39: 1921540-39.patch queued for re-testing.
Comment #47.0
(not verified) CreditAttribution: commentedadding parent issue link
Comment #48
kim.pepper.
Comment #49
kim.pepperBefore I embark on an epic re-roll, I assume the parts of the patch that touch entity in #39 are no longer relevant?
Comment #50
tim.plunkettInventing a new tag.
Comment #51
tim.plunkettThis 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.
Comment #52
tim.plunkett#2269401: The anonymous user cannot be validated because it has no name is the issue about validating anonymous user.
Comment #54
tim.plunkett51: user-role-1921540-51.patch queued for re-testing.
Comment #56
tim.plunkettBased on the discussion in #2269401: The anonymous user cannot be validated because it has no name, I think this is the correct fix.
Comment #58
tim.plunkettAh, need to adjust the test coverage to check the violations directly.
Comment #59
tim.plunkettTo clarify, this was the idea of @fago and @berdir from #2269401: The anonymous user cannot be validated because it has no name
Comment #60
kim.pepperLooking good. Some minor nitpicks.
This is now deprecated.
Comments.
Comments
Full stop.
Comment #61
tim.plunkettAlso switched out entity_load/entity_create, and updated the context name per #2272161: Entity-based conditions should use type => entity:$entitytype instead of constraints.
Comment #62
hass CreditAttribution: hass commentedHow 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.
Comment #63
tim.plunkett@hass please open a support request or ask a question on http://drupal.stackexchange.com, this is not the place.
Comment #65
tim.plunkettReroll after #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities
Comment #66
hass CreditAttribution: hass commentedFago asked the same #1921544: Create a Current Path Condition. We cannot reuse the code. This is very bad and not OT and means CNW.
Comment #67
tim.plunkett@hass Because that one did not use typeddata. You are OT.
Comment #68
fagoHad a short look at the patch - it generally looks great. One question though:
Why is that validation necessary? This should be covered by form api.
Comment #69
tim.plunkettHAH! Very good point, I never thought about that.
Comment #72
tim.plunkett69: userrole-1921540-69.patch queued for re-testing.
Comment #73
tim.plunkettComment #74
fagoThis outputs just role ids? It should use the labels.
There should be no array_filter necessasry here.
why is User Role Condition all starting upper case? It should either be the class name or regular lower case I guess?
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
Comment #75
tim.plunkett#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:
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.
Comment #77
tim.plunkettAdjusting the assertions.
Comment #78
fagoThanks, changes look good.
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.
Comment #79
tim.plunkettBecause of the changes in ContextAwarePluginBase, this is a bit more important than your regular conversion.
Comment #80
alexpottCommitted 5004fa8 and pushed to 8.x. Thanks!