🇧🇪Belgium @kristiaanvandeneynde

Okay, locally it works for me now.

I'm not sure why we need the following, but it was in the very first inception of this class:

    $entity->$field_name = $this->layoutParagraphsLayout->getParagraphsReferenceField();

Also added a form state rebuild at the end of submit just to be safe.

Think I have spotted 6 different modules so far where a user was added as maintainer without any second thought.

I think that warrants escalation. Those users should be removed as maintainers post haste and the original maintainers should be warned to properly vet applicants before handing them the keys. This is how supply-chain attacks happen.

Okay so the old MR was actually better in a sense that it emptied out the form and relied solely on the things coming from the form display. Will restore that behavior. I got it to work locally already, just need to work on the validation and submission handling.

Ohhh, I was going to release as 3.0.3, but opted to go with 3.1.0 last minute and forgot to move to a 3.1.x branch. I'll add those branches and see if that changes things. Thanks!

kristiaanvandeneynde → created an issue.

Oh my god 🤦

Thanks for putting up with my stupidity...

kristiaanvandeneynde → made their first commit to this issue’s fork.

kristiaanvandeneynde → created an issue.

In the latter example I would even consider the following:

/**
   * Calculates the permissions for an account within a given scope.
   *
   * A detailed explanation goes either here or at the top of the interface.
   *
   * @return \Drupal\Core\Session\RefinableCalculatedPermissionsInterface
   *   An object representing the permissions within the given scope.
   */
  public function calculatePermissions(AccountInterface $account, string $scope): RefinableCalculatedPermissionsInterface;

In case we add more docs at the top of RefinableCalculatedPermissionsInterface and AccessPolicyInterface:

/**
   * Calculates the permissions for an account within a given scope.
   */
  public function calculatePermissions(AccountInterface $account, string $scope): RefinableCalculatedPermissionsInterface;

Reasoning

With dedicated value objects such as the ones in the access policy API it could be better to document these once on the value object (or its interface) and then omit its documentation in @return tags.

