🇧🇪Belgium @kristiaanvandeneynde

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.

Re #23 exactly, but the emphasis should be on the last one. We can keep old messages as is without tripping up the code sniffers, but for the sake of maintainability should switch to the new format going forward.

If we ever run into a case where we feel like it's crucial to mention the minor and/or patch version for the deprecation, we can. But I can't think of any scenario where that would be crucial information. The CR we have to link to usually contains that extra piece of information anyway.

So I'd suggest we go with #23.3 and open a follow-up for D12 where we only allow that format from that point on.

I'm actually also in favor of #19. We may deprecate things in any minor, but will only ever remove them in a major. If we're going to drop the minor version for when we deprecated something it makes no sense to me that we'd still specify the full version string for the next major. We'd never remove a deprecation in 12.3.1 for instance.

Thanks for the steps to reproduce and confirmation. Normally I don't easily assign credit for these types of comments, but given the impact I think quick responses ought to be rewarded.

I've updated the guide here: https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →

At this point it would be nice if anyone could confirm that the changes here improve the upgrade path, then I can commit it and tag a new release.

Updated the guide to remove confusion and wrong info re relationship type IDs

Right, added tests to prove this works. Just need to check the update guide now to make sure we got all bases covered.

Updating IS to reflect the real problem, we may need to double-check the upgrade guide and specifically mention that old IDs are fine, even if a bit confusing.

Needs tests for new approach, but basically we now have:

1. A state key set during the updates that keeps track of whether we came from a legacy version (v1/v2)
2. A check for said key when trying to form a relationship type ID
3. If the check passes, we try to load the old ID
4. If the check fails or no old ID was found, we use the new ID pattern

You can already try the MR out if you wish. I'll only commit it once we have tests, though.

Okay, so we're always going to set a group_update_10300_detected_legacy_version state key now and leverage that in GroupRelationshipTypeStorage::getRelationshipTypeId(). This means older sites will be slightly slower as we cannot be sure there what the relationship type ID is.

Will try to take care of this as neatly as I can.

MR added, now I need to figure out why the IDs weren't adapted. Although I recall being against that because changing a bundle name has all sorts of extra implications. So I think the idea was to keep the old bundle names, but then the code needs to be able to deal with that.

Updated the fixture and can confirm, will see what I can do.

All right, makes total sense. Thanks!

Can't seem to rerun the pipeline for this MR (got push access).

To be honest I'm fine with keeping this a setting. I don't regard Settings::get() as that bad of a Drupalism, or at the very least don't think this issue should be held up by it.

If we want to move away from using Settings in favor of container parameters, then we need to first agree on that in a separate issue and come up with a plan to remove all use of Settings. But then you'd have to inject the container into every single service that might need to check for a setting. There's also a difference between what you can do in a settings.php file and a services.yml file.

As you can see, many things left to discuss on that front. So maybe let's be fine with adding "yet another" setting here and save that discussion for another issue?

Will rerun the pipeline as there seem to be random failures. Keeping at NW for a hook_requirements test, but that should be straightforward.

First and foremost, the elephant in the room: None of the IS is true unless you have a menu link that didn't change being saved frequently. So my first question to you would be: Why do you have menu links being saved without changes so often?

MenuForm::submitOverviewForm() seems to prevent a menu link from being saved in the tree storage if nothing has changed. So that seems like a double layer of protection there (one in the form, one in the storage) that tries to reduce work.

So what is causing a menu link without changes to be saved so frequently outside of the menu form? If we can't answer that, then I fear we might be optimizing something that may not need optimizing just because a website has some other code running that is actually causing havoc.

I'd err on the side of "Closed (won't fix)" if we can't get some better insight into this.

As to the MR:

I'm not sure we can simply change the return of MenuTreeStorage::doSave() like that. We didn't declare the service as internal and it's definitely not unthinkable that there are custom extensions out there that implemented their own ::doSave() to do some extra work in other (perhaps external) systems.

Also not sure about the code flow here:

    if ($original) {
      $affected_menus[$original['menu_name']] = $original['menu_name'];
      // ...
      if (array_diff_assoc($fields, $original) == [] && array_diff_assoc($original, $fields) == ['mlid' => $link['mlid']]) {
        $affected_menus[$original['menu_name']] = FALSE;
        return $affected_menus;
      }
    }

Why keep the original assignment?

I'd go into even further detail, but as I said at the start of my comment, I'm not even sure anything needs to be done here.

Okay so would it make sense to mark this one as fixed again as it was about deciding a style? And then open a follow-up about enforcing it? Or is there a chance we need to enforce it in e.g. gitlab_templates and we can't open up a follow-up yet before we know where to open it?

E.g. in drupal_flush_all_caches() we call `\Drupal::theme()->resetActiveTheme();`, which does get an instance of ThemeManager, only to set `$this->activeTheme = NULL;`.
But if at the time this is called, ThemeManager had not been instantiated yet, `$this->activeTheme` would already be NULL / uninitialized.

Now imagine if ThemeManager had a CacheBackendInterface dependency instead. And let's also assume that this would be a "default" MemoryCache which is used by many services, not unlike the "default" DatabaseBackend.

The point is that it then no longer matters if ThemeManager is instantiated or not because, in order to flush its caches, we would invalidate a certain cache tag. That would go over all cache bins, which are already instantiated, and then clear the data. At no point would ThemeManager be instantiated by our actions. Nor any other service for that matter.

There is overhead in reading and writing these cache items, which is always going to be more php operations than with a property cache.

True, but that's a tradeoff we should be willing to make as the benefits of this approach far outweigh the drawbacks. We'd get the above plus cache tag, context and max-age support. It would also mean that all of the classes using this cache would immediately benefit from any optimizations we can realize in the caching layer itself.

And as you said:

All of it is just basic php operations. So it will only become relevant for caches where we read or write many small values.

Fully agree. We can still opt not to use a MemoryCache on a case by case basis in those edge cases where it really does make a difference.

Perhaps we should discuss this further in dedicated issues as you suggested. My hunch is there's a cache context missing somewhere.

But also could we use RefinableCacheableDependencyInterface instead of CacheableMetadata that way I could pass the calculated permission directly when getting the group.

Please post the relevant code in the issue summary (for the new issue)

Thanks for the reports!

Hooks group_update_10300 through group_update_10302 run but don't do anything if you were already on v3. Hook group_update_10303 needs to run no matter what version you came from.

Was the "needs work" intentional or a mistake? Is it because you think we need another follow-up?

Small disclaimer that #41 and other comments mentioning MemoryCache are potentially no longer up-to-date as we now have full support for using MemoryCache. See this CR: https://www.drupal.org/node/3409455 →

With that said, I don't see why we can't use MemoryCache for what used to be cached in properties. The CacheTagsInvalidator service collects all cache bins and services that tag themselves as invalidators. We only need to focus on the first part here.

Any cache bin collected is instantiated because we use service_collector rather than service_id_collector for the CacheTagsInvalidator. So if we would create a dedicated bin for every class that used to have a "property cache", we would indeed have to instantiate a bunch of services.

However, I would argue we could have a "default" cache for memory like we do for the DB and encourage everyone to use that cache for their simple property caching. Then there's no overhead whatsoever because there will always be something using said memory cache and therefore the service will already be instantiated.

Re #45

We want to avoid that a service gets instantiated only to reset its internal cache property.

Would that not apply only to static properties being used as an internal cache? Most of these internal caches I've seen use regular properties, meaning the service is instantiated either way.

Having said all of that, the kernel.reset reset tag does look really cool. Thanks for bringing that to our attention.

• Hid all patches as we switched to MR
• Saved credit for the code contributions, leaving credit for reviews to the core committer
• Removed credit for comments that only uploaded broken patches
• On the fence about #29 as it's a tiny change but kept credit for now as it was to some extent impactful
• Will also update CR to be a bit more informative

All green now.

Saving credit for Tobias, might remove some credit for broken patches but asking internally first.

LGTM now. As I said before, I don't think we need \Drupal\Core\Routing\RedirectDestination::getAsArray() here because AFAIAC it doesn't make sense in this context.

I saw one seemingly random test failure, so running that one again. Can RTBC if that one goes green.

Thanks for updating the MR. Can't immediately see any changes outside of the resolved conflict, so fine with keeping RTBC.

Don't agree with #18.1, but other than that changes look good minus some tiny things. Left notes. Thanks for converting this @tstoeckler!

+1 to #4 if we must have documentation on the cases. I'm not really a fan of the proposed solution, though, unless it can be optional like constructor parameters or even as discussed in #3376518: Allow omitting method and parameter comments when variable name and type is enough → .

Going with the example on phpwatch, I think an enum like this is fine:

enum Suit {
    case Clubs;
    case Diamonds;
    case Hearts;
    case Spades;
}

If we're forced to document obvious enums like the above, then we're back to square one.

https://www.drupal.org/project/group/releases/3.3.2 →

Also re #12, it seems like some search_api code is running before Group has had a chance to fully convert group_content to group_relationship.

Dammit you're right I shouldn't have upped that number in 3.3.x.

Will hotfix release in the morning. This mess is coming from the fact that we have two versions being kept in sync but with different update hook numbers :/

Updated the release notes

Good catch, but I agree that this is not worth fixing. I'm sure if you pass it an empty array, the code will work fine. So it's really down to the question: Why are you passing a string where an array is expected? :)

Re #36: The reason we cannot always use a title to describe a single symptom is because one problem might lead to multiple symptoms and then it muddies the waters. This is what happened here. Instead, we point out the problem in the title so that the release notes contain the right information, but have the IS and/or comments describe some of the symptoms.

If you use the search feature, you'd still be able to find the most relevant issues then, even if the title might not immediately give away that it's the one you're looking for.

Will cut a release and recommend people affected by this bug run:

drush php-eval \Drupal::service('update.update_hook_registry')->setInstalledVersion('group', 9211);

kristiaanvandeneynde → created an issue. See original summary → .

As always for v2/3, it will be applied to both branches

xjm → credited kristiaanvandeneynde → .

Will commit and release in the morning

Change record here: https://www.drupal.org/node/3488636 →
Updated docs here: https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →

We should probably at one point create a dedicated documentation page for that rather than hide it away in an upgrade guide.

Added shared: false changes