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:
- We used to have
__construct(A $a, B $b, C $c, ?D $d = NULL) - 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:
- 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?
- 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.
- 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.
Moving to the Group v1 guides
This seems to have spawned from 🌱 [META] Hooks via attributes on service methods (hux style) Active , do we want to close that one and credit the contributors here?
I'm hoping tests will go green now. Need to add a CR about memberships now validating role scope. The fact that the other two checks got moved to the same constraint doesn't need a CR because we were already throwing an exception from those checks.
Follow-up created here: 📌 Disallow the programmatic assignment of insider/outsider roles to members Active
Need to update the footer of this CR once this is fixed: https://www.drupal.org/node/3480111 →
kristiaanvandeneynde → created an issue.
All green, pushed another test but that went green locally, so committing in a few and writing a CR in the meantime.
Okay, thanks for clarifying.
Done, only thing missing is alt texts for the images.
On it
Is there anything else you'd like one of us (subsystem maintainers) to confirm? I can run some more tests this week if you want.
- Clicked on the MR posted by Björn and could edit it. Didn't click save, though.
- Tried to push to 11.x (that was scary!) and got the following: "remote: GitLab: You are not allowed to push code to protected branches on this project."
- Could also resolve and unresolve threads on the other link Björn posted
So can confirm @bbrala's findings :)
I attached a patch that used json_decode() rather than Json::decode(), fixed that.
I've just run into this issue tracking down why a post update was taking a long time and agree the saves should be removed outright.
That would be a behavioral change, which would rightfully lead to complaints from people who relied on this functionality. Having said that, I am more than willing to accommodate during this major by using the syncing flag, but as I've already mentioned in this issue I am hesitant because core has yet to define what the expectations and limitations of said flag are.
It might need to happen in a groups minor release with a change record or something.
Same as previous point, it could be considered a BC break. However, 4.x.x is about to be tagged and we can definitely fully remove it there if need be.
All in all, my personal preference would still be to keep the save, but perhaps with the syncing flag. An alternative would be to grab the target entity's cache tags and invalidate those. Now that the entity storage is using a proper cache internally, we could do that.
Either way, it would be really nice if we could get some movement/clarity on 📌 [PP-2] Better describe how SynchronizableInterface should be used for content entities Needs work
Sorry for not getting to this sooner, see 📌 Bump flexible_permissions module version to v2 Fixed
Caught a bug in one of the new storage methods while writing tests for them. Gonna move the one you wrote into a dedicated class for the constraint next week.
Reading through the MR I feel like I'm missing something. We're changing the default value from FALSE to NULL and then the subsequent check for said variable is also updated accordingly. What was the problem with it being FALSE rather than NULL?
kristiaanvandeneynde → created an issue.
Module is obsolete, no idea why I'd commit this :)
kristiaanvandeneynde → created an issue.
Please don't spam module issue queues.
https://www.drupal.org/user/3477971/track →
If you had spent 5 seconds looking at this module's project page, you would have noticed it has been part of Drupal core since v10.2
Cool, now to backport this to 2.3.x...