undefined - Contrib.social

📌 | Drupal core | [PP-1] Ensure asynchronous tasks that run beyond the length of a request have the chance to complete before process exit

🇳🇱Netherlands kingdutch

You don't need to test deprecations directly unless there is complex BC logic.

Then I think in this case a test isn't needed since all it does is trigger a deprecation? There's no logic involved.

The question is what happens if someone is calling that function directly and the files get loaded in composer as well.

The requires have been removed from the function, so nothing happens. If someone has their own implementation that copy pasted the files then PHP will not require the files again because it was using require_once so it was a no-op.

My worry about unit tests would be performance but I am so often wrong in what is useless micro optimization and what is legit worry.

Fair concern. I think without profiling directly I can only reason towards "it's unnoticeable/no impact":

The list of files that was automatically loaded (without loading any classes; just preloaded files) was 40 items long and has now increased to 45. That on its own is a significant increase. However, if we look at one of the unit tests that was changed in this PR, Unit/LanguageNegotiationUrlTest.php, then that already loads 100-200 classes and files just for the test and this does not include what PHPUnit itself might use.

If we make the math simple and we say a simple unit test in Drupal requires 100 files loaded to run, then we're adding a 5% increase in file load (assuming all files are equal). Then if the test did nothing but load files we'd have a 5% decrease in test performance. However, file loading is usually a small part of a test so the 5% is really then an upper limit and is much likely far below 1%.

The reason I focus on file loading is that the files themselves only contain function definitions and don't run any code, so they don't actually execute anything.

I hope that alleviates your worry.

I've written a change record and addressed the review feedback (except for the deprecation test).

✨ | Drupal core | Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments

🇳🇱Netherlands kingdutch

If the event loop is in index.php, does that mean that e.g. drush is required to run it and will break without it, or does it mean that drush would need to implement it to support the event loop? If it's the latter, that seems fine to get things going?

Anything that ends before the request would be fine. So if you use Revolt to schedule multiple async tasks and then suspend until they're completed, then that works fine. This is the case for most tasks (like async database queries) that are a dependency for the final request to be sent.

Tasks that run longer than the request lifecycle would be killed by the process exiting because the EventLoop::run() is needed to block the process. This would be the case for things like optimistic cache warmups that were scheduled before the request completed but are not yet done.

To support those kinds of tasks, Drush would have to implement it itself.

Alternatively if putting it in the kernel enables it everywhere, it feels like that would then enable us to move it to Symfony runtime transparently later.

I guess you're right. The biggest issue I had with putting it in the Kernel would be that we'd still block the possibility for people to create a long running process that might handle multiple requests at the same time, since in that case each request would call EventLoop::run() with its kernel, which would conflict.

With that in mind I prioritised ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active . I hope that there's some support to get that finalised and merged and then this can go on top relatively quickly as an adjustment to DrupalKernelRunner. Drush could then either provide its own runtime for an application, or Drupal could provide one for e.g. ✨ CLI entry point in Drupal Core (Dex) Needs review that Drush could also benefit from.

After that has landed, they can provide the modified kernel or application and we're free to provide any environment changes that may be needed without requiring them to adjust code.

✨ | Drupal core | Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments

🇳🇱Netherlands kingdutch

Forgot to snip a reformatted paragraph, whoops.

🇳🇱Netherlands kingdutch

Updated the issue summary to help reviewers.

📌 | Drupal core | Replace DrupalKernel::loadLegacyIncludes with composer autoload.files

✨ | Drupal core | Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments

🇳🇱Netherlands kingdutch

Will this affect unit tests?
If I understand correctly the legacy files would not be loaded before this change, but after this change all unit tests will load all legacy includes.

Not sure if this is a problem or acceptable, but it likely is a change.

You understand correctly. There were two tests that still relied on those functions and redefined them themselves, those have been changed in the diff. For the other tests if they ignore the functions then nothing happens. I guess the only risk is that people start using them in more places? However, there's nothing stopping someone from doing that in a place that's covered by a test that's not a unit test, so I'd argue that's acceptable.

I'll write up a change record and update the deprecation links later this week :) I'm not sure how I would go about writing a deprecation test. Any pointers to an example would be welcome.

✨ | Drupal core | Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments

🇳🇱Netherlands kingdutch