The notion of a scope is used across a few methods, so perhaps that concept should be explained atop the interface (it's currently documented on d.o) and then omitted from all @param tags.

We could also decorate the PermissionChecker service to merge the scopes. That might actually be more logical.

Added a kickass idea for a new editorial workflow

kristiaanvandeneynde → created an issue. See original summary → .

but that would break two other cases that are allowed now:

As far as I can tell FourJSONCase is not allowed, it would be FourJsonCase.

And I don't think we should account for the word 'a', I have yet to see a class named ADog rather than Dog in all my years as a programmer. If you run into trouble with the sniffer on the very rare occasion that you might want to use 'a', then you should probably just add a sniffer ignore comment before that.

Replace with a workflow_list cache tag. That said, this alone won't help, since the implementation then again loads all workflow entities.

Why would adding a list cache tag load all entities?

In case you're worried there are too many workflows that are not related to content moderation, we could introduce a few custom list cache tags in workflows so you can have something like: workflow_list:type:content_moderation

Going to see if I can get the best of both the patch and MR into a new MR.

kristiaanvandeneynde → made their first commit to this issue’s fork.

Oh wow, that's great. We should consider making it the default then, no?

One final nit, but can be accepted upon commit.

I agree that we should create a follow-up for checking if we have sufficient documentation re cache.bin.memory and why entity storages don't use it like that.

Okay so the code will work, I really like the thorough test coverage and the fact that we uncovered MemoryCache itself didn't have a test extending GenericCacheBackendUnitTestBase yet. Only remark I have in general is that some methods are dedicated to numeric keys while other methods cover both string and numeric keys in one go. But don't let that detract from the overall excellent coverage.

I found a tiny nit regarding the word pidgin and think the test could benefit from renaming the assertion and an extra comment or two. This is why I'm setting back to NW.

The comment about moving the shared logic I'll leave up to you to decide, as MemoryBackend also seems to copy-paste things across methods occasionally. So I wouldn't be too bothered if we copy-paste a few lines of code here too. So feel free to RTBC without addressing this.

Now that this introduces another memory cache, would it be prudent to add a line to MemoryCache and LruMemoryCache's docblocks to tell people they should declare these bins using the cache.bin.memory tag?

I'll have a quick look myself to see if I can +1 this.

Please read my latest comment.

One thing I for instance think is a huge foot-gun is putting the specific permission methods on the interface. Right now it's getUpdatePermission, getDeletePermission, etc. but we cannot know all of the possible entity operations in a site. In Group I fixed this by having a method that takes an operation and returns a permission.

Either way, I don't think we should introduce yet another handler that no-one else can alter properly due to how handlers work in core right now.

To clarify: I don't think we need permanent coverage of the adding and getting of cache tags, as that would be rather pointless and hard to pull off as we can't decorate a trait. But for the sake of this issue, we could hack into the traits, add some coverage and see how many times we call array_unique in there.

Then create a second MR with both the coverage and the fix to see the difference and finally create an MR with just the fix.

MR generally looks good to me, but after reading the interface docs, I'm still not sure if people will fully know what to do when they need prewarmable dependencies in their prewarmable service. Maybe a code example could help here?

E.g. Service A is required by service B, both can be prewarmed. Due to the randomization, service B is requested first. You mention Fibers in the docs, perhaps showing people how to properly call for A from B using Fibers could be a good code example?

Added to the list, along with "Drop constructor documentation and use property promotion where possible"

Uh yeah, property promotion and the omission of constructor docblocks is also on my todo list

I'd be willing to give this another go, but with what I've learned in the past 6 years since I commented here I'd have to revisit if what was being discussed back then is still relevant now. I have since dropped my dependency on contrib Entity API and created new mechanisms in Group to deal with access.

Some of these are permission providers and access control handlers, but as services that can easily be decorated. Ultimately, I'd like to see if we can do something similar in core but that would be a huge endeavor. Either way, most of the discussion happened years ago and perhaps the first step should be to check how much of the discussion is still applicable today.

As previously pointed out by others, breadcrumbs were a big offender in core and contrib and the linked fix for the Book module shows exactly how to alleviate the issue.

On how to improve the message: I actually had the CID in there before but chose to remove that to keep the message shorter. I am not at all opposed to adding back the CID as I agree it can help debugging.

MR is RTBC for me, will update IS a bit. Keep in mind the goal is to turn the E_USER_WARNING into an exception in future major versions of Drupal. Once ✨ Add a Production/Development Toggle To Core Needs work lands, we can already convert the warning into an exception for dev sites.

Okay so let's start with the positives. I like how you:

1. Managed to profile this all the way down to the merging of cache tags
2. Considered that a render array without cache keys won't be cached to we can skip some processing there

The second point is something we should take into separate consideration to see if we can't optimize more for render arrays parts that won't be cached.

Having said that, there's some more light to be shed on the issue at hand. Let's get the obvious out of the way first: If you ever have a render array or CacheRedirect with thousands of cache contexts, you are doing something wrong as your whole page would not only become uncacheable, but actually slower.

Likewise, having thousands of cache tags is not going to be doing you any favors either and at that point you're probably better off thinking of other approaches. So it makes absolute sense for the current Cache class to call array_unique the way it does, because the expectation was never that it would be called thousands of times on large data sets during a single request.

Now, this leaves one use case we might want to address where you still run into the issue you have here: You are adding only a handful of cache tags, but you're adding them over and over and over again. To me, this is a red flag and something that probably doesn't happen all that often.

I'm inclined to say that we shouldn't fix this in core, but rather have GraphQL consider the fact that it's manipulating cacheable metadata thousands of times during a request and that it should perhaps optimize for that. This could be as simple as a custom CacheableMetadata object that incorporates the logic from the MR and at the very end of "adding fields", adds that objects metadata to the actual response once.

So the MR in its current shape doesn't quite cut it for me, personally. I'd rather see us revisit what we do with cacheable metadata during the render process, as demonstrated in the MR, but I'm not sure that would be such a performance gain. A great first step here would be to use the PerformanceTestTrait to gain some metrics on what is currently happening and then see how much it improves if we change the renderer.

Keep in mind that the renderer is a crucial part of Drupal, so we shouldn't be changing it lightly without enough evidence to back up the necessity of the change. We should also make sure we have ample tests covering us when we do make changes.

Finally, we could also consider changing the RefinableCacheableDependencyTrait and CacheableDependencyTrait in a way that it only calls array_unique on a get, but only until a new set has been detected. That would probably be a net gain across the board, but requires us to refactor how both traits interact with one another as they would need to share some information between them.

This avenue is worth exploring and I would definitely support that change, but not until we have above profiling in place first to see if it even makes sense to carry out.

The code we currently have in StandardPerformanceTest looks like this:

    $recorded_queries = $performance_data->getQueries();
    $this->assertSame($expected_queries, $recorded_queries);
    $this->assertSame(34, $performance_data->getQueryCount());
    $this->assertSame(124, $performance_data->getCacheGetCount());
    $this->assertSame(45, $performance_data->getCacheSetCount());
    $this->assertSame(0, $performance_data->getCacheDeleteCount());
    $this->assertSame(36, $performance_data->getCacheTagChecksumCount());
    $this->assertSame(43, $performance_data->getCacheTagIsValidCount());
    $this->assertSame(0, $performance_data->getCacheTagInvalidationCount());

We should probably add some tracking to the traits mentioned above and get some metrics on those too. It might not cover every cache tag manipulation as we're not guaranteed that everyone uses those traits, but most classes do.

I set this to working as designed because for this particular case it seems like it is. I saw mention on Slack of other issues related to this problem space and those might be actual bugs. I'd appreciate if someone could link them here so we can investigate if this patch helps any of those other issues.

To summarize what the code does now:

1. A login redirect is hi-jacked to add the check_logged_in to the query arguments of the destination URL
2. Any page with check_logged_in in its query args will throw the error from the issue title if no session was found

So as far as I am concerned, no URL should have the check_logged_in query argument other than the one you are redirected to after login. As soon as you start browsing, the query arg goes away. So for this purpose, it's doing exactly what it's supposed to be doing. The downside of this lightweight mechanism is that we cannot know whether someone was login redirected to a page or whether someone manually added the query arg or refreshed a stale browser tab.

So you could design a new system that is more complex and therefore likely more expensive or invasive, but for the issue reported here I would argue that it's absolute overkill. Again, if other issues are linked here where the error is shown when it shouldn't be shown, then we can discuss how we want to move forward.

Tempted to close this issue again but will leave open until the related issues are linked.

For what it's worth I always considered the R in RTBC as "Reviewed the code" whereas the T means "Tested the outcome / changes". Non-technical users can provide great feedback for the latter, but at least one technical person needs to check the former for an issue to truly be RTBC. It causes a lot of noise for committers if a bunch of issues are set to RTBC and none of them meet the code quality gates.

Catch found the original issue: 🐛 Login fails and no warning is issued if cookies are not enabled Fixed

Okay so Moshe doesn't recall any relevant discussions and yielded to me.

At this point I'm inclined to not add extra code to core for an edge case that is manually triggered in a way where other web apps would also show confusing messages the user might not understand.

Also, it only applies to the single time you are redirected right after login, as that's where check_logged_in is added to the query arguments. Any other tab you had open and is refreshed after logout won't show the error message because those tabs will most definitely not have check_logged_in in their query args.

So going to decline this MR as working as intended. There is no reasonable use case where this is an actual problem, so I don't see why we would add so much code for it.

If throughout the work on your MR you found that some of the existing code is sub-par, we can have that discussion in a separate issue. But after some brief digging it seems to be doing its job rather well.

Class names are case insensitive in PHP, so can't we simply change them in core?

Other UpperCamelCase checking can be problematic, sometimes developers want to use abbreviations in the name. For example "FirstJSONCase". I think Symfony prefers "FirstJsonCase", but we don't enforce that in Coder (yet?).

Are you sure about that? I recall trying to add methods like FirstJSONCase and getting scolded at by GitLab CI, telling me to turn it into FirstJsonCase. Not entirely sure myself, to be honest, just vaguely remembering this.

Yeah I mentioned that option in the review. In this case I would even consider disabling it so admins don't face varying forms depending on which user they're editing and end up getting confused.

Something along the lines of: As an administrator, you cannot block your own account. Either [cancel it] or ask another administrator to block it for you.

This predates my tenure as subsystem maintainer so will ping Moshe and see if he has anything to add. Will hold of reviewing any code until we get some clarity.

Removing tags that might make it appear on people's personal trackers.

Just reviewed as a subsystem maintainer, looks good to me. I also strongly agree with leaving out the description rather than adding something that states the obvious. Thanks everyone!

Please don't RTBC based on someone else's comment unless they explicitly state they have reviewed the code. For all we know @sagarmohite0031 only applied the patch without checking the code.

Left some feedback that warrants setting back to NW.

Awesome, thanks for confirming.

This did bring to my attention that core (and now Group) use the'revert' operation, but node still uses 'revert revision'. So we may need to open an issue to adjust group_support_revisions to account for that.

I'd say there's a good chance there are many websites out there that only use one provider. For them, this MR is sufficient.

Delaying it to add more info that then needs tests to see how well it behaves when using multiple clients alongside each other seems unnecessary. I'd rather we focus this issue on expanding what can go into a session and then another issue on investigating how well the session currently behaves with multiple clients and if we can or even need to store the client ID in said session.

The issue is about "Save all tokens", so let's keep it like that? A Client ID is not a token, even though I understand why you brought it up.

Is this why this job is failing? https://git.drupalcode.org/project/group/-/jobs/3758589

larowlan → credited kristiaanvandeneynde → .

So only RevisionUiAccessTest::testUpdateDeleteAccess() fails for non-default revisions when reverting. Will have to look into that.

Okay we can drop everything, but our revision UI tests now have 2 failures when we do. Need to investigate that.

Checking ✨ Implement a generic revision UI Fixed for revision counterparts in core.

kristiaanvandeneynde → created an issue.

Added a MR that calls the module handler. This should make it work on both pre and post 11.1 Drupal.

kristiaanvandeneynde → made their first commit to this issue’s fork.

Not sure I can follow. We're using the MR from this issue on a project and it's working fine in its current state. We simply always use the same client throughout our application. Also it seems like your MR is tagged against a different issue so perhaps we should keep this issue and its MR small in size? It's far easier to review and commit smaller MRs than big ones with multiple goals.

Will set to NR but feel free to RTBC again if you agree.

Opened up 🐛 ResourceTestBase::getExpectedUnauthorizedAccessMessage() should be removed or replaced with something smarter Active .

I really don't have the energy right now to fix these test failures, maybe someone else can take over. Getting blocked by jsonapi tests is becoming a recurring theme and it's really demoralizing.

kristiaanvandeneynde → created an issue.

Tests are failing on jsonapi (again...) because its ResourceTestBase wrongfully assumes that an access denied will always have the same reason:

  /**
   * Return the expected error message.
   *
   * @param string $method
   *   The HTTP method (GET, POST, PATCH, DELETE).
   *
   * @return string
   *   The error string.
   */
  protected function getExpectedUnauthorizedAccessMessage($method) {
    $permission = $this->entity->getEntityType()->getAdminPermission();
    if ($permission !== FALSE) {
      return "The '{$permission}' permission is required.";
    }

    return NULL;
  }

With our changes we can now return an access denied for entity view displays without having to run the manual permission check. But jsonapi tests are too rigid for that. Not sure how to fix this as it probably requires a significant rewrite of jsonapi tests.

Another bit to rant about, but maybe this should become an issue of its own. From FieldUpdateActionBase:

  /**
   * {@inheritdoc}
   */
  public function access($object, ?AccountInterface $account = NULL, $return_as_object = FALSE) {
    /** @var \Drupal\Core\Access\AccessResultInterface $result */
    $result = $object->access('update', $account, TRUE);

    foreach ($this->getFieldsToUpdate() as $field => $value) {
      $result->andIf($object->{$field}->access('edit', $account, TRUE));
    }

    return $return_as_object ? $result : $result->isAllowed();
  }

If the very first check returns Forbidden, this will still run ALL the other checks even if it can never affect the outcome. Better code would not run the loop if the initial result is F and break the loop it the compound result became F during the loop. Core and contrib are littered with these types of poorly performing access results and I feel it's partly because we gave them the andIf() and orIf() methods, making it so developers don't seem to know the impact of what's going on.

It's like the entity query accessCheck() method story where we ended up forcing it being set to make developers aware of something. We should consider doing something similar here.

Thinking about this further, I am really not a fan of the concept of orIf and andIf and any possible variation thereof in general. It will always lead to too much code being ran if implemented incorrectly. In #25.2 I demonstrated how some code needs to be refactored, but that code would be far more performant if written this way:

    $filters = $editor->getFilterFormat()->filters();
    if ($filters->has('media_embed') && $filters->get('media_embed')->status) {
      return $media->access('view', $this->currentUser, TRUE)->addCacheableDependency($editor->getFilterFormat());
    }
    return AccessResult::neutral()->addCacheableDependency($editor->getFilterFormat());

So the old code would run both checks and always add the dependency. The new code in the MR always runs the entity access check (the most expensive part) but only conditionally adds the dependency.

The above code, however, always adds the cheap dependency, but only runs the expensive entity access check if need be. This is still the best possible way of doing this, but not achievable with any of the four methods introduced in this issue due to the fact that an entity access check can return any of A, N or F.

Okay so I added two types of examples and discovered a few things while doing so.

First of all, by not adding the new methods to the AccessResultInterface, my IDE complained that calling them is potentially polymorphic as the test class UncacheableTestAccessResult does not have the new methods. So either we add them using the 1:1 rule or we leave it as is but face issues when people have their own versions of AccessResult.

As to the examples:

1. The simple scenario is looking for orIf(AccessResult::allowedIfHasPermission()) like I did in EntityFormDisplayAccessControlHandler, EntityViewDisplayAccessControlHandler and BaseFieldOverrideAccessControlHandler. These can be directly replaced by the new code.
2. The advanced case is as I did in CKEditor5MediaController. Here, the old code would run both checks either way, but the entity access check could return any of A, N or F. So our new methods can't be called here because we don't know which one to call. By hoisting the uncertain one to the top and then adding the certain one conditionally, we were still able to achieve a performance gain because now we only add the dependency if it makes a difference.

Point .2 led to another interesting bit. We could actually still benefit from an orIfCallable() and andIfCallable() method for cases where two unknowns (A, N or F) are combined because in both andIf() and orIf()'s case, nothing will happen if the original one is F.

So a combination of 2 entity access checks can still be optimized when the first check returns F.

We already had a CR for the membership deprecation so no need to create a new one.

From Group:

  /**
   * {@inheritdoc}
   */
  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    // If a new group is created and the group type is configured to grant group
    // creators a membership by default, add the creator as a member unless it
    // is being created using the wizard.
    // @todo Deprecate in 8.x-2.x in favor of a form-only approach. API-created
    //   groups should not get this functionality because it may create
    //   incomplete group memberships.
    $group_type = $this->getGroupType();
    if ($update === FALSE && $group_type->creatorGetsMembership() && !$group_type->creatorMustCompleteMembership()) {
      $values = ['group_roles' => $group_type->getCreatorRoleIds()];
      $this->addMember($this->getOwner(), $values);
    }
  }

