🇳🇱Netherlands @kingdutch

kingdutch → created an issue.

I am okay with relaxing the standard. I am actively opposed to enforcing lowercase using Coder.

The fix required to switch over would touch nearly every file in a project. That means that you basically reset git history and git blame for thousands/millions of lines of code with no real benefit. That can be very disruptive for day to day operations of a project.

klausi → credited kingdutch → .

I think an important business rule that's not part of the above scenario but definitely should be considered to design/test the system is a rule that we originally discussed:

1. Rule 1 should only apply to the global scope. Published entities in the scope "Group" should only be seen if the visibility of that group is "public". If the group visibility is not public then the user may only see published content in groups they're a member of

Are you saying the Access Policy API could allow you to bypass that bit at the end by revoking the user's view own unpublished content permission?

Yes absolutely. You would implement AccessPolicyInterface::alterPermissions.

What if, we had this hypothetical method / implementation for a query?

I think the "use permissions to influence the query" is a solved problem? That's essentially implemented by existing query alters that should already be performing permission checks together with the query that's being altered. So implementing the right Access Policy API would already achieve that.

The problem area is (in my understanding) much more in scenarios where complicated business rules need to be combined together.

In your example there's already 2 business rules being combined which are:

1. ->condition('status', 1) - User can view any published entity
2. ->condition('status', 0)->condition('uid', $account->id()) - User can view unpublished entities they own

Now lets add a new condition that lives in a different system and requires data from different tables:

1. User can view unpublished entities in any group they're a member of a group and on that membership have a role that has the "view unpublished content" permission

This business rule would be based on the configuration of permissions for a type of group. There's not a specific permission for the user we can check, because it not only depends on the scope, but on the identifier of group. There may be 100s or 1000s of groups so we have to encode this logic in the query itself and join into the membership table.

This would already be impossible to write as a simple entity condition in the current entity API because there's no path from the content entity (Node in the example, but could be any entity) to the membership table. That's because for efficiency (to prevent modifying the group or user whenever memberships change) the structure is as follows (<:: and ::> indicate entity references):

[group]  [<:: group_membership ::>] [user]

So in this case the system that wants to add these conditions also needs to indicate that this join is needed to modify the query, so here the information already becomes more complex. That's likely a change we can make in your proposed additive system by providing more information.

Lets complicate it more, we introduce a new business rule:

1. We want to add the additional restriction that Rule 3 only works on entities that have "initial review = TRUE" status. "initial review = FALSE" unpublished entities can only be viewed if the user has the additional "provide initial reviews" permission. This should only happen within a specific set of groups, it should not affect the global scope

In this case the additive system that's linked together with "OR" is no longer sufficient and we instead need to be able to identify the conditions added by Rule 3 so that we can create our changes on that rule.

I think within the current system (and within your proposed example) the biggest failure is probably that the conditions are mostly just put into a pile. There's no identifier for the conditions that allow saying "this should combine with or replace that".

I think the idea of having a separate ConditionFactory is the right direction towards the ValueObjects that Kristiaan was talking about. Basically "declare your goals" (which is very similar to declaring permissions) and then let another system combine those goals efficiently. The reason to stay away from queries for slightly longer is that part of the goals might be "I need data from this other part of the database".

@davidwbarratt, I'm not sure where the difference in vision is based on exactly but I'll make another attempt to see if I can clarify it. I'm also not sure how familiar you are with the group module and its usages, which could also explain why we're talking past one another.

For the below example I want to sketch the scenario where we have a Drupal (global) scope and we also have a Group scope. Within the Group scope there is additionally a sub-division of Groups with identifiers A through Z. This matches the terminology that has been implemented for the Access Policy API (where modules are entirely free to determine exactly which permissions exist within their scope and for a specific identifier). Kristiaan did a great video on the reasoning and how that policy worked at DrupalCon Barcelona.

Within the system we have five users:

1. User 1 can view content anywhere (permissions: "view content" in scope Drupal; and "view content' in scope Group)
2. User 2 can view content outside of a group but not in any group (permissions: "view content" in scope Drupal)
3. User 3 can view content when it's in a group but not outside of a group (permissions: "view content" in scope Group)
4. User 4 can view content in Group B only (permissions: "view content" in scope Group with identifier B)
5. User 5 can't view content (permissions: none)

I can see in the current "scoped" system quite easily how I could assign these permissions. The management of these permissions is also entirely delegated to the system that's responsible for the scope (in this case either Drupal core for the global permissions; or the group system for the group scoped permissions).

If I want to promote User 5 from not being able to see anything to being able to view content in a specific group, then I can do that by assigning them the "view content" permission in the group scope for a specific identifier. That's essentially what group does when a user becomes a member of a group.

If I understand your proposal correctly, then in your proposed system of additive permissions, I would have to first grant user 5 the "view content' permission globally and then add the group specific permission . However, now I have a problem because the meaning of "view content" has changed. If this flow would work then that would mean that "view content' itself does not provide global content viewing rights, otherwise I've overshot my goal and assigned User 5 too many permissions. We could say "view content" doesn't do anything without another permission, but now I've broken User 2.

The proposed solution then seems to be to have another permission "view content", "view global content", "view group content". Where the top level is some kind of "enabler" and the other two are there for the specific scope. However, this causes a myriad of problems:

• First and foremost, Drupal is now suddenly aware that it has a global scope and on a site that I don't have any other scopes, I'm still stuck with multiple permissions
• Even with this more complex permission structure, I'm unable to solve my use-case for User 4, because the permissions are only on the level of the scope, whereas I want to distinguish for a combination of scope and identifier (Group B only)

The above exercise is basically the reinvention of the Access Policy API and why a purely additive permission structure does not solve the problem but a more complex system was needed where modules had the freedom to override the entire access check within a specific scope to achieve more complex access scenarios.

One such scenario would be scaling the above example to 100s of groups with 10.000s of users (you now have an Open Social installation). At that point it becomes infeasible to add permissions in multiple places just to give a user access to a single, or multiple groups. The system itself needs to be able to apply certain rules based on data relationships that exist.

The goal of this issue should not be to re-litigate the Access Policy API. I think Kristiaan is right in identifying that's where we're going with the discussion (and while trying to figure out what example I needed above, I came to the same conclusion). At least in my eyes the Access Policy API is a system with a great and proven design.

Two non-group use-cases that I'm aware of to drive that home:

• Within Simple OAuth we currently decorate the User entity to be able to overwrite permission checks based on the token. With the Access Policy API we can put that into an access check so that permissions can be cached. The permissions are either the exact permissions assigned as scopes to a client (for system-to-system communication) or they are the intersection of the application's permissions for scopes and the authenticated user's permissions (for applications on behalf of a user).
• Within Open Social we have the concept of a verified user. Which is basically the authenticated user role but after someone has performed some verification steps (i.e. everyone gets it, not based on role, but based on some user state). This is currently a role juggling nightmare and with the Access Policy API we can implement that state based logic.

The Access Policy API in its implementation has shown that there are complex Drupal use-cases where an additive system is not enough and there needs to be the flexibility to re-arrange or even entirely overwrite, based on business logic (whether that's the scope; or even the time of day or whether another senior is currently working).

The goal of this issue should be to create a system in a similar vein as the Access Policy API, where the outcome is not individual permissions that feed on to other access checks, but where the result is a bunch of conditions that can be combined into a database query (SQL or e.g. MongoDB) so that we can efficiently fetch data we know the user has access to. Within the current system we're often forced to move that kind of logic into an entity access handler where we can actually perform complex logic, which leads to overfetching from the database, filtering out, and then fetching more to make up the result set.

Very interesting discussion! At Open Social we've run into similar issues as Kristiaan has expressed. Both within the context of our use of Group, but also in other areas (e.g. around finding user entities based on user profiles and complex platform and per-user settings). I'd have to dig into our codebase to find you exact examples but I'm not sure they would further the discussion here.

There's two observations that I'd like to add:

1. IFF we would solve access handling with a ValueObject/Non-Query-Condition system that Kristiaan is proposing (I'm a fan) then we would actually be solving a broader class of problems which is basically "Being able to filter in the database based on business logic" which can be huge for doing performant things like user-segmenting, without having to pre-calculate that constantly.

Which brings me to my second point which might help steer the discussion:

2. I think (apologies if my assumptions about people's mental model are wrong): there's a framing difference between what Kristiaan (and us at Open Social) are trying to achieve and the model mental model that davidwbarret is working with.

Effectively we have an access control system that, allows a user to enable a module, with the express intention of performing an access bypass. I think that is fundamentally the wrong way we should be thinking about access controls.

The use-case that group has (and that I'd argue exists in multiple places) is not that the module performs an access bypass. Instead it introduces a new "context" in which it is the owner of access checks. Using the group module as example.

For any content not within a group, the context is global and all the default permissions apply. However, for any content within a group, the global permissions may or may not apply and there is a different set of rules that should apply. Within the Access Policy API this exact problem can be seen as well and it was solved by adding even two levels of this (the $scope and $identifier variable) in the API.

I think this is what Kristiaan is trying to frame: there is a difference between which is a matter of user intent (I want to find green apples) and which is a matter of context specific rules (anyone can find green apples on the tree outside, but you can only see the apples in a secret apple club if you're a member of that club).

I would disagree that the club system trying to manage access to the content in its clubs would be an access bypass, because the site builder has specifically chosen to be able to operate in this different context.

Thanks for the additions and questions AlexanderAllen and quietone.

I've updated the comments throughout the file to address the feedback, after setting up api.drupal.org locally (that was more difficult than I expected). I wasn't familiar with the tooling so I appreciate the link! There were a few hurdles (such as the nested multi-line comment) but I've done my best to adjust the docblock so that it renders nicely on api.drupal.org and also added a link to explain the generics. I've ensured there's a newline between template definitions so that they at least don't bundle into a single paragraph but remain readable.

I realize that Drupal hasn't really used generics yet, but they're a very powerful feature to help developers deal with "container classes". In this case as AlexanderAllen explained, they're indeed not optional, so removing them to improve the documentation rendering would be a downgrade for anyone using a static analysis tool.

As an example the PHPStan types will make the following type detection possible and help you handle both the success and error path:

/**
  * @return Result<string, Exception>
  */
function get_result() : Result {
}

$result = get_result();

$value = $result->getValue(); // PHPStan will tell you that `$value` is `string|Exception`
if ($result->isOk()) {
  $value = $result->getValue(); // PHPStan will know `$value` is `string`
}
else {
  $value = $result->getValue(); // PHPStan will know `$value` is `Exception`
}

It's a really powerful feature and I hope we can improve the API Doc support for it in the future. Especially with applications such as 🌱 Static analysis for EntityStorage Active coming up to better document entity storage classes (and remove a bunch of logic that's currently in mglaman/phpstan-drupal).

I've done my best to change the way it's documented in the class to ensure that it remains readable within the current capabilities of api.drupal.org.

Access should be checked on the account proxy of the current user. Loading the user and checking directly on the user would create a security issue of escalated permissions since it ignores the token's restrictions.

Although the Access Policy API now somewhat changes this and reloading the user wouldn't actually change the permissions (since User::hasPermission delegates to the Access Policy API as of 10.3 → ), there are still implementations (such as Simple OAuth) which decorate the user object and change permission management that way (which was one of the few options available pre-10.3).

The hasPermission function of TokenAuthUser (which itself contains a fully loaded user object) looks as follows

  public function hasPermission($permission) {
    // When the 'auth_user_id' isn't available on the token (which can happen
    // with the 'client credentials' grant type):
    // has permission checks are then only performed on the scopes.
    if ($this->token->get('auth_user_id')->isEmpty()) {
      return $this->token->hasPermission($permission);
    }
    // User #1 has all permissions.
    if ((int) $this->id() === 1) {
      return TRUE;
    }

    return $this->token->hasPermission($permission) && $this->subject->hasPermission($permission);
  }

In case of a system token (client credentials) only the permissions actually on the token will be used. In case of a user token (authorization code) then the token will only be allowed to do what both the user and token have access to. This ensures that 1) The external application using a token can not do things it wasn't allowed to do (e.g. delete a user) even if the user can take those actions 2) The user can not take actions they otherwise wouldn't be able to take (e.g. an application can offer user deletion to sufficiently permissioned users, but the user must still be allowed to do this through the normal UI).

The issue that you're running into is likely that the scopes being provided do not grant sufficient permissions. There's currently no way in simple_oauth to grant a base-set of permissions and the choice to make scopes point to a single permission was a conscious design decision implemented in #3283821: Support individual permission and role reference in the new scope data model → . The original job of scopes was described as follows (from an internal Open Social document before our contribution to Simple OAuth).

OAuth scopes exist not to indicate what a user may do, for this we have an extensive permission system built within Open Social. Instead OAuth scopes define what an application may do, either standalone or on behalf of a user. It's possible that an application may have permission from a user to take an action on their behalf but still encounter an access denied error because the user themselves does not have the permission to perform that action.
It's therefor important to realise that when talking about OAuth scopes the scopes indicate what type of trust the user (either acting on their behalf, or the site manager giving access to the platform) places in the application.

My suggestion would be to map out your scenario and the permissions you need. It may either be solvable by changing your scope hierarchy, implementing a set of base permissions in Simple OAuth (anyone with a token regardless of scopes gets this), or by allowing multiple permissions to be assigned to a scope. (Those options are presented in order of author preference).

I didn't get around to answering the question asked by grashmash

@kingdutch it doesn't look like your patch actually addresses this issue. can you provide an example of how we can use the changes in your patch to achieve the goal of this issue, which is to NOT revoke users' access token when their account is updated, unless their password is changed?

I did my best to elaborately explain my motivation in https://www.drupal.org/project/simple_oauth/issues/2946882#comment-15747184 📌 Auth revoke on profile update Needs work but I'll see if I can provide a TL;DR below.

The current implementation with simple_oauth is great from a library point of view, because it's absolutely safe in all scenarios. A simple CMS which has editors should assume that any user account change might have access implications and thus invalidating the tokens is safe.

The main problem is that projects that don't fit that mold have no way to change this behavior. By moving the behavior to a service, it's trivial for any project to implement its own service and decide on what the business rules are for their use-case. Any system that this module is going to implement is going to be overkill for simple applications and likely miss rules for complex applications. It's also just quite a bit of code to maintain.

If I look at https://git.drupalcode.org/project/simple_oauth/-/merge_requests/164/ then it's cool that we add a lot of configuration but that's quite a maintenance burden that this module is taking on (and I can already see it does not cover some of my own use-cases; nor would I want the module to).

My counter proposal would be to merge the PR that I proposed where the only thing this module does is provide a way to swap out the invalidation logic (through a simple service provider). More complex logic like proposed in the PR could then easily be moved to a separate contrib module that can come up with the best way to capture various use-cases or make the system smarter.

kingdutch → created an issue.

Whelp, that’s embarrassing

Fixed, thanks!

Ah thanks! I searched for issues but came up empty :)

kingdutch → created an issue.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Drupal 7 has come to end of life and I haven't worked with Drupal 7 in many years so I'm marking this as won't fix.

Hey anybody!

I've tried to get this working before but it's unfortunately not trivial. The module is tested with Behat (which has my preference because most of the module happens in the UI and Behat is easier to read than PHPUnit functional tests), but I have not been able to get Behat working within GitLab CI.

As an additional developer experience challenge, this module depends on a compiled JavaScript package that consumes the Yoast SEO library, creates the widget and wraps it in a composer.json: https://github.com/Kingdutch/RTSEO.js. This was originally included within the module itself but was split out in #2726871: External library included without exception approval from the Licensing Working Group → . The module and the JavaScript it depends on are currently worked on and versioned separately.

My goal is to move both packages into a monorepository in https://github.com/Kingdutch/drupal-real-time-seo which would include GitHub workflows to do perform the Behat testing (since that's something that I know how to set up trivially). The module code would then be brought back into GitLab through a subtree split so that it can be distributed through Drupal's Packagist.

That would allow the module and the JavaScript to be versioned in lockstep and allow changes to either to be immediately tested together using automated tests.

Hey Anybody,

Yes I think that follow-up would be 🐛 Update YoastSEO JS to recent version Needs work . The newest release for the Yoast SEO library which finally came somewhere last year moves most of the processing to a webworker and keeps only the UI updates on the main thread. So that ensures that even if the server needs a bit more time we're not blocking.

My 5 year ago developer idea was to do that update with an overhaul of the widget itself and move that to React so that there's fewer event listeners and a more linear "Updated data comes in" -> "Render UI element top down" approach. One of the features that that should enable in the future is multiple keywords at the same time. However, I think that's a much larger undertaking so it makes sense to refocus the update issue to moving to the newest version as-is and seeing if we can only make smaller improvements to the existing widget, like replacing jQuery with vanilla JS.

2.0 has been tagged today. Since I was unable to make the improvements that I wanted, I've merged the improvements contributed by the community, so that the module at least keeps moving forward.

Closing this issue and then we can see how we can bring in more improvements into future updates and realise some of the original plans through smaller steps :)

Thanks for your contribution!

Thanks for your contribution!

I guess given that 🐛 Using the module filter on the module list page a module can only be found by system name if the description ends with a period Needs work is fixed which I once reported as a result of this module not having a dot at the end, I guess we can now make the module consistent with everything else and add the dot ^^

Thanks for your contribution!

Good catch, that file should already be minified so it shouldn't be reprocessed.

Thanks for your contribution!

Thanks for your contribution!

Thanks for the contribution! I've gone with the smaller fix:

diff --git a/src/EntityAnalyser.php b/src/EntityAnalyser.php
index 0734233..e6555cf 100644
--- a/src/EntityAnalyser.php
+++ b/src/EntityAnalyser.php
@@ -213,7 +213,7 @@ class EntityAnalyser {
     foreach ($metatags as $tag => $value) {
       $metatags[$tag] = str_replace('[current-page:title]', $entity->label() ?? '', $value);
       // URL metatags cause issues for new nodes as they don't have a URL yet.
-      if ($entity->isNew() && (substr($tag, -4) === '_url')) {
+      if ($entity->isNew() && preg_match('/[.\-_:]url/', $value)) {
         $metatags[$tag] = '';
       }
     }

The other patch also changed the logic of the title by moving $entity->isNew() outside of the loop altogether, which seemed to do something differently than what was needed for the bug report.

If this issue persists, please ping me on Slack and we can replace it by one of the other patches.

Thanks all! The logo from the Open Collective page will have a special place in my heart, but I think the logo that sourojeetpaul made will likely fit better amongst all the other Drupal modules in the project browser so I've committed it to the repository. :)

I've been hoping for the past time that I'd find the time to fully update the Real-Time SEO module to the now released version of the Yoast SEO library so that we could move to webworkers and fix this issue. However, unfortunately that has required more time (and a better development set-up then I currently have available).

Given that the first release of Drupal CMS is around the corner and the Real-Time SEO module will be part of it, I'm adding the button and tagging a stable release. However, I do hope that in a future release we can once again remove the button and go back to "Real-Time" while preserving a smooth experience :)

While merging I've split this issue in two. One to refactor the controller into a SettingsForm and the other to introduce the actual change with the new setting. I also fixed some comments to use more inclusive language.

Thanks for your contributions everyone!

Thanks for your contribution!

The offending plugin has been removed as part of the update to Group 2 and replaced with a different solution.

For anyone else finding this through search and wondering why you have it in your code-base. You're likely using 📌 Optimize getCommentedEntity() Needs work as a patch which introduces the method on ContentEntityBase (and doesn't add it to any interfaces, so PHPStan will complain for you).

nicxvan → credited kingdutch → .

There's a slight "API change" this causes that may trip some people up (which I don't think is worth fixing, but I want to share it because it may save people some debugging time).

In previous versions of group it was 'valid' to call $group->addMember($user, ['group_roles' => '']) where group_roles was an empty string. Because of how Drupal treated fields this would just be seen as an empty value and otherwise ignored in code.

Now that the validation exists, the group module will try to load a route with an empty ID which can never exist, thus throwing a validation error. This means that you code should check that you're actually assigning a role and that you didn't receive empty user input. If you're not assigning a role you should unset the group_roles entry of the membership data.

(This scenario occurred for us in test code where scaffold data was being parsed from markdown tables into arrays. Empty fields in this system are interpreted as empty strings.)

For me #3324241: Provide DIC-friendly, concurrency-safe alternative(s) to `drupal_static()` → could be part of that.

I think focusing on “reset” is the wrong part of the equation. The focus should be on creating a context for static caches to be attached, to that allows for switching or clearing. A request may have such a context and it may be shared (e.g. between sub requests). However it should also be able to exist without a request (e.g. in a long running process).

A long running process might have a memory cache that has changes from another application passed to it through a message bus to make sure it doesn’t use outdated data.

The API that Drupal provides should make it possible for services to easily use the right cache at the right time, that’s likely the hardest part.

Catch already did a great job explaining in text. If anyone comes across this and is looking for an explanation that includes visuals then I recommend watching the talk I gave at DrupalCon which attempts to explain the scenario's in which the Revolt event loop will help us now and in the future: https://www.youtube.com/watch?v=tfppKrK1zGU

In the past week I've also been a guest on the Talking Drupal podcast where I did my best to answer similar questions that may be asked slightly differently and help it click: https://talkingdrupal.com/474

The question about the Async Database is a good question. The event loop can indeed use streams directly (as e.g. amphp/mysql does). However, with the primitives that the library provides it's also possible to do it in a looping manner. For example:

function drupalAsyncDbHandler(....) {
    // Start query that requires polling
   mysqli::startSomething(...);

    $suspension = EventLoop::getSuspension();

    // Check our database connection whenever nothing else is happening.
    $callbackId = EventLoop::repeat(0, function ($callbackId) use ($suspension) {
      // Ensure only one instance of this callback runs at at time.
      // Not needed if we're 100% sure that the rest of this function is synchronous.
      $ready = mysqli::poll(...);
      if ($ready > 0) {
         
         // Fetch a result
         // Continue the code that's waiting for us with the query result.
         $suspension->resume($result);
         return;
      }
       
      // Eat the error if the repeat was cancelled.
      // This could happen if we cancel the request and no longer need the result for example.
      // Until: https://github.com/revoltphp/event-loop/issues/91.
      // Otherwise since we're not done we try to poll again in the next callback.
      try {
        EventLoop::enable($callbackId);
      }
      catch (EventLoop\InvalidCallbackError $e) {}
    });

    // Wait for the result to have been fetched from the database.
    $result = $suspension->suspend();
    EventLoop::cancel($callbackId);
    return $result;
}

If you're dealing with Revolt primitives directly then you'll have to think about the async states so that your calling code doesn't have to. For contrib there's the options of pulling in a lower level library of their choice (e.g. ReactPHP, amPHP or something new) to do this for them.

For the above snippet I modified one of my examples of the Revolt playground which attempts to demonstrate some of the scenarios you might currently find in Drupal (using Fibers) or other scenarios that have been discussed that we might need: https://github.com/Kingdutch/revolt-playground/

📌 Add revoltphp/event-loop dependency to core Active has been merged so this is no longer blocked.

Should this be backported to 10.4.0? I'm not entirely sure what upgrade paths are supported, can site builders go from 10.4.x to 11.0.x or are they expected to jump from 10.4.x to 11.1.x?

Contrib code that would use this would be compatible with ^10.4 || ^11.1 only, if we backport (i.e. would break on 11.0 since it's introduced as feature in 11.1).

dan2k3k4 → credited kingdutch → .

gábor hojtsy → credited kingdutch → .

Adding credit for an internal review that pointed out a spot was missed (which was fixed in the eventual commit).

I think a change record makes sense as per #21.

Moving to "Needs Review" for the CR to be reviewed and approved :) Implementation is unchanged and was approved in #18.

Added tests :D

kingdutch → created an issue.

The (minor) risk I see with this is that people start adding diverging types to class methods that inherit a common, yet untyped method interface. Covariance would allow do so now, but then it will make life more complicated later once we will try to typehint the interface.

I think PHPStan can actually really help us here, in the way I described. I think that's something the Drupal Update Bot may be able to do even. However, it requires that we update coding guidelines that projects that consume Drupal should, before they upgrade a Drupal major version:

• Ensure they're on Level 3 (or above) OR enable at least PHPStan\Rules\Methods\MethodSignatureRule
• AND ensure the parameter reportMaybesInMethodSignatures: true is configured

If we consider changing the PHP return type of an interface as a breaking change and reserve it for majors (12.0.0), i.e.

interface Foo {
  // Before
  public function bar();
  // After, this would be a breaking change.
  public function bar() : string;
}

However, we ensure that we fix the PHPDocs correctly:

interface Foo {

  // Before
  /**
   * @return
   *   The return type.
   */
  public function bar();
  // After, this is not a breaking change and PHPStan starts flagging it downstream.
  /**
   * @return string
   *   The return type.
   */
  public function bar();
}

The above code would flag an implementation issue given the configuration described at the start: https://phpstan.org/r/f54ce223-907b-49b2-a45e-70e817347b13

Although this does go against the PHPStan philosophy which is "Don't pick and choose which rules you apply, it works best if you apply all of them", at this point I'm happy to have any progress towards better type support in Drupal.

The only thing I'd want to add is that this doesn't change the need for the related issues mentioned in the parent. Even though it provides an important step in the right direction by adding return type hints for new code.

Hey everyone!

I'm excited that a release for the Yoast SEO library has been created, which unblocks some of the work.

2.x made some important changes vs 1.x. Most notably it provides better analysis support in how entities are rendered rather than looking at individual fields itself. It also better manages how the Real-Time SEO module attaches fields to entities. I initially wanted to use that momentum to make some more changes.

Most notably I wanted to change how the analysis results were stored to enable a few things:

1. Store both the SEO score and the reading ease score; currently only the SEO score is stored (the library has since also added an "inclusivity score"
2. Adjust storage and the widget in a way that allows multiple keywords to be analysed
3. Store these results in a way that allows creating overviews to enable insights into how pages rank (e.g. comparing pages for the same (related) keywords).
4. Decouple analysis from the form itself so that in the future it could also be used for on-page analysis.

Secondarily I want to ensure the webworker is used by the widget to fix issues like 🐛 The page freezes while the Real-Time SEO module runs script Needs work without adding a button the user needs to press manually.

Finally this module currently allows editing the title and description, but realistically a better UX can be provided by having a site builder provide a dedicated field for this and only rendering the result from the metatag module (that also ensures more complex logic can be included).

The biggest reason not to tag a stable 2.0 would be that these changes might cause changes to the code or storage which can break other modules if they rely on a specific structure. From a data perspective these changes can usually be made while preserving the data itself so that for actual site users there's be an upgrade path within the 2.x-* range of versions.

I think the next steps now that this module will make it into Drupal CMS would be to create a clear plan of which of these features would be needed for 2.0 and which should be pushed onwards to a 3.x version.

One of the things I hope to do in the coming week is to move development of the module from Drupal's GitLab to https://github.com/Kingdutch/drupal-real-time-seo. This is because the module requires both the Drupal code of the module itself as well as a custom JavaScript library which contains the widget: https://github.com/Kingdutch/RTSEO.js This allows both the module and library to be developed and versioned in a single repository (with the same version) and allows the Behat tests to run on GitHub (I've not been able to get those to work in Gitlab). The module code can be subtree split back to GitLab so that MRs here are still possible. The library code was previously split out in #2726871: External library included without exception approval from the Licensing Working Group → . That should massively ease development.

I'll also be at DrupalCon Barcelona where I hope to have time to do some of the above things :)

kingdutch → created an issue.

Forgot to update the IS.

With the other improvements that were made in Drupal this can now be solved with Symfony's decoration_on_invalid tag directly. Since the related issue about replacing YamlFileLoader entirely has hit some speedbumps I think it's worth it to add decoration_on_invalid support to our own YamlFileLoader already.

We're seeing that the community is adopting "decorate over extend" more and more and this would be a big DX win by allowing a single property in a YAML file to replace an extra PHP class for optional decoration. It also lessens the gap to Symfony for any future replacement of the YamlFileLoader.

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

Hey kopeboy,

Good question!

This module is definitely not deprecated with the introduction of recipes. It was actually created at a time that Recipes was already pretty usable. Though there way be some overlap in problems that it solves, the primary goals of both projects are different.

Recipes are intended as a way for site builders to quickly apply common configuration to their site. For example to be able to add an event functionality to your website, or to get started quickly with a blog that has commenting functionality.

Config Modify is a much lower-level tool that's aimed at module developers. The shortest way I could describe this module is "partial optional configuration". Within Drupal, a module can use the config/optional module to say "If some criteria matches" (e.g. the views module is enabled) then also install this config. The problem with config/optional is that it only works on whole config objects (e.g. the view itself). At Open Social we often had a need to make partial configuration changes. For example "if module X is enabled then modify this view with a single field". This previously required writing code in hook_modules_installed and hook_install to be order agnostic, but can now be easily described in a YAML file.

The problem we solved in Config Modify is very specific to the type of application that we're building. In most cases for Drupal you'll likely want to leave the choice of "if both these modules are enabled add this field" to the site builder themselves, because some may not want it. However, in the case of a Drupal-based product, that flexibility is much more limited and Config Modify provides us with a good way of ensuring that if 2 separate features are enabled the integration is also provided to make them work together.

So in summary the Recipe's initiative is a powerful site builder tool to let them determine what configuration and functionality should be present. The Config Modify module is a developer tool to let multiple modules/features be integrated if they are enabled at the same time.

I hope that answers your question!

klausi → credited kingdutch → .

klausi → credited kingdutch → .

klausi → credited kingdutch → .

I just wanted to add an alternative which was previously created by by GoalGorilla. A version with white text features on the Open Collectie page.

It was accompanied by the following background.

I also like what sourojeetpaul made, though I'm wondering if it wouldn't look better without the browser window :) I think I might primarily be looking for something that's a bit more colorful.

Happy to let some other people from the community chime in here.

I guess we postpone this until 📌 Configure use-statement sniff to be case sensitive Active is done.

kingdutch → created an issue.

kingdutch → created an issue.

Applied the changes from the MR in one of our projects and together with the CI feedback found I made a few typoes. Fixed those in the MR which means my proposal is ready for review; or for "needs work" in case people disagree with the direction :)

We found this issue to be a blocker in some of our projects. Unfortunately the existing MRs were not a solution that would work for us.

Philosophy behind this simplified change
The main idea behind the proposed change is that when tokens should be invalidated will differ between projects and be highly implementation dependent. This can also be seen by the fact there there's a discussion ongoing for about 6 years.

Additionally changes to consumers and users may happen frequently depending on the application that's being built. This makes an event a possibly costly thing to process. From an ecosystem perspective it's also likely undesirable that individual modules start listening to this event and implementing logic for when they think tokens should be invalidated. If a project uses such a module and disagrees the recourse for them to change the outcome of the event may be challenging.

With that in mind there's likely going to be a single override of the logic per project. At that point the event is no longer needed and the absolute simplest implementation is moving the logic into a service that can be swapped out by whatever project that needs it.

Why not change the logic right away
In my personal opinion any change to the logic for invalidating tokens should be considered a breaking change and require a major version bump. If sites are currently relying on the logic to always invalidate and their security sensitive scenario is not included in the proposed defaults (password, roles, and status) then updating may cause them to suddenly have a security issue. Using a major version would be the proper SemVer way to communicate this. Alternatively we go for an implementation which doesn't change the logic but requires purposeful action from a developer to change the logic.

Similarly with these kinds of changes which may be security sensitive we should keep the change as small and focused as possible. This makes it easily reviewable, understandable and in worst case, revertable.

Both PR 53 and 99 suffer from trying to do more than is in scope of this issue. Code quality issues in unrelated files should not be included in a potentially security sensitive PR. Issues like [#2978041] and 💬 [PP-1] Revoke refresh tokens Needs work should also be discussed and resolved in their own issues for that same reason, rather than trying to group them into a single change.

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

I've skipped this fix for now because it doesn't touch production code for the module and the tests are indeed not running in GitLab. Getting Behat to work there was unfortunately not easy. We can make this change in a way that we can test both Drupal 10 until EOL and Drupal 11 once the tests are fixed :)

Thanks for flagging it!

Tagged 8.x-1.9 → and 8.x-2.0-alpha11 → with Drupal 11 compatibility. Thanks all and thanks thejimberch for pinging me on Slack :)

What is the reason we're introducing new methods? Especially since we want to remove andif and orIf. That means we expect all future code to be more verbose and redundant in being named after its argument type (which, unless there's multiple alternatives, is generally considered an anti-pattern).

Could we achieve the same thing through a deprecation dance which changes the parameter types but not the method names? orIf and andIf are both already statically typed to AccessResultInterface so we could first widen them to callable|AccessResultInterface, deprecate on a AccessResultInterface being thrown and then later dropping the AccessResultInterface type altogether to allow only callables. That leaves the final codebase still with the succinct orIf and andIf but with the desired callable type.

nicxvan → credited Kingdutch → .

larowlan → credited Kingdutch → .

The proposed improvement in #1158720: Improve text for parameter type hinting in function declaration → shows that the type-hinting block is likely not only about interfaces (3 in this issue). As such I don't think it should be removed in its entirety, but we likely want to combine both changes. The wording proposed in that issue may already give more room for this issue since it qualifies interface usage with "if available", which I still think is what people should be doing.

I think the newer text here is more useful to outline Drupal's stance towards PHP hints. I also think that the new text shows that removing the block entirely ( #3263602: Allow type hinting with classes where appropriate → ) leaves a gap in our coding standards about type-hinting. That's likely because the other issue is focused only on interfaces.

Kingdutch → created an issue.

Dated as per #1539738-11: Define and document a policy on list() syntax →

quietone → credited Kingdutch → .