Took some force-pushes (one day I'll get FunctionalTests working locally again) but the MR is now green across the board :raised_hands:

📌 | Drupal core | Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service

🇳🇱Netherlands kingdutch

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

📌 | Drupal core | Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service

🇳🇱Netherlands kingdutch

I've been playing with this issue (and some related things) all day and I've come to the conclusion that this (and a few other things) become about 100x easier if we adopt Symfony's Runtime component because it provides a clear separation of responsibilities for the "Runtime" and the "Request handling" (Kernel).

Since I'm actively working on ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active to make that happen (it exactly solves the issue described by Andy in #105) I'm marking this as postponed on that.

🇳🇱Netherlands kingdutch

I've been working on trying to figure out how to move Drupal to a place where it might be able to handle multiple requests in a single process as part of #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole → and 🌱 Adopt the Revolt event loop for async task orchestration Active . While doing this I've been looking at how the Symfony Runtime works which has already been requested for adoption in ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active . With that background I've been looking at this issue (since global state is problematic).

If I look at the proposed app service, then it doesn't really work until `setRequest` is called, which shows that up until the DrupalKernel has bootstrapped, as a service, it doesn't work within the container. We can also see in the proposed MR that there's a hidden dependency of App on a Request in our tests, even though those tests might be testing components which should be independent of a request. That highlights an architectural issue.

Tangentially, the name App is somewhat confusing because it has overlap with the concept of "Application" which could be something like Drush.

With those things in mind I would like to propose a change in solution direction:

- The App class should be an AppContextInterface which is pulled out of DrupalKernel.
- The class AppContext is added with most of the code of the proposed App class which is the only implementer of AppContextInterface in Drupal
- AppContextInterface and AppContext are extended from the proposal to include getAppRoot which replaces the function of the same name on the kernel.
- The $app_root argument on the DrupalKernel constructor is replaced by an AppContextInterface instance (some type checking and backwards compatibility work would be needed to manually instantiate this if not provided).
- Front-end controllers like index.php are updated to create AppContext from the request which is already present there and pass it to the kernel.

Performing the change in that way makes the path configurations part of the runtime, rather than the kernel. This makes it easier for non-request based applications like Drush to create their own context implementation.

Making and maintaining these kinds of changes in the index.php, install.php, update.php, and reload.php at the same time may be somewhat cumbersome. This is exactly what the Symfony Runtime component tries to solve (with a few of those responsibilities now included in the DrupalKernel).

🐛 | Drupal core | Validatable config job is broken when MR introduces new dependencies

🇳🇱Netherlands kingdutch

Updated the title and IS. Ran into this while working on 📌 Replace DrupalKernel::loadLegacyIncludes with composer autoload.files Active .

📌 | Drupal core | Replace DrupalKernel::loadLegacyIncludes with composer autoload.files

🇳🇱Netherlands kingdutch

📌 | Drupal core | Replace DrupalKernel::loadLegacyIncludes with composer autoload.files

📌 | Drupal core | [PP-1] Ensure asynchronous tasks that run beyond the length of a request have the chance to complete before process exit

🇳🇱Netherlands kingdutch

kingdutch → created an issue.

✨ | Coding Standards | Disallow PHPDoc comments within a functions parameter declaration

🇳🇱Netherlands kingdutch

Linking ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active while I try to figure out if it's a requirement or not. The feeling from initial trials of placing EventLoop::run in the kernel is that this shouldn't be a Kernel responsibility, but the runtime itself should handle the event loop.

This is less obvious in a single request environment which Drupal currently primarily works in. However, it becomes more obvious when extrapolated to other environments like Drush or applications that handle multiple requests where there should still be a single event loop at the root of the application. The DrupalKernel is based on Symfony's HttpKernel whose job it is to "Turn a request into a response". However, this suggests that an application that handles multiple requests might want an instance of the kernel for each of them so that they're properly isolated. This becomes clearer when looking at TerminableInterface which terminates with a specific request/response pair.

We could likely get to the EventLoop at root by just putting it in the index.php. However, that means everything that deals with Drupal needs to run EventLoop::run itself. That becomes much easier when standardising on a runtime pattern like Symfony's runtime.

✨ | Coding Standards | Disallow PHPDoc comments within a functions parameter declaration

🇳🇱Netherlands kingdutch

Since the documentation is already in order as per #5, I'm moving this to the execution project :D

🇳🇱Netherlands kingdutch

kingdutch → created an issue.

✨ | Coding Standards | Stop using uppercase TRUE, FALSE, NULL literals

Comment about 2 months ago →

🇳🇱Netherlands kingdutch

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.

📌 | GraphQL | Implement strict typing + return types + property types on all our code

🇳🇱Netherlands kingdutch

klausi → credited kingdutch → .

✨ | Drupal core | Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

✨ | Drupal core | Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

🇳🇱Netherlands 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:

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

✨ | Drupal core | Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

🇳🇱Netherlands kingdutch

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:

->condition('status', 1) - User can view any published entity
->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:

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:

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".

✨ | Drupal core | Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

🇳🇱Netherlands kingdutch

@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:

User 1 can view content anywhere (permissions: "view content" in scope Drupal; and "view content' in scope Group)
User 2 can view content outside of a group but not in any group (permissions: "view content" in scope Drupal)
User 3 can view content when it's in a group but not outside of a group (permissions: "view content" in scope Group)
User 4 can view content in Group B only (permissions: "view content" in scope Group with identifier B)
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.

🇳🇱Netherlands kingdutch

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.

✨ | Drupal core | Implement a Result type in Drupal Core

🇳🇱Netherlands kingdutch

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.

🐛 | GraphQL | GraphQL OAuth Authentication Fails Due to Incomplete User Entity in Access Checks

🇳🇱Netherlands kingdutch

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).

📌 | Simple OAuth (OAuth2) & OpenID Connect | Auth revoke on profile update