kristiaanvandeneynde → created an issue.

Forgot to mark one access policy as final, will do that and then merge right away.

Fully green now. Running one last manual check before committing.

Okay, please keep me posted.

Removed the test in another issue, rerunning tests.

Okay all green, pushing one phpstan baseline change and then committing.

Uh yeah, there's an issue for that one already I just had to find it :) Thanks

kristiaanvandeneynde → created an issue.

was called 18 times on a page refresh of /node/add/blog before the update and now after the update it is called 7058 times

Holy crap

Any idea what could be causing such an extreme difference?
Could this be a caching issue or issue with the permission calculator?

Not unless this can be reproduced on a clean install. I have no clue what custom code and/or patches you have running, so either please run xhprof, Blackfire or whatever performance testing tool you're confident using or see if you can reproduce this on a clean install.

Erring on the side of Major right now as if this is the case for all Group installs, something is really broken.

So as predicted the upgrade path test goes red. Will fix that first in another issue and then circle back here.

Will probably fail on v2-v3 update test which needs to be removed

kristiaanvandeneynde → created an issue.

kristiaanvandeneynde → created an issue.

kristiaanvandeneynde → created an issue.

Gave the IS some thought and at first felt like this was wrong:

See that fastBackend is happily setting invalid data, which will never be useful

After all, if you're getting the data while note caring about its validity, you may as well store that in memory for future gets which do not care about validity. However, then I realized that a cache get which allows invalid could the poison the memory cache for a subsequent cache get which does not allow invalid. And that would be really bad.

I still have the eerie feeling that there are edge cases we're not seeing here, but for now I'd say it makes more sense to commit this change rather than keep the old behavior.

So RTBC +1.

This is missing support for $element['#cache']['keys'], some comments seem to have snuck through the sniffers. I'm not sure how much value the test adds, to be honest. We're simply testing for the value of a constant, I don't see the point in that.

I'm on the fence regarding removing the test coverage, because it's really not testing anything other than "is constant X value Y". So if you ask me I'd remove them. But let's leave that for core committers to decide.

Okay no more discussion, so closing as won't fix :)

May sound like a dumb question, but did you drush cr after upgrading your files with composer?

It will be released as a security release, so I'd hope people read that even more than they would a CR.

Remove dependency on flexible_permissions 2.0.1 and use the core Acess Policy API.

That would require a new major version, which is exactly why I'll only be able to do this in Group v4, which will start development soon.

Result is a an error message relating to flexible_permissions

Which error?

However the site becomes inaccessible with the message 'The website encountered an unexpected error. Try again later.'

What does the dblog show?

There was an issue with group content types that had long names. Have you tried the latest release? 3.3.3

Also #7 seems completely unrelated to this issue.

Left a comment why we don't need the instanceof check. So back to RTBC

Removed the test. Should go green again and in my opinion is ready to ship given my explanation in #30.

Okay so the reason the test still goes green shows how poorly we were testing things.

Pre-patch:

• $user_1 cache context value = is-super-user
• $admin_user cache context value = authenticated,RANDOM_ADMIN_ROLE_NAME

Post-patch:

• $user_1 cache context value = authenticated
• $admin_user cache context value = authenticated,RANDOM_ADMIN_ROLE_NAME

So obviously that assert still goes green both before and after the patch.

So RenderCacheTest::testUser1PermissionContext() only goes green because we deliberately set $usesSuperUserAccessPolicy to TRUE and added a todo there to remove said flag. This warrants a test rewrite or removal.

Then RenderCacheTest::testUser1RolesContext() goes green both with and without the change, this is explained above.

The funny thing is that the second test case automatically also tested for user.permissions because those cache contexts are always added. This leads to the test still going green even if we remove the quirk from the UserRolesCacheContext. Adding the following code to the test makes it go green before the removal, but red after the removal.

  public function setContainer(ContainerBuilder $container): void {
    $renderer_config = $container->getParameter('renderer.config');
    $renderer_config['required_cache_contexts'] = [];
    $container->setParameter('renderer.config', $renderer_config);
    $this->container = $container;
  }

So all in all, this test was all sorts of broken and not even testing the thing it promises it was testing. Its removal is long overdue.

Okay so that test specifically tests the quirks that used to be (user.permissions) or still are (user.roles) in Drupal core's cache contexts. Given how we already removed one quirk by introducing the access policy API and we're about to remove the last quirk in this issue, that test has no reason to exist any more.

The only thing that puzzles me is why the test still goes green after we remove the final quirk. I would expect it to go red for the user.roles cache context at least. I'll investigate this and report back, then we can remove the test and set the issue back to NR.