🇳🇱Netherlands @kingdutch

Addressed the feedback where possible, provided explanations for why the code looks the way it does in other places.

kingdutch → created an issue.

TL;DR: Landing ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active first and retaining $app_root (and the broader AppContext proposed in 📌 Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service Needs work ) ensures DrupalKernel is decoupled from the structure and would allow this issue to be implemented in contrib entirely before moved into core. This also ensures that if a contrib project has needs not covered by the DrupalLocation implementation or the fallback to the current structure, that the project could implement the logic itself.

Joachim shared this issue on another issue that I was working on. I see a lot of great work has already been done here and I'm a big fan of decoupling Drupal from its current structure since getting things into `vendor/` will make development a lot easier.

One request that I have for this issue in a slight change of direction is that we NOT deprecate the app_root parameter.

In my proposal for the related issue 📌 Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service Needs work I outlined how we might replace $app_root with a broader context object that can be used by different type of applications to help decouple Drupal as an application framework from requests.

The request is also related to 📌 [meta] DrupalKernel has too many responsibilities Active . It's great that this issue makes Drupal more agnostic to the folder structure. However, it also increases the responsibilities of the DrupalKernel. It requires a project either to use the scaffolding plugin or that the structure matches the current structure and the app_root is exactly dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2);. If you have a contrib project that for whatever reason doesn't fit these criteria (maybe you're integrating Drupal somewhere where the plugin is not an option), then your project is now stuck and there's no way to change the Kernels' behavior.

Essentially where the "app_root" is located depends on the runtime environment in which Drupal is used. We may not be able to predict all the possible scenarios that we could support. The main issue we run into in Drupal's current form is that the only place we can do set-up outside of DrupalKernel is in index.php, which feels like the wrong place.

I propose that before this issue is merged we work on ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active . This would allow the $app_root determining logic to be placed in the new DrupalRuntime class. We can then update the front-controller (index.php and associated files) to get the value of $app_root from the runtime (which would contain the logic that this issue puts in DrupalKernel::getApplicationRoot) and then passes that on to DrupalKernel.

That change ensures that the DrupalKernel is truly root agnostic, and contrib can even propose new ways to do the determination of app_root. For example, this issue could've been implemented outside of Drupal core for prototyping in that scenario.

Accidentally committed the file mode changes for the scripts that I needed for testing :) Reverted those files back to non-executable.

PHPUnit doesn't use the root as working directory, which requires the path for the CLI front-controllers to autoload_runtime to be absolute, similar to autoload.php

Coding standards got me.

The change to `DrupalRuntime` is kept as close to the `SymfonyRuntime` as possible to ensure consistent behaviour and make switching to that runtime in the future easier. The switching is not done as part of this issue since the SymfonyRuntime has a constructor which does a bunch of runtime configuration that is easier to discuss in a separate issue.

The latest commit converts the core scripts to use the Symfony Runtime pattern. The console runner behaviour is copied from Symfony's Runtime (since the generic runtime does not support applications). This provides us with input and output handling and adds environment and debug handling for free.

The bootstrap of Drupal in the scripts is not yet moved out of the front-controller. Rather than guess at a solution now this is reserved for https://www.drupal.org/project/drupal/issues/2363353 📌 Refactor / split up \Drupal\Core\Site\Settings singleton, and introduce other multisite-related classes. Postponed: needs info and https://www.drupal.org/project/drupal/issues/2529170 📌 Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service Needs work which can make these changes for the Kernel and the console at the same time in a structured manner.

The blog post is done and can be found at https://web.archive.org/web/20250908093711/https://www.alexandervarwijk.... (Web Archive Link).

I don't currently have a proposal for how we might tackle this issue. However, if we view this issue within the context of the broader componentization of the DrupalKernel then I do expect this issue to become much easier. With the changes described so far, we're moving a large amount of logic out of the Kernel.

Having these components outside of the Kernel will make their true dependencies clearer and makes it easier to change how they're instantiated or introduce new components altogether. For example we may be able to move the building of the container out of the Kernel, setting only certain requirements for the parts of the container that the Kernel itself needs but leaving other aspects of the container up to the application.

This is a discussion that fits into the broader dialogue of how we can leverage Drupal for more complex applications. A long running application that may perform background tasks or handle multiple Drupal requests at once for example may need its own container to set-up the socket server or other services outside of Drupal. By rethinking where the container is built these applications and the multiple Drupal requests that it handles, may be able to build the container only once*.

* Many Drupal services currently rely on handling only a single global request.. For example the current.user service. In order to make the container truly reusable across requests Drupal would need to introduce a "task" (e.g. "request") concept that parts of the code can use to get the current user.

Adding ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active as related issue because it provides a path towards doing set-up before the DrupalKernel is loaded without requiring end-users making constant complex changes to their application entry points.

I think it makes sense to keep this issue open.

In my recent blogpost (Web Archive Link) I've outlined how this comes essentially for free with ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active while providing end-users an easy way to swap out the logic if needed.

[The Symfony Runtime Component usage] provides us with a standardised way to get the environment name regardless of what kind of application you're building. For the initial Runtime implementation we may even resolve the $environment variable statically to prod to mimic the current behavior. That would allow work to be done in [this issue] to determine the name of the variable that sets the environment and then provide this as an update to Drupal websites without having to touch their front-controllers again. Alternatively if they're unhappy with how Drupal determines the environment, then they can overwrite the runtime to resolve this variable themselves, without having to touch the front-controller.

I've outlined how this issue can make other issues (see "referenced by") easier in https://web.archive.org/web/20250908093711/https://www.alexandervarwijk....

The TL;DR is that it aids in decoupling the front-controller (index.php, update.php, or script entrypoint) from the environment. In this way, the end user can retain control over what the application does (e.g. user the DrupalKernel to handle a request; or use Drush to run a command) while still providing Drupal Core a place to split out responsibilities from the DrupalKernel that set-up the environment itself. This happens in a way where the end-user can easily use what's built-in to Drupal, but also replace these parts of they don't suit them (e.g. to use FrankenPHP or AMPHP).

I'm working on a blogpost that ties this in to some other things that exist in Drupal Core, so I believe this issue is still relevant. Depending on how we structure the work we may close this as a duplicate of something else, but for now I think it's still a useful overarching issue.

There’s another thing to consider in the React vs “other framework” debate which we recently went through at Open Social, where we opted for Svelte for a widget on a page, but not the entire page.

React requires that all code on the page use the same React version. Due to its use of globals there can also only be one version of React active on a page.

For things like Experience Builder this is fine, since it’s expected to control the entire page (as far as I’m aware). However, for other widget style elements, like a pagination, or inline form errors, this can be much more problematic.

The React ecosystem moves at a very different pace than Drupal, and having to update any custom React code you have at the pace of Drupal core (whether that’s fast or slow) can be a significant challenge.

With htmx that issue is much smaller and it allows front-end theme builders to pick which version(s) of a front-end framework like React they use, if they want to add it, independent of what Drupal core is doing.

So from that point of view, having React for full page apps in Drupal and HTMX for small sprinkling of interactivity, is the more responsible choice as a framework.

This issue is still relevant. It's part of a larger set of issues around core Drupal classes having too many responsibilities.

One discussion was around "Drupal Application Context" highlighted here: https://www.drupal.org/project/drupal/issues/2529170#comment-16075215 📌 Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service Needs work

Similarly the ongoing discussion about the many things that the DrupalKernel is doing: https://www.drupal.org/project/drupal/issues/2282779 📌 [meta] DrupalKernel has too many responsibilities Active

As donquixote outlined in the issue summary and the first comment, the Settings class currently isn't just a read-only container of settings. However, it's also responsible for loading those settings depending on the current site and even setting up the database configuration.

damienmckenna → credited kingdutch → .

I'm assuming a stance that if people want to use patches of unsupported (non-released) changes in their own project they can be expected to make their own patches. In that case, it's built in git functionality to to exclude files with git's powerful path matching syntax. That works in diff, log and format-patch to name a few of the commands that use the same underlying features.

As an example for a recent Drupal core commit (using --stat to limit the output to what's relevant for the discussion):

$ git diff --stat 422099995ecfe78f1e29248374fc05cd4a556c1d^!   

core/modules/package_manager/src/ExecutableFinder.php                | 22 ++++++++++++++++++++--
core/modules/package_manager/tests/src/Unit/ExecutableFinderTest.php | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 2 deletions(-)

$ git diff --stat 422099995ecfe78f1e29248374fc05cd4a556c1d^! -- ':!**/tests/**'
core/modules/package_manager/src/ExecutableFinder.php | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)

Notice how the second diff doesn't include the test files and could be used to create a patch that would work.

Lets spin that out to a separate issue. I'm not entirely sure what you mean by "article" tag, so I'd like to discuss that separately. Also because then we can write a more specialised test to demonstrate the issue :D

As someone may currently be relying on users having the edit URL permission in order to control access to the field, I'm hesitant to remove it without a major version bump.

However, we can get there by adding a setting that preserved the behavior for current sites, ensures new behavior for new installations, and provides the necessary deprecation messages.

I'm unable to reproduce this myself. The library that gets installed through composer is goalgorilla/rtseo.js which should be loaded through /libraries/rtseo.js/dist/rtseo.js as part of analytics_core in the module's libraries.yml file.

So if that doesn't seem to work then I'll need more information about your setup to figure out what's wrong.

We've had some challenges specifying the required PHP version, sorry about that. However, this feature was introduced in PHP 8 which is now a minimum requirement for all supported versions of Drupal core, so I'm marking this as won't fix.

Thanks for the research dieterholvoet, I'm closing this as "works as designed" as there's nothing to do in the module and this will need Drupal core to move.

The issue suggests this wasn't an issue in the module. I'd need more info to know if there's a bug to be fixed.

This was fixed in https://git.drupalcode.org/project/yoast_seo/-/commit/70f9ad03fec77180be...

Sounds like a useful addition.

We'd have to add some special values to the widget configuration and make sure those can never be valid theme names. That could be something like reserved:default and reserved:admin since I believe a colon can not be in the theme system name.

Work towards updating the documentation has been started in 🐛 Configuration of bundles not available at /admin/config/yoast_seo Active though never completed. I'll do my best to prioritise that issue. Thanks for bringing this back to my attention.

The move to https://github.com/Kingdutch/drupal-real-time-seo has been completed and test coverage is up and running there :D This also means that it'll now be easier to try and update the underlying JavaScript library.

Sorry for the sloppiness in using language features without declaring a PHP language version. The bit of code relied on a feature that was introduced in PHP 8.1.

As the only supported version of Drupal (10) requires at least PHP 8.1, where this operator is supported, I'm going to close this issue as "Won't fix".

This was fixed in 🐛 Uncaught DOMException: Failed to execute 'remove' on 'DOMTokenList': Active , thanks!

Thanks!

I'm closing this as outdated. There have been a few changes to the module since this issue was opened.

Using the view mode configuration you can now decide exactly which fields are being included in the analysis. Additionally as part of 🐛 The page freezes while the Real-Time SEO module runs script. Needs review , there is now a button that allows you to manually decide when to trigger an analysis.

Those two things should cover the scope of this issue. If that's not the case, please open a new issue.

Thanks for the report! This was fixed as part of fixing the coding standards set-up.

You can see the result in https://git.drupalcode.org/project/yoast_seo/-/commit/01fb9a656d9944465f...

Thanks for the investigation and proposed fix borisr! I've added a line to test coverage to test for the bug and merged your fix.

I'm not entirely comfortable with simply rerunning an update hook. I think one of the important clues that's missing in this issue is: What version did you upgrade from when this happened?

The issue seems to be quite specific and before simply running an update hook twice, I'd like to figure out why it didn't run in the first place.

greggles → credited kingdutch → .

There's a mismatch in breakpoints for the main navigation.

The smallest navigation shows correctly.

There's a small window where some elements have already jumped to their alternative position but the overall styling is still from the other size.

The full navigation displays correctly again as screensize increases.

🎉

Looks good to me, ensures that Upgrade Status can be installed for projects that got ahead of it with PHPStan and looks to work as expected :)

Based on the code changes I'd be happy to RTBC this, but the tests are failing so I think this needs some work to figure out why that is.

dww → credited kingdutch → .

kingdutch → created an issue.

The quickest example I can give you is the pull request for 📌 [PP-1] Ensure asynchronous tasks that run beyond the length of a request have the chance to complete before process exit Active . Other items I'm excited to work on, but they require a bit more puzzling. I hope that in addition I can add more information/rephrase my case and that can hopefully lead to a more concrete understanding (and possibly an updated issue summary).

DrupalKernel is currently a monolith and is responsible for:

1. Setting PHP environment configuration in bootEnvironment
2. Initializing settings and determining which multi-site if any we're in
3. Initializing global variables for application context
4. Handling requests

This worked for Drupal because, for the most part, the only use-case we were considering is the simple request response flow where there is a 1:1 mapping of process to request. However, in modern web applications we're seeing that that no longer holds true. A websocket server may have a long running application that handles many different requests. Similarly Drush might execute Drupal code and not have a request at all for its tasks. These kinds of use cases are discussed in #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole → .

For the adoption of Revolt and enabling of asynchronous applications we must introduce a call to EventLoop::run. By definition, this call must be "global" because in an application there can only be a single EventLoop. The EventLoop has the task to schedule different cooperative tasks and calling EventLoop::run from within an EventLoop::run call causes a fatal error because their scheduling would conflict.

This was discussed in 📌 [PP-1] Ensure asynchronous tasks that run beyond the length of a request have the chance to complete before process exit Active where I tried to find the best possible location. From Drupal's single-request perspective the most logical place would be in the Kernel at the end of handle and the end of terminate. However, this would cause a problem for developers who want to build multi-request applications, or applications in which a Drupal request handler may only be a small part. Their application can not function if EventLoop::run is buried within the Drupal Kernel. The logical conclusion for that then is that the EventLoop::run calls should be placed outside of the Kernel, but this would require it to be placed in index.php, update.php, the related scaffold files. Additionally, anyone that would build their own application that does not use those files would need to remember to include EventLoop::run in the right location for their calls of Drupal to work.

If we look at the list of responsibilities for DrupalKernel then we can see a similar tension for items 1 through 3. They're tasks that should really only happen once and they should happen in a specific manner for Drupal to work. The only consistent place that currently exists in Drupal to satisfy those requirements is DrupalKernel, because if we move them out, then we risk someone not doing the environment initialization separately.

To reduce the amount of responsibilities in DrupalKernel safely, we need a way to extract functionality from it while ensuring that it remains easy for ourselves (as contributors to Drupal Core) but also for application developers who integrate Drupal to properly set-up the environment that Drupal will run in.

We could write that code ourselves but symfony/runtime was created by the Symfony maintainers specifically to solve that problem:

What if we could decouple the bootstrapping logic of our apps from any global state?

This PR makes it possible via a new proposed symfony/runtime component.

The smart thing about the symfony/runtime component, which we would be hard-pressed to replicate ourselves without arriving at the same code, is that it decouples the runtime/global state and application in such a way that you can switch out either side. This puts Drupal in a position where Drupal can provide a runtime that would work in PHP-FPM, as I do in my proposed PR, but it can also provide a runtime that would let Drupal work within a console application, like the drupal CLI tool that is shipped with Drupal core.

Then anyone would be able to write a server or console application that uses Drupal and could use Drupal's out of the box runtime and be sure that their environment is set-up correctly. However, if they have specific needs and want the environment to be set-up in a different way then they can also change the runtime itself, while still use the Drupal PHP-FPM application or console tool.

We can already see an example of the necessity for a consistent environment in the Drupal CLI tool. We see that the InstallCommand and ServerCommand, both have a boot method which goes through steps 1-3 outlined at the start of this comment (and RecipeCommand and RecipeInfoCommand do the same through BootableCommandTrait).

I'm still puzzling a little bit of how to untangle DrupalKernel (though I think it's a worthwhile effort) but I'm confident that we can clean up the code for those commands as well.

1. Move DrupalKernel::bootEnvironment into DrupalRuntime::__construct (different environments can swap out the runtime if needed): 📌 Extract DrupalKernel::bootEnvironment into SAPI adapter Closed: duplicate
2. Move DrupalKernel::findSitePath to DrupalRuntime. This should most likely be requestable as an argument from the main function and passed into the kernel since it's required for the Kernel to boot. That standardises the logic, but allows other tools to easily overwrite it if needed.
3. Application root and initializeRequestGlobals would similarly be moved as per #2529170-104: [PP-1] Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service →
4. By moving the site path detection out of the Kernel we can do the same for Settings::initialize from DrupalKernel::initializeSettings and make the settings available as front-controller argument from our DrupalRuntime:getArgument as well. That would make it trivial to write a Drush-like application that reads settings

I think there's more possible. For example some of the environment is now set based on drupal_valid_test_ua but we may want to switch out the runtime altogether in those. Some of that also requires untangling some existing global state before it becomes clearer.

I realise that's quite a long post which still assumes some knowledge of symfony/runtime. I think the main takeaway is the clear delineation and swapping mechanism that symfony/runtime provides between environment and application.

If environment is E and application is A, then the DrupalKernal is currently (EA), a very tightly coupled environment and application. With symfony/runtime this turns into

E
---- symfony/runtime
A

This makes it easy to change E for E' (in case you have different path requirements around file system structure or front-controller for example), but it also makes it easy to swap A for A' (in case of Drupal console or Drush for example).

The next steps that I'll work on:

1. Expand the PR for this issue to include updating the Drupal console script
2. Work on some of the related issues as stacked MRs to show how they would be implemented on top of the runtime.

Reopening this but postponing on ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active so that that issue can remain small and focused on adding the runtime to Drupal but this issue moves over the actual environment set-up.

quietone → credited kingdutch → .

kingdutch → created an issue.

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.

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.

Forgot to snip a reformatted paragraph, whoops.

Updated the issue summary to help reviewers.

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.

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

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

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.

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

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

kingdutch → created an issue.

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.

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

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.