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:
- Setting PHP environment configuration in
bootEnvironment - Initializing settings and determining which multi-site if any we're in
- Initializing global variables for application context
- 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.
- Move
DrupalKernel::bootEnvironmentintoDrupalRuntime::__construct(different environments can swap out the runtime if needed): 📌 Extract DrupalKernel::bootEnvironment into SAPI adapter Closed: duplicate - Move
DrupalKernel::findSitePathto 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. - 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 →
- By moving the site path detection out of the Kernel we can do the same for
Settings::initializefromDrupalKernel::initializeSettingsand make the settings available as front-controller argument from ourDrupalRuntime:getArgumentas 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.
I've written a change record and addressed the review feedback (except for the deprecation test).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:
- 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:
->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".
@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.
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: trueis 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 → .