🇧🇪Belgium @kristiaanvandeneynde

Moving back to normal as I found out Search API will always convert whatever data that came from the index into arrays. Which raises the question whether we need to even bother storing the data as scalars.

The field we were struggling with was being added by a views data alter gone rogue.

I just did some more digging and this cannot be solved in only this module. I implemented IndexParamsEvent to make sure my index had actual scalars in it and verified that using the opensearch dashboard.

But Search API still uses arrays as ResultRow properties, because of SearchAPiQuery::addResults() relying on a bunch of Drupal\search_api\Item\ItemInterface, which in turn contain a bunch of \Drupal\search_api\Item\FieldInterface. The latter can only contain array values and this module's QueryResultParser::parseResult() correctly feeds it that.

So even if we store scalar data in the index, Search API will still pass it around as arrays once retrieved from the index. Which means the problem from my previous comment persists.

I just ran into this as my view was breaking.

What happens is that, if you configure a view for an index named Foo, to use the field "Link to InsertEntityTypeName" under the category "Index Foo", the view crashes.

This is because said field will eventually end up calling for the "langcode" property of the row in Drupal\views\Entity\Render\TranslationLanguageRenderer::getLangcode() with a $this->langcodeAlias as "langcode"

  /**
   * {@inheritdoc}
   */
  public function getLangcode(ResultRow $row) {
    return $row->{$this->langcodeAlias} ?? $this->languageManager->getDefaultLanguage()->getId();
  }

Because $row->langcode is an array containing a single langcode rather than a string, the parent call from \Drupal\views\Entity\Render\EntityTranslationRendererBase::getLangcodeByRelationship() crashes because it has a return value type of string.

  public function getLangcodeByRelationship(ResultRow $row, string $relationship): string {

Fields from the category "Foo datasource" do not seem to suffer from this as they use search_api's EntityTranslationRenderer instead. But that's from my limited testing.

Long story short:

1. Views expects strings in some places
2. This module returns arrays of strings
3. Weird crashes and painful debugging ensue

So I'd bump this to major given how it can completely break Views :)

Add a couple functions to the AccessPolicyInterface that requires the policy to return a list of the affected permissions

That might give false info as another policy could easily override that.

A description could be nice. So maybe start with an overview of currently active policies that show a name and description and then instruct the more technical people where to look if they want to see the code?

Either way, I'm a fan of an overview. I just don't want to overload access policies with methods for the sake of the overview; getLabel() and getDescription() could be nice, getAffectedPermissions() not so much.

You could use attributes, but those get discovered regardless of whether the policy is actually in use. E.g.: If module A has an access policy that module B swaps out, then there's no point in showing module A's version in the UI. With attributes, you would do just that, whereas with methods you would only load the active policies. (Swapping out or decorating access policies is an edge case and generally not recommended, but it's nice to keep the option open)

Moshe suggested you try putting this in https://www.drupal.org/project/security_review → . You could start by listing the class names in your UI, see how that looks and then circle back here to request two new methods for more descriptive info

Attached MR doesn't fix anything, it merely reverts to calling the parent so we have a patch to use while otherwise using the latest version of WSE. Waiting for maintainer response to decide a proper fix before doing any real work.

kristiaanvandeneynde → created an issue.

kristiaanvandeneynde → changed the visibility of the branch bad_di_fix_with_bc to hidden.

I was asking myself: "Why isn't this using setter injection?" when I noticed this issue and saw that @godotislate did exactly that. So huge +1 for MR 69 as it's both forward and backward compatible. We should not be juggling with version_compare for this (MR 68).

I'm unsure where the notion of of custom $ops comes from exactly, do we have any data that supports that this is a thing people did?

No, and people shouldn't have. I'm trying to predict possible fallout from the changes. Like how we sometimes go the extra mile even though our BC policy says we are free to just change constructor arguments on some services: It's being nice. But I doubt we need to do that here.

The idea is that we isolate the code in a way that allows us to provide an alternative implementation using the new mail API, first in an experimental module, then later moved back to user (at least that's my recommendation).

Yeah, I got that and I like the idea. My suggestion was more that, when copying the old code into the (perhaps temporary) service, we might want to see if the comments are still up to standards. But if we want to keep the scope narrow, I'd be fine with that too. As you indicated: This code might not live very long.

My main concern was that someone who used to use custom OPs would now have that option taken away from them. But giving it some more thought, nothing stops them from calling the mail service with said custom OP themself. So I left a few nits left and right, but nothing too major.

The new service copies the old code in ways that seem to make sense, but also do not go any further. One could argue now would be a good time to revisit some of the comments, but if you want to limit scope to "just move the code" then I could get onboard with that too.

Moshe seemed okay with the approach taken here too (via Slack) and so am I. So as far as subsystem maintainers are concerned, you are 2 for 2. Thanks for working on this.

I just realized we're not taking list cache tags into account here. This could become a security issue if released like this. So we need to create a follow-up that makes sure we tackle list cache tag invalidation too. The only downside is that core makes EntityBase::getListCacheTagsToInvalidate() protected. Would have been really nice to be able to just call that.

One thing I just realized by reading this IS, is that 🐛 Node update hook is triggered upon group content create RTBC did not add list cache tags to its solution.

So if you had a list of 5 nodes you had access to due to how they were grouped and you lose that access, we are fine because the list was tagged with both the individual nodes' cache tags and node_list. However, if you gain access because a node got added to one of your groups, the list won't show it because we no longer invalidate list cache tags.

Now, to the issue at hand, hook_entity_access() does not apply to lists, so the facepalm I just had should not get worse if we do as the IS suggests. And as you mentioned, adding or removing an entity from a group will either save the entity (v2/v3) or invalidate its tags (v4). So a main menu block should always get updated when one of the nodes that are actually in the menu have their group status changed.

So I think you're right and we can completely remove that piece of code.

Fixed for v3, on to v2.

Okay, time to backport this.

https://github.com/sebastianbergmann/phpunit/pull/5225

Essentially, the following is no longer supported (see second note):

   * @depends testRevisionsTab
   * @dataProvider revisionsOperationsProvider

Unless we stop using named arguments in our cases.

Okay turns out there's more to it than simply updating the version. Let's do that in this issue.

Thinking of lifting the new editor flow where we use contexts into a minor release. Already getting rid of the wizard in the issue about not allowing automatic memberships outside of the form approach so that's one step in the right direction. Then 4.1 could contain more context provider support.

Good catch, can definitely accommodate this.

kristiaanvandeneynde → created an issue.

Also, will fix composer in a standalone issue on Monday. Don't need tests to run just yet anyway.

Okay so I did some debugging and found that during an access plugin's lifespan, the view is far from initialized as it saves a lot of resources to not calculate or render a view you don't have access to. But we do need some of the view to be loaded to know which contextual filters were configured.

The latest commits do just that: If, and only if, the GroupPermission plugin is configured to also check the view argument will we load the handlers for the provided display ID and see if any of them use our GroupId ViewsArgument plugin.

This all relies on $this->view->element being populated, which is usually the case when access() is called. But I have a feeling if anything about this approach is fragile, it's that little part. So curious to see what VBO does with $this->view->element, for instance.

Either way, needs an update hook to fix our default views to actually use the group_id plugin ID instead of numeric.

Okay, so I gave this some more thought and I want to document some findings for posterity:

Q: If we're going to allow the GroupPermission access plugin to use the value from the argument (contextual filter), then why don't we put the new stuff where you choose a context provider inside the views argument? Then we can convert the access plugin to always use the argument instead.

A: Because the group won't always be in the contextual filters. Imagine a scenario where you can publicly view articles, but if you're a member of whichever group the article belongs to, you can also see whatever annotations the editors made in a Views block in the sidebar. Here, the contextual filter would be the node ID, but the GroupPermission plugin would have to be configured with the "view annotations" group permission on the group provided by a custom context provider ArticleParentProvider or whatever.

So for that reason I've chosen to keep the work I did and combine it with the work from #37, as previously explained in #61.

So, even though nothing changes, I'm writing this down in case other people have the same idea and wonder why I didn't offload the GroupPermission plugin's group negotiation to the views argument entirely.

@graber and I were talking on Slack and I think we can incorporate the patch from #37 into the current MR.

Let's say you have a view where you want one (or a few) context provide(s) to be checked, but on certain pages you want said view to be used inside a block with a hard-coded group ID as the view argument. It would make sense that the view argument were evaluated before the context providers then.

So with that in mind, we should add a checkbox on the config form allowing people to opt into this behavior and explaining that, when checked, a manually provided view argument will always take precedence over the context providers. I'll try and work on that tomorrow.

It also occurred to me that, once this lands, we should also open up a follow-up to allow the same context provider selection in the contextual filter modal. It makes very little sense to be able to choose which context provider to use for the access check, but not the actual views argument.

Okay, works for both entities and groups now. Also tested with all relationship fields hidden.

So this needs:

• Tests
• Option from group type to "use wizard" removed + update hook
• Option from relation type to "use wizard" removed + update hook

But so far it's looking very good and the code is a lot cleaner this way. No more temp store and wizard stuff.

Yeah that's what I also reckoned from the information I could find. I just felt I'd raise it in case my inexperience gave me false confidence and there was, in fact, an issue with the MR.

Left a comment. Mind you I am very new to fibers, but from what I understand we are manipulating fibers that we didn't start and that seems like it might cause a deadlock if the code that spawned said fiber isn't sturdy enough to deal with our suspension.

Going to leave at RTBC, but would be nice if my mind were put at ease :)

Ah yes, I was unaware of that feature. Thanks!

Okay the PoC now works for group forms.

Still need to check a bunch of stuff, but the gist of it is that whatever is configured in the relationship's form display will be shown inline in the group form. This gives some level of control over the whole thing. Probably should also hard-code language to whatever the parent form specified.

This does mean that any hook_form_FORM_ID_alter for the group relationship form will not apply here. That's an unfortunate drawback.

Thanks! For future reference, where can I find this output myself?

No wait, that seems to be a comment indicating what commands are being run. In that case I'm clueless as to why the pipeline is failing.

kristiaanvandeneynde → created an issue.

🎉

Big thanks to everyone involved!

As mentioned in the other issue, it needs to be a standalone thing. We cannot privilege Pathauto over other contrib modules so it's best to leave any contrib module related code out of Group and put it into targeted submodules instead.

Follow-up for Pathauto here: ✨ Consider a submodule to support pathauto Active

kristiaanvandeneynde → created an issue.

Okay CI is still running 11.1, let's not have that stop us as the 4.x branch will only support 11.2 and up anyway.

Closing as a duplicate of the other issue as that one is about to get merged.

CR looks nice, MR looks nice. Running tests once more now that Drupal 10.2 is officially out. If all is well, I will merge this in.

Assigning credit already.

Let me put it differently: If I were using a contrib module that extended a @final class and after a while my site broke out of the blue I'd be pissed. But if said module could not be installed without patching core (until the core issue re final is resolved), at least I'd know exactly what I'd be getting myself into.

1 if contrib needs something that is final they are cannot, contrib cannot require projects using them to patch core.

Then they would have a valid case to open a core issue asking to not mark said class as final and we get to my point about a discussion taking place: "Why does your module need to extend a class that we don't wish to offer support for?" If it's a good case, we can consider removing final or opening up our code for better alterability.

2 ghost wasn't blocked by drupal using final but symfony

What was the outcome of the issue he opened in the Symfony issue queue?

The other big issue with final is it adds more examples of open source preventing people from using open source code.

To counter with a bit of an exaggeration myself: I could mark half the classes in my module as final and people would still be able to use it just fine.

Why does glass have to be broken. We don't even put @internal in all internal classes.

But @final =/= @internal?

For internal code it could very well be that we allow internal extensions of it, but still do not wish to offer public API support for it. The difference is that we do not have a language construct to enforce that so we fall back to the second best thing. For final code we do have a construct, so that should be the default.

Again, if the decision falls to use @final I will follow it, but so far I haven't seen that many strong arguments for it. Both @final and final would have the same implications across the board, except one requires more deliberate action to get around. I find that virtual "I hereby acknowledge I'm about to fuck shit up" checkbox quite useful.

Unless anyone has any further objections, I'm thinking of committing this this week.

How does final help this situation? You can still miss the final unless you mean the suggestion includes code, but then you just can't even try! final prevents all extension. Are you talking about in contrib? Again, if marked final you can't even work around it. If the contrib maintainer would close an issue because it extended @final code they surely wouldn't accept code that would require patching out final. If I'm misunderstanding something please clarify.

If you miss @final, nothing will stop you from extending the class. If you miss final, PHP will stop you.

But my point wasn't necessarily about people making mistakes, it was about showing intent and requiring intent when people do want to break glass.

There's a few cases where we might want to use some concept of final. This issue, right now, isn't about when that is but what we can do when it is the case. Although the scope has changed a lot already. So with that scope in mind, there's only a couple of situations where we might encounter final:

1. In contrib
2. In core
3. In one of our dependencies

For both 1. and .2 my comment from #67 still stands: If you have a project where you want to extend a final class and fully well know that it's an edge case or project-specific requirement, by all means write a local patch that merely removes the final keyword and be done with it. Intent is clear, and you have made a manual, small effort to break glass and know you're on your own from here on out.

For 3. I would expect core/contrib not do anything funky there other than perhaps decorate the class, but according to #70 that expectation might be wrong?

Now, for the elephant in the room: @ghost of drupal past's effort to make entities use DI. Here you obviously wouldn't write a local patch, but instead go to the issue queue and argue your case that you are indeed blocked by the final keyword and think you could make a great contribution if it were removed.

At worst, it starts a discussion on why that piece was marked final, you get more info and nothing happens. At best, the final keyword is removed or the final class is refactored to allow for outside influence without extending said class.

Either way, I don't think there are that many efforts where a big change is blocked by the final keyword compared to the amount of times people are deliberately breaking glass, knowing what they're doing is risky business and accepting said risk. So I can see the amount of local patches far exceeding the amount of times we'd have to open an issue to discuss the removal of final somewhere.

So I'm erring on the side of final over @final again because in most cases it requires the person breaking glass to show intent, instantly making it clear to everyone on their project that something funky was done and support was forfeited.

One thing that's starting to bother me about the terminology of "break glass when in need" is that we're not actually making people brake any glass :)

Unlike the final keyword where you really can't extend it or get errors, the @final annotation only warns you in your IDE and some inspections but does not stop you from extending at all. So it's more like a "Do not trespass" sign on a bank vault that's wide open. It doesn't require much effort to break into said vault.

This leads me to reconsider the benefit of @final. One could argue that "breaking glass" would be writing a small patch that removes the final keyword from whatever core or contrib class you wish to extend. At that point it's both a deliberate action and plainly obvious from the patches folder that someone did, in fact, break glass.

I'm afraid that allowing people to extend @final classes at their own risk will cause confusion among people brought in to debug why their project is acting so weird. It won't be that obvious from the code base that a final class was extended.

Either way, just my 2c. I still stand by #35: If the majority is in favor of @final, I won't go lying on the train tracks so to speak. But now that I've given it some more thought I'm starting to lean towards the keyword again. We're simply not allowing people to "break glass" when in need, IMO we're just removing the glass altogether.

Oh yeah, we can provide whatever we want. That "whatever" should be part of this MR, though, so that we can consider the reported issue here fixed.

Either way, attached screenshot shows the progress I've made today. Seems to work rather well, but still need to write a CR and put the proper link in there. Also, tests would be nice.

Okay so the groundwork seems to be working. Great.

An even better approach would be to allow a list of context providers to be selected that will run in the specified order until a group is found. Then you can reuse "Group from URL" without having to copy its code into "Group from VBO". It would lead to less copy-pasted code all around.

If we switch to that, then I could write a small support module that contains the code from #16 and people could configure their permission plugin to use "Group from URL", followed by "Group from VBO" if they so choose.

I'll try that next. It would fully solve this issue, open up Group's view plugins to work with contexts and make any future request far easier to fulfill "from the outside".

I have plans for Group 4 to rely more on contexts. Group Sites already heavily does and it's been working great there. So I'd like to explore an option more in line with that to make everything work more nicely together in future versions.

The patch from #37 is guesswork. As soon as someone comes along with different needs, we're right back to square one on this discussion. I'd like to fix this in a way that I can tell anyone with slightly different requirements to "just write a context provider" that contains their requirements. Then we can close this once and for all.

Do you want your views to work with Group Sites, which detects the "active group" from domain, path, or whatever you want? Great they will work with that. Do you want them to work with VBO instead? Also possible...

I'm trying to see how this approach is inferior to or less flexible than the patch in #37. Could you please provide more detail? Am I missing something?

Okay so I've given this some more thought and I don't see why a context provider wouldn't work. By default we can use "Group from URL", which behaves 100% like the current code does. With all the same upsides and downsides.

But now you could write your own context provider that defaults to "Group from URL", but falls back to whatever you like. For example, you could first check the route and, in absence of a group entity, see if VBO is installed and try to get the group from the information in the temp store, like @larowlan suggested in #17.

Re #49:

context - based access is something that should be avoided as it creates ways to bypass restrictions

The access wouldn't be based on the context, it would still be based on whether you have the right permission in a Group entity. All that changes is that you have more power to tell Drupal which Group entity you want the permission to be checked against.

- an individual either has access or doesn't and not maybe has and maybe not, so site builders can accidentally leave some doors open and create security issues.

Not sure I can fully follow here. Isn't it always the case that site builders can misconfigure their site if not careful?

Also, it seems bad that group prevents access to routes without group context as usually in cases outside of some module scope neutral would be returned. Can we just return TRUE if no context is available?

Sadly no, because that would actually increase the risk of showing data that someone shouldn't see.

Now I'll admit that in most cases you could argue that, if the route does not contain a Group entity, the view will probably show nothing and then we could allow access. But as demonstrated in the VBO scenario, sometimes the route doesn't know which Group to deal with even though it should. In that case it seems far safer to block anything from happening further than allow perhaps unpredictable code to run. I would imagine that creating a bunch of group relationships without a group to point to can lead to crashes.

Either way, I'll pick this up today and see what I can come up with. I'm very much a fan of a solution which takes guessing completely out of Group and into the responsibility of those who have certainty about their environment. Be it the VBO maintainers, myself (with a VBO support submodule) or developers working on a specific client project with custom Group detection logic.

Yeah I'd need to look into this but when I added it way back when there was no core-provided alternative. We could compare the one from core to the one from Group and perhaps remove the one in Group. Would need an update hook to convert all blocks to start using the one in core.

Big RTBC+1.

To go into a little more detail: At first the processor was designed as a chain processor, which itself could be put into a chain. This design was copied from similar service collectors, but abandoned during the review process of getting Flexible Permissions into core as the Access Policy API.

The service alias must have gotten through the cracks of code review and it surprises me it took this long for someone to find it :) Once again, good catch. This should be easy to commit.

This is indeed an oversight when converting from Flexible Permissions. Fix is straightforward, but tests seem to be failing on Nightwatch, please rerun those. And this should indeed be backported to D10.

Thanks for finding this!

Added code that also invalidates cache tags when an entity is removed from a group and expanded the test case.

Backporting as we speak.

I could live with #37.

Seems to check all the boxes:

• Document as "Use at your own risk"
• We won't stop you from using it at your own risk
• You will get zero support when ignoring @final

Adding an attribute to the class that skips it for registration if the dependency is unmet seems like a good solution to the optional constructor argument problem. If we know that the dependency needs to exist for our hooks to be registered as a service, we can safely use the dependency's services with autowiring.

Be very careful when using the patches provided here. It bypasses Group's access promises when certain data could not be found. Use at your own risk.

Re @firewaller please don't reopen closed issues, the module is far from unusable without this patch and I encourage you to look into which module is checking group relationship create access without providing the right context.

I think this will be fixed as part of 🐛 Node update hook is triggered upon group content create RTBC . I.e.: We will no longer save entities when they are added to or removed from a group.

Test-only fails, full MR goes green. I call that a win :D

kristiaanvandeneynde → created an issue.

Seems like this MR also needs to tackle similar code in ::postDelete()

We also need to make sure that, when we're on a config form that got modified by our service, we do not call ConfigWrapperStorage->wrapEntity() during the form validation step as that actually saves a ConfigWrapper in the system for a config entity that may not get saved after all. It should be harmless if that happens, but better safe than sorry.

We should instead manually create one so that it has isNew() still set. It's only for validation purposes, so the wrapper should never get saved.

Okay I just realized we can kill the wizard with this approach + 📌 Create a new editor flow that does not involve wizards or custom pages Active

First of all we will need to remove the wizard, but not with the automatic selection of the group from the other issue. This can be done as follows:

1. Adjust GroupRelationshipCardinalityValidator so it works with new referenced entities
2. Create a service that attaches (part of) GroupRelationship form to another Group or entity form. This will only attach the form if there are any custom fields on it that are accessible.
   • If it can't attach a form, it will still attach a submit handler that creates a blank GroupRelationship (GR) entity with either the newly created Group or groupable entity added to it.
   • If it can attach a form, we attach a validation handler and submit handler along with the form:
     • Validation handler builds the original form's entity and assigns it to the GR's correct reference's "entity" property. It then calls validate() on that and flags violations on the GR subform.
     • Submit handler builds and saves the original form's entity, then assigns that to the GR it builds and saves the GR.
3. Double check that cardinality is properly reported even if fields for group and entity are hidden. Should be fine because we call $entity->validate() ourselves

- when we retrieve an item from the fast cache, we check if the node uuid matches the node uuid we're on, if it does, then we ignore $last_write_timestamp only for the current node.

Imagine this set-up (with write-through on set):

1. Node A writes item X to the persistent cache, we flag item X with the UUID for node A
2. Node A immediately writes item X to the fast cache, also flagged with the UUID for node A
3. We keep a last write timestamp per node

Now, item X should remain valid indefinitely unless its tags got invalidated, it's got a max age that has expired or it got manually flushed. The first two scenarios are no troublemakers as both the fast and persistent cache can deal with tags and max ages just fine. What has me worried is the manual clear.

If I trigger a manual clear on node B, that will clear the persistent cache used by all nodes, but only the fast cache of node B (unless otherwise set up). Now node B has to recalculate item X and stores it in the persistent cache as originating from node B's UUID. Cool.

But node A's fast cache hasn't been cleared and it still contains item X as originating from node A. In the meantime, node A's last write timestamp has not been updated. So now node A will happily keep serving a stale cache item because it has no clue that the underlying persistent item now originates from node B. With the current system, this cannot happen as we have one timestamp to rule them all.

This can be cured by making sure markAsOutdated() called from invalidate calls also updates the last write timestamp of all nodes. That would, however, beat the purpose of having a timestamp per node unless we differentiate between writes and invalidations when calling markAsOutdated().

- we also have to compare against the max($timestamps) of all the other nodes though, just in case they've written more recently than the item was written.

Maybe that was your way around the above, but then I wonder if we still gain anything. Checking if any other node recently wrote something seems to undo what we're trying to achieve here. It sounds a lot like one centralized timestamp, with extra steps.

I think the big question is whether the better hit rate here, would make it worse diluting the grace period approach in #3526080: Reduce write contention to the fast backend in ChainedFastBackend.

We would need to bring back the write-through on save and delete, or at least delete the APCu item if that's better for locking and space usage, we couldn't just ignore it as is done in that issue.

The more I think about this, the more I'm wondering whether we should just pick one road and go down that one. Trying to combine the grace period from the other issue, dropping write-throughs in the process, seems rather incompatible with this one.

And given my thoughts above I'm not entirely sure how much benefit we could gain here. But that might very well be because I'm not fully picturing what you had in mind yet with the timestamps per node.

Say you have a group relation plugin called group_media_foo to enable adding media entities to a group. This plugin can be installed on a group type and by doing so a group relationship type will be created for that unique group_type-plugin combo. Any media added to groups of that type will be using a group_relationship entity of the automatically created group relationship type (aka bundle).

After a while you install pathauto and you need it to work with grouped media entities. If supporting new functionality would require a change in plugin to , say, group_media_bar, then you would not be able to switch existing relationships for media over to that plugin as it would inherently mean you'd have to swap out their bundle.

That's why group relation plugins serving as an extension point is a bad idea. Changing the plugin definition to point to the extending class also doesn't work because then only one module can ever do that.

on other hand, all that the hooks implementation knows is that cache has been cleared without context about group related operation .

No, they would have to implement hook_group_relationship_insert(). Which tells you nothing about a cache being cleared and everything about an entity being created.

I would also strongly advise against adjusting or adding group relation plugins to achieve the desired result, as they are what power group relationship types. Those are bundles and so you'd have to be very careful with changing a plugin for another as then you'd have to update the group relationship entities' bundle, which is a total pain in the ass.

Re #78:

if we go back to this comment, may i suggest to add new method to Drupal\group\Plugin\Group\Relation\GroupRelationInterface which takes care of invoking exactly the kind of hooks required for each entity instead of calling $entity->save() ?

The issue with that is that we cannot know about people's implementations of those hooks. So one might very much want us to invoke the hook, whereas the other absolutely does not want us to. Then we're back at square one where we can't do things right for everyone.

Clearing caches and then expecting people to respond to the creation of a group relationship makes a lot more sense and we will no longer be doing any harm by guessing what people want.

Okay, after resolving the mix-up in my brain between ChainedFastBackend and BackendChain I actually really like this MR now. I still have some notes about spelling and docs in general, but the code looks good to me.

On a cold start we don't want several processes trying to write the same thing to APCu (or other software) thousands of times, so the 50ms (now 1s) locking mechanism prevents that. We still are able to retrieve the calculated thing from the DB cache, so it's not that bad. Over time, we will populate the fast backend properly and then we get the full benefit of it. But at least we won't be choking it when on cold caches.

The only thing I could still see is that both the 50ms and 1s window seem arbitrary. But something is definitely better than nothing here, so I suppose that's fine.

Other than the comments on the docs, I don't have any objections here. Seems like a great addition.

One thing I wondered about was whether we could have the fast backend store a random key to identify the local cache, and add that key to items in the persistent backend, and then only use last_write_timestamp when it doesn't match. This would help on single web head setups a lot, and maybe a bit with more web heads too. It would mean stuffing it into the persistent item transparently (like nesting the actual data in an array with the hash.

So each item in the persistent cache would be flagged with a key like so: ['some_key' => 'abcdef...', 'data' => $the_actual_cache_data] and upon retrieval we convert that back to $the_actual_cache_data?

But to what purpose? I'm not sure I can follow what you mean with "to identify the local cache" here.

Yeah for a second there I confused ChainedFastBackend with BackendChain where we combine a memory and database cache. That is often what we use for items that are requested multiple times per page. Okay let me revisit what I wrote and see which bits still make sense.

Also if we are to go ahead with the current approach, the class docs need updating. This bit won't hold true anymore:

 * (chained) cache backend. Those cache entries that were created before the
 * last write are discarded, but we use their cache IDs to then read them from
 * the consistent (slower) cache backend instead; at the same time we update
 * the fast cache backend so that the next read will hit the faster backend
 * again. Hence we can guarantee that the cache entries we return are all
 * up-to-date, and maximally exploit the faster cache backend.

It seems to me that we are limited by the $lastWriteTimestamp being shared regardless of CID. Meaning if we write 5 different items with a small delay between them, entries one through four will be considered invalid because of the fifth item being written. Why?

From the docs:

 * Because this backend will mark all the cache entries in a bin as out-dated
 * for each write to a bin, it is best suited to bins with fewer changes.

Isn't that bit worth reconsidering instead? If I have an expensive item I know will be requested multiple times, why would I accept it not being found in the cache because another item in the same bin so happened to got written just a few ms before my retrieval attempt? Seems like keeping a last write per CID could also go a long way here.

Gave this a review, generally like the idea and it seems promising in reducing a cache stampede's impact. However, I have some questions about whether we are profiling all the pieces involved in the process.

If I understand this correctly, we are completely eliminating fast backend lookups and writes for a whole second whenever anything is written to the persistent backend. Won't that simply move the stampede from the fast backend to the persistent backend?

We know that ChainedBackend is used for cache items that are expensive and/or requested frequently during a request. So turning off for a whole second the very thing why people use ChainedBackend seems a bit... odd?

If we are talking about higher and lower level caches being warmed, wouldn't it make more sense for the lower level caches to not use ChainedBackend if we're going to be storing calculations derived from the lower level data in a higher level cache? Talking about your example from #8.

Either way, not sure about this.

My idea was that if the submodule is small enough, I could put it in this project. I'll have a look at the link and code you provided and try to make a decision. Thanks for the info.

One thing that doesn't get solved here is fields on the relationship.

We need to figure out which fields we want to add:

• Do we add the entire relationship form (minus actions)?
• Do we scan the form for "extra" fields and only show those?

Also, do we call the relationship's form submit handler or use entity validation and manually save it?

kristiaanvandeneynde → created an issue.

That text was added to show that you now can upgrade from v2 to v3, but only if you first update v2 to 2.3.x. It does get confusing over time, I suppose.

The proposed resolution doesn't seem to be fully actionable, what would you suggest we change and where?

List is getting smaller :) Next one to tackle is probably the easy navigation entry and then "Disallow automatic membership outside of form approach". The brand-new creation UI I'm on the fence whether I want to add this now or at a later time. Still deciding...

CR added here: https://www.drupal.org/node/3526255 →

Looks good enough to me, gonna leave at NR for a few days and then commit. Will add CR now.

kristiaanvandeneynde → created an issue.

Okay, thanks for the clarification. It seems you have no objections going forward like this, so consider mine retracted.

That would fix the concern raised in #4, but it would still analyze all functions starting with the module name and "guess" that it could be a hook and add it to the container, no?

I'm asking because the initial issue title and summary were about having to put a manual stop sign in your module file and now it seems to have been repurposed entirely, not addressing the initial concern.

Okay, I've given this some thought and I'm going to move the Pathauto thingy to a hook implementation in a submodule (group_support_pathauto). I really don't like privileged code and if I were to put Pathauto stuff in GroupRelationship, then it won't be long before others start asking if they can be in there too.

That module can be part of a follow-up.

Well the issue still persists.

We are allowing people to call ModuleHandlerInterface::alter() with any number of unspecified variables. This in itself is a code smell, but for the sake of argument let's assume we want to go with the original proposal. We'd have to reduce the amount of extra parameters to 1, typehint it as an array and release a CR detailing this BC break.

That said, I'm not a fan at all of jamming all data we need to pass into a magic array either. A better way would be to find a way to be able to call specific (alter) hooks and know what these hooks' signature is. So perhaps in the future we could do something like this instead:

class MyModuleApi {

  /**
   * Full hook information goes here, like it would in *.api.php
   *
   * @param $arg1
   * @param $arg2
   * @param $arg3
   *
   * @return void
   */
  public static function myAmazingHook($arg1, &$arg2, $arg3): void {
    \Drupal::moduleHandler()->invokeAll('my_amazing_hook', func_get_args());
  }

}

So rather than call module handler's invoke(), invokeAll(), alter(), etc. we would make those functions only to be called by these MyModuleApi classes. People who'd want to invoke a specific hook would now call MyModuleApi::myAmazingHook(1, 2, 3) instead.

This would make it so the public API has all the right type-hints and reference handles, but the internal API, i.e.: ModuleHandler, can still accept any number of arguments and loop over all hook implementations, passing said arguments along. IIRC, func_get_args() should be able to keep references on those parameters that were passed by reference. We could probably pull this off with minimal BC break too, given how we can reuse most ModuleHandler methods as is, just troublemakers like alter() would need to have an updated signature.

If there is no support for this in a few months, we can still close this. I just really dislike that we can call any (alter) hook with no real signature. It's served its purpose but we should try to do better in this modern age of php. Also, if you think this goes too far off topic of the original IS, we could close this one and open a new one.

Follow-up can be a child issue of this one: 📌 Replace direct comparisons to user 1 in core. Active

LGTM.

But now we need a follow-up as I wanted to comment here that we should also remove MigrateAccessCheck then, but realized it has more implementations. So we need a follow-up where MigrateAccessCheck is removed, because it still runs UID == 1 checks.

+1 for deprecating passing NULL instead

Issue title is perhaps slightly off as I also noticed an issue with a comment in doRenderRoot(), updating

Working on it here: 🐛 Renderer::doRender() contains some outdated information Active

kristiaanvandeneynde → created an issue.

Posted a first stab at this to see which tests fail. Also not sure about the approach and comments re Pathauto. Feels like this needs to be done in a hook or event or something, even if Pathauto won't be the one implementing it.

Going to commit this in its current shape. Can still make changes before release if the meta in core evolves regarding best practices.

I noticed the docs in doRender() were not adjusted:

     // Set the bubbleable rendering metadata that has configurable defaults, if:
     // - this is the root call, to ensure that the final render array definitely
     //   has these configurable defaults, even when no subtree is render cached.
     // - this is a render cacheable subtree, to ensure that the cached data has
     //   the configurable defaults (which may affect the ID and invalidation).
-    if ($is_root_call || isset($elements['#cache']['keys'])) {
+    if (isset($elements['#cache']['keys'])) {

They still mention that doRender() can be the root call, and from the MR it seems that doRender() should no longer be concerned with any of that. Tackle this in a tiny follow-up?

Having it check for the "access site reports" permission would also be an acceptable solution to me. The essence is that the UID 1 check needs to go and preferably be replaced with a permission check.

Declaring all your hooks as extensions of Hook would definitely solve this as we can then support a "source" property on the base Hook class that we can scan for as described in the IS. The only drawback I see is we'd be creating tons of one-of attributes. They would be contained to namespaces that have Hook in them, so I suppose that could be fine.

So maybe add an optional "source" property to Hook that we can specify in the Hook constructor as a named argument and then extensions of Hook can prefill it for you? The default value could be empty or "drupal" to indicate that it's a core hook.

Anyways, big +1 to the idea. We shouldn't add classes to the container if we know they will never be called.