🇧🇪Belgium @kristiaanvandeneynde

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

Feeling confident about the MR now, just needs a CR and docs update.

Trying a different approach where the service provider in Group makes it so no-one will ever have to specify "shared: false" again. This is easier to test as I can now test for the container definitions in a kernel test. Still needs said tests, though.

And probably a change record and updated documentation.

How can I check whether the outsider/insider scoped group roles actually are assigned?

They aren't assigned, their permissions are by virtue of an access policy: SynchronizedGroupPermissionCalculator

Okay, going to see if I can get tests in, though. We don't want to accidentally reintroduce this bug in the future.

There, please review but I'm confident this fixes the issue. I'm surprised the tests didn't fail on this so will see if I can adjust them.

Okay this bug was introduced in 📌 Since symfony/dependency-injection 6.4: "Symfony\Component\DependencyInjection\ContainerAwareTrait" is deprecated Fixed . In that issue, we dropped the requirement for relation type handlers to set themselves as "shared: false" because we can set it for them. However, the actual decorating services still need to declare that manually.

Reverting the overzealous removals should fix your problem.

MR coming up.

Okay I was able to reproduce this on a clean install of Group 3.3.x, so this has nothing to do with the upgrade path. Tentatively adjusting title until I know more.

All green, well I'll be damned.

Added a MR to see what tests have to say about this. I'd be surprised if everything goes green, but who knows.

Only open thread is about cache hit ratio and I agree with @mxr576 that security trumps that. We can open a follow-up to further discuss those ramifications if need be.

All aboard the RTBC train, choo choo!

Okay so I saw @bbrala's comment about isNew() now, but I'd still say we should change the $node->id() boolean checks to a version of isNew(). Reason being that if isNew() ever gets updated to be more strict, we immediately benefit from that change. If we keep manual ID to boolean conversions, then we may end up lagging behind on core changes.

MR looks good to me now for the parts that I can review confidently (so anything but jsonapi tests). Don't let my isNew() comment hold you back, although I think it'd be a nice change.

Test fails seem to be random ones, so if you can ping @bbrala for a final approval then it's RTBC in my book.

See #48

I've gone and adjusted the title as you suggested as I agree this should/could be about more than Group. I opened it as such as I did not want to speak for other maintainers.

I also agree with almost everything you said and, if I'd have to choose, option 2 you suggested definitely seems like a nice compromise.

The only drawback is that it would still weigh a project solely on two factors:

• Whether it's considered strategic
• How many sites report using it

So I feel like either there needs to be a way to apply for the strategic status or there needs to be a second type of strategic label for this purpose. One label (10 credits) is managed by the DA, the other (5 credits?) can be requested on a per-project basis.

To circle back to my perspective: Sites built using Group are usually massive. The multi-million dollar type of massive. So it draws a lot of developer attention and money to the Drupal ecosystem that would otherwise simply not be there. Weighing that value solely on reported installs feels like it isn't fully giving Group the credit it deserves.

I know I'm speaking for my own benefit here, but if we were to go with option 3 or a mix of 2 and 3, Group would definitely be one of those projects that would request this "special status" in the blink of an eye. Having said that, I'm also aware of the fact that having strict metrics as opposed to a request process takes a lot of "politics" out of the process and will therefore be far more resistant to drama.

Either way, any progress would be welcomed. Thanks for your input @fjgarlin!

kristiaanvandeneynde → created an issue.

Oh wait, #3223580: Use MemoryCache (not MemoryBackend) in the installer → still exists. So back to [PP-1]?

Yup, to summarize the other issue: We can now use MemoryCache as long as we declare it as cache.bin.memory. Its factory is cache.backend.memory.memory.

Example:

  cache.access_policy_memory:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin.memory, default_backend: cache.backend.memory.memory }
    factory: ['@cache_factory', 'get']
    arguments: [access_policy_memory]

This means we should encourage anyone who wasn't using MemoryBackend because of how it works to switch to MemoryCache. We can then rename MemoryBackend to something that implies we only use it in testing to mimic a real cache.

Oh yeah, definitely a separate issue. But it could be a good idea to explore that avenue first and then return to this issue with its findings and/or commits.

Big +1 to #18. The hard part with splitting them up will be to figure out which attributes need to be aggregated into the entity type info and which are completely unrelated. It's not unthinkable people would add non-entity type attributes there for whatever reason.

I also plan to one day suggest we turn entity type handlers into services like I did in Group and having these separate attributes would go a long way to keep the declaration validation logic in one place. But that's for a more distant future as the current handler interfaces tend to be quite big.

Awesome, thanks!

LGTM

Pipelines seem to be fine aside from the test-only one, which makes sense.

smustgrave → credited kristiaanvandeneynde → .

Yeah we could have a deprecation test here and then remove it when the deprecation is removed.

Generally speaking, I would agree that we don't need a deprecation test here, but we have way too much public API surface in core and I've been bitten before where it turned out we should have kept in mind that some people might be extending these classes.

Having said that, MR looks good to me and so does the CR. So that's great :)

Few minor nitpicks but looks good to me otherwise.

@richarddavies thanks for those steps, will have a look!

You probably still have a supporting module or custom code checking for the old entity type IDs, but I need the full stack trace to confirm. Also these last few comments were completely unrelated to the original report so please add your comments to an issue dedicated to the 2->3 upgrade path.

Looks good to me so far.

Probably needs tests (need to check if we have tests for other member permissions) and I need to think whether we can release this in a minor version. I would err on the side of "yes" as they're new permissions and people with the admin member permissions should still be able to administer members.

We need to confirm the above, though, preferably with tests.

Thanks!

Updating IS: We've had two security issues reported already that boil down to this bug. It's time to rip the band-aid off.

Let's simply remove the incorrect return value and put out a CR and release note mention. If you're still using user.roles to determine access, which you shouldn't, then the only affected person by this change will be UID1 or people who share the same roles as UID1. And only if you had access logic riding on it. Blocks such as described in the IS will actually start working properly now.

Coming back to this after all these years, I no longer feel like the solution we have in Entity API is sufficient for core. I tried it out with Group and ended up having to copy a bunch of it because of Group's complicated structure.

So what I instead propose is a service collector that gathers the right metadata from either an entity query or a views query (like Entity API and Group do now) and then passes that along to the tagged services for alteration. In a second phase, we could introduce a core tagged service that copies some of the work from Entity API so that people can still define entity type query access handlers that return some of the value objects we saw earlier.

• Phase 1: Service collector, allow modules to alter queries more easily
• Phase 2: Port the entity type handler from Entity API as a tagged service
• Phase 3: Try to provide sane defaults for every entity type in said handler

makes me realize, isn't the access policy API a reason to NOT implement this?

Correct.

Furthermore, the whole point that we have the old UID1 logic in a separate access policy that can be turned off, is that this behavior is undesired in a secure system. The goal is to get rid of UID1 altogether and come up with a better alternative. Introducing an isAdmin() flag on users would be taking a step back in the wrong direction.

To me this reads as if you are defining the permission item as an admin permission explicitly. I would expect that normally you would leave this as FALSE, and only set it it if you yourself are declaring a user an admin based on your own unique factors similar to SuperUserAccessPolicy. Usually the isAdmin in this case would have been set by UserRolesAccessPolicy.

Again, correct.

If there are any further questions, Ill be glad to answer them. As it stands -now that we have the Access Policy API- and with the goal of removing the UID1 policy, I don't see any future for this issue. So for me, this can return to Closed (Won't fix), but I'll leave it open for a little while for the discussion to take place.

I think we should leave the foreach as is, but change this bit:

      if ($breadcrumb instanceof Breadcrumb) {
        $context['builder'] = $builder;
        $breadcrumb->addCacheableDependency($cacheable_metadata);
        break;
      }
      else {
        throw new \UnexpectedValueException('Invalid breadcrumb returned by ' . get_class($builder) . '::build().');
      }

To drop the addCacheableDependency() call and move that below the loop.

The reason it's failing now is because you're adding the cacheable metadata to a breadcrumb that gets replaced by build() further down the loop.

While I appreciate people chiming in to report it's broken for them too, I need steps to reproduce or I cannot investigate this. Leaving open for a while to give people the opportunity to provide these steps, but I'll be forced to close this issue if I can't reproduce it.

I'm going to err on the side of this being a local issue. Without steps to reproduce on a clean install there is nothing I can do here. Please feel free to reopen if you have steps to reproduce.

This error occurs when the group is deleted, but the group content still exists.

But that should never happen. See Group::preDelete().

Something is wrong with your website if deleting a group leaves orphaned relationships. It could be a bug in Group, but judging by the method I just mentioned above, it probably isn't.

I'm wondering if the cure is worse than the symptom here

The symptom is a potentially unstable or even insecure website, so the cure is definitely not worse :)

Normally I would agree, but as the IS states we're specifically looking to turning these providers into services so that we can benefit from everything the container has to offer us. If we were to keep them inside the permissions.yml file, we are severely limited as to what we can do.

One could even argue the permissions.yml file is another "magic file in a magic place" like the MyModuleServiceProvider class. In my opinion, the more callable code we can put into the container, the less custom code we have to write for these types of discoveries. The current controller approach means we need to inject the container to fetch said controller's dependencies, an approach that is not encouraged by Symfony (see removal of ContainerAwareTrait).

The way I see it:

• Permissions can only come from tagged services
• Core ships with one generic such tagged service that scours every module folder for a .permissions.yml file and gathers those
• Each module can then add as many such tagged services as they like, although usually one should suffice

Upside of this approach is that you can now turn off a service you don't want permissions from or decorate it to alter the result. With the current approach that's simply not possible.

My preferred solution is to only save the display once. It's being saved multiple times for each loop iteration, that's just unnecessary.

kristiaanvandeneynde → created an issue.

@joachim Are you alluding to the IS (which might need updating)? My last comment basically steers the MR towards tagged services, which are declared in the services.yml file. And that seems to be exactly what you want to see more of, or am I misreading this?

nicxvan → credited kristiaanvandeneynde → .

I just read @longwave's comment in #15 and I fully agree we don't need a locator here. A simple service collector (tagged services) would be better here. Repeating what he said:

• When we need the permission providers, we need all of them. So no point in having the option to lazy load them.
• When we want to build a permission UI or verify permissions on save, only then will we need the collector, so not using a locator has no drawbacks here

Furthermore I agree with the following:

I am also still weak -1 on having to specify the method, it feels like permissions services should just have a fixed interface

If a module for some reason needs multiple ways to provide permissions, it should simply provide multiple services; not multiple methods on a single service. Heavy +1 for a simple interface.

Should be all green now. Will commit and release after my vacation.

Looks good, updating title.

In php, you can pass as many arguments to a function even if it has only a few parameters. This is where some of this func_get_args() voodoo comes from in functions that accept any amount of arguments.

As to the $cache property, that's a tough one. Technically it's a BC break and at the very least we should have added a second trigger_error with a "there is no replacement" type of message for $cache like we did in doGenerate().

We could have kept assigning a cache backend to the $cache property, but then the question arises: "What's the point?" The actual class you were extending no longer makes use of it, so does it make sense for the extending class to still make calls to it?

Either way, this seems like an oversight and I apologize for the inconvenience. Personally speaking, I think this shows why we should have more classes in core marked as internal and/or final. One could argue the permission hash generator should not be considered API, as it's only consumed by one cache context and sent to the frontend for external caches.

I'd rather make it internal then and if people really want to change the outcome, encourage them to decorate it instead. This would retain our freedom to completely overhaul the internal hashing logic (like we did here), without risking people ending up in a situation like yours.

kristiaanvandeneynde → created an issue.

The changes followed the BC dance:

1. We used to have __construct(A $a, B $b, C $c, ?D $d = NULL)
2. Now we have __construct(A $a, B $b, C|E $e)

, where extra code runs in case someone is still providing ABC or ABCD

Obviously in D11, the BC layer has been removed, but in D10.3, you should be fine.

Okay so I've just read up on this issue and I think I understand the problem space and proposed solution.

While I'm onboard with the idea behind the proposed change, a few questions arise:

1. Can changing the way we store things be considered a BC break? I.e.: Could people, for some reason, be relying on the current behavior and have targeted workarounds in place either in code or at the system level? If so, would this change break that?
2. Do we have a way to properly test this? I currently only see unit tests for ChainedFastBackend using the PhpBackend and kernel tests using the MemoryBackend. For ApcuBackend we only have unit tests. The approach suggested here seems to be specific to APCu and I'm not sure we have sufficient testing to ensure everything remains working as is.
3. Can we performance test this? The IS mentions Umami and we do have performance tests for that in Umami, but are they tailored to what's suggested here? If not, what would a new test case look like, keeping in mind PerformanceTestBase is a FunctionalJavascriptTest.

Having said all of the above, I do not see how this relates to VariationCache whatsoever, so I am unassigning myself from the issue :) As a subsystem maintainer, I am keeping an eye on this however.

I've added a section to the release notes:

Disabling VariationCache (the module)

This release bumps the version of Flexible Permissions to 2.x.x, which no longer needs the VariationCache module as it now relies on the one from core.

If you are upgrading using composer, you should either composer require drupal/variationcache before or after downloading the new Group version (composer require drupal/group:^3.3 -W). After running database updates, you can then turn off the variationcache module and remove the dependency once and for all.

If you are still using manual downloads, just update, turn off variationcache and then feel free to delete said module from your filesystem.

I could never accept a patch that adds if ($group = $group_relationship->getGroup()) { to the code base. Group relationships coming from the DB must have a group and target entity. If they don't, your site is seriously broken.

VariationCache is now a dev dependency of Group so that people can upgrade and then get rid of it. It will be fully removed in v4. I'll add instructions to the release notes. Thanks for reporting this, everyone.

The reason Flexible Permissions isn't removed yet while VariationCache is, is that FP got a new major release, dropping VC. Whereas Group hasn't gotten a major version number bump, so we can't drop FP yet.

Tests for that submodule do go green, so unless there are steps to reproduce on a clean install, this cannot be fixed.

This is covered in the video series that was linked in the 2.0.0 release notes. You need to set up an insider and outsider role for admins now, because in some cases people only want one but not the other and for those cases it's an optimization to not have useless roles.

I've given combined scopes a thought int he past and couldn't resolve it nicely, but with 4.x.x we might get a second shot at this. Either way, marking this as a resolved support request as it's already been explained above why it's behaving like this.

The preSave logic has been moved to constraints that are checked in GroupMembershipTrait. This takes it out of GroupRelationship code, even though the constraints technically apply to the whole entity type, they have plugin ID checks inside them.

See: 📌 Disallow the programmatic assignment of insider/outsider roles to members Active

So all that's left here is to move the postSave code. This could probably be moved to the GroupMembershipTrait also.