🇳🇱Netherlands @kingdutch

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

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

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

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

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

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

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

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

    $suspension = EventLoop::getSuspension();

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

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

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

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

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

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

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

dan2k3k4 → credited kingdutch → .

gábor hojtsy → credited kingdutch → .

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

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

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

Added tests :D

kingdutch → created an issue.

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

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

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

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

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

However, we ensure that we fix the PHPDocs correctly:

interface Foo {

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

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

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

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

Hey everyone!

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

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

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

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

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

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

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

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

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

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

kingdutch → created an issue.

Forgot to update the IS.

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

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

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

Hey kopeboy,

Good question!

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

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

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

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

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

I hope that answers your question!

klausi → credited kingdutch → .

klausi → credited kingdutch → .

klausi → credited kingdutch → .

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

It was accompanied by the following background.

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

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

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

kingdutch → created an issue.

kingdutch → created an issue.

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

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

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

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

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

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

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

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

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

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

Thanks for flagging it!

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

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

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

nicxvan → credited Kingdutch → .

larowlan → credited Kingdutch → .

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

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

Kingdutch → created an issue.

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

quietone → credited Kingdutch → .

Sorry Alexander Allen! Looks like you started off from an older starting point. From a types perspective the MR is already ready to go.

I've updated the issue summary's remaining tasks to better reflect that.

This looks really interesting! Would this also fix the issue discussed in 🐛 Optional configuration from dependence modules is installed before installing module when installing profile Needs work ? That's something we're running into at Open Social. It seems like possibly the following mentions that it does?

Rework \Drupal\Core\Config\ConfigInstaller::checkConfigurationToInstall() and \Drupal\Core\Config\ConfigInstaller::findDefaultConfigWithUnmetDependencies() to work with a list of modules. This has a side effect of fixing an existing bug in the config installer that hook_modules_installed is not called for already installed modules when a module is found that triggers an pre-exisiting configuration exception.

However, the initial "split up in multiple parts" would suggest that we go the opposite direction and make the site installer more different from how individual module installs work rather than bringing the two closer together.

Kingdutch → created an issue.

I'm happy to second quietone's example since it's understandable for people who may have never seen this syntax (you can see $stars is an array).

I initially wasn't sure about not have a space before the first command (proposed [, , $foo] vs my brain's [ , , $foo]) but after writing them out and trying to change them, the proposed spaceless first comma is closest to how it would look if there was a variable, since in that case there would be a variable there and not space. So you can go from [, , $foo] to [$bar, , $foo] without having to remove any characters. That makes sense to me.

zanvidmar → credited Kingdutch → .

zanvidmar → credited Kingdutch → .

I think this is at least tangentially related to #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache → ?

Jean-Paul Vosmeer → credited Kingdutch → .

mikelutz → credited Kingdutch → .

Kingdutch → created an issue.

This is something that can be evaluated using a truth table (using Allowed for TRUE and Forbidden for FALSE).

| Partial Result | A Allowed | A Forbidden |
| B Allowed      | Allowed   | Allowed     |
| B Forbidden    | Allowed   | Forbidden   |

Based on these results we can see that if B is allowed (TRUE) then the result itself is independent of A. So if B is Allowed then we indeed don't need to look at A or A's cacheability. So with that in mind I would expect that if B is forever cacheable and B is allowed, then the result is also forever cacheable; because the result won't change if A changes.

It's important that in case B is Forbidden (FALSE) we do cache based on A's cacheability because it entirely dictates the possible outcome.

Because of the commutative properties of the "or" operation, I would expect this behaviour also if B (endlessly cacheable) was provided first and A (uncacheable) was provided in the orIf.

----------

The above is how I believe, based on the described logic, how Drupal should work. So this would be contradicting your steps to reproduce (where the result should be "cacheable to infinity"). However, I don't quite understand Drupal's comments so I can't comment on whether the current implementation matches the above outline.

I'm always in favour of making working with services easier, especially if it brings us closer to the way Symfony works and reduce WTFs that Symfony (or Laravel) developers may have coming to Drupal.

I do want to voice my opposition for the proposal where other modules can determine what namespaces are made available as services.

Allow a module to define directories within *all* enabled modules that will be used for discovery.

As someone working in projects where we have 300 modules enabled on average this sounds like a potential nightmare where any module can suddenly start exposing classes from any of the other modules. This creates scenario's where my module's behaviour can no longer be evaluated only looking at my own module but must be specifically evaluated within the context of every other possible Drupal module.

The example is to make classes of a specific type available (e.g. CacheTagInvalidator) but whether I expose all the services in my module as cache tag invalidator really should be up to me as module author, not to another module.

Symfony provides us with Service Tags which can provide exactly the desired behaviour. The module that provides the CacheTagInvalidatorInterface can configure the service container to automatically tag services with that interface as cache tag invalidator. Any service that would want all cache tag invalidators could use a Tagged Iterator to collect all those services.

Similarly within my module I may have many more services/components than I want in src/Services. For example if I have sub-functionalities in my module that I want to break up I might have src/EventManagement/{Controller/*.php,ScheduleAvailabilityManager.php} and src/EventRegistration/{Controller/*.php,PaymendHandler.php}. We should be careful not to make assumptions about how module authors may want to lay out their modules, especially as we evolve the capabilities of the service container.

This was shortly discussed on Slack where Bard added

I would imagine attributes aren't currently cached because they weren't considered at the time the system was first built out and/or weren't thought to be something that would materially affect the cacheability of the response. As solutions have matured there's more of a need for this stuff, but the caching layer hasn't caught up

And catch confirmed

None of this particular code has been touched since about 2013, it was all stop gaps around the routing system which was extremely broken at the time.

So in that sense the private file system will likely need attribute caching in 📌 Use attributes instead of query string in PathProcessorFiles and stop replacing query string in RouteProvider::getRouteCollectionForRequest() Active but that can likely happen as part of that issue and I don't see a reason to block this issue itself. We don't currently see a reason why attribute caching wouldn't work.

Happy to leave this RTBC (also after looking at the code) :D

I just want to chime in here that the proposed PathProcessor is exactly what we implemented in Open Social where I found the problems with query on cacheable requests (which is why I assume the private file system didn't suffer) and moved to attributes instead.

However, one issue we're seeing is that there is a difference between the initial uncached request and secondary requests that utilise the page cache. Specifically we're seeing that for the first request the attribute that's set is properly run through Drupal\Core\PathProcessor\PathProcessorDecode but for subsequent requests that doesn't happen. This causes the attribute to be inconsistently decoded or encoded which makes downstream code brittle and can cause errors.

I'm not sure if that's fixed with the MR proposed here (I have to look at the code) but at least wanted to share that issue because maybe someone already knows how this MR might affect that behaviour.

Kingdutch → created an issue.

This was caused by changes made in 🐛 Re-installing the app invalidates the subscription but not the pop-up Fixed which incorrectly interpreted the purpose of pushNotificationPromptTime. Before that issue the code in social_pwa.module would not attach the JavaScript if a prompt result was stored server side for the user. However, this caused the prompt to be shown only on a single device at a time. After the change the JavaScript was always attached. The new logic would only re-show a prompt after [time-since-unix-epoch] which would be very long. However, due to a limitation of setTimeout this wrapped around to 0 which caused the prompt to be shown again immediately.

A caveat to the implementation in 🐛 Re-installing the app invalidates the subscription but not the pop-up Fixed is that platforms require support for localStorage now to be able to support push notifications. With the information we currently have, all push notification enabled platforms also support localStorage.

bbrala → credited Kingdutch → .

quietone → credited Kingdutch → .

I'm unsure why this change seems to break the container magic that's used in the PHPUnit tests :/

It took me some time to come up with a solution to the problem you described :D

I think the proposed solution now also properly handles modules that are installed later. Figuring out a way to find which cache bins are defined by the installed module wasn't trivial but I found that the ModuleInstaller has code for this and if it's good enough for Drupal core's module installer, I figured it was good enough for us.

ronaldtebrake → credited Kingdutch → .

Kingdutch → created an issue.

Normally dropping Drupal 8 would be a breaking change, but I think it's been unsupported long enough that that's fine :D

0️⃣ Who is here today? Comment in the thread to introduce yourself. We’ll keep the meeting open for 24 hours to allow for all time zones.

1️⃣ What topics do you want to discuss? Post in this thread and we’ll open threads for them as appropriate.

2️⃣ Action items

2️⃣.:1️⃣ Approve minutes for previous meeting(s)

3️⃣ Fixed since last meeting

4️⃣ RTBC issues

4️⃣.1️⃣ #3295249: Allow multi-line function declarations →  (edited) 

4️⃣ .2️⃣ #3339746: Decide on a coding style for PHP Enumerations →  (edited) 

4️⃣.3️⃣ #3074131: Use null coalescing operator ?? instead of a ternary operator with an isset() condition →  (edited) 

4️⃣.4️⃣ #3324368: Update CSS coding standards to include PostCSS and Drupal 10 →

5️⃣ New issues

5️⃣ . 1️⃣  #3439004: Coding standard for attributes →

5️⃣ . 2️⃣ New activity on needs review issue 📌 Coding standards for "use" statements RTBC

6️⃣ via @Jonathan1055 suggesting updates to the project pageItem for discussion, didn't want to raise a separate issue for it. Suggested update to the project page, see attached screen grab.Needs an line explaining what the links areCan the links be a in a larger font? they are major important parts of the project, but do not stand outThe one line of text about PHP could be moved to the linked page. It is odd that PHP has some extra words here, but the other sections/links do not. Better to have just clean links here, with the words on the linked pages.

7️⃣ via @quietone #3099562: [Policy] Consider creating coding standards depended on specific PHP versions →

8️⃣ via @quietone #2689243: Import classes used in annotations →

9️⃣ via me :smile: #3360160: Stop using FQCN in PHPDoc annotations →

Participants:

Kingdutch, larowlan, Alex Skrypnyk, quietone, catch, urvashi_vora, bbrala, kim.pepper, dww, borisson_, Jonathan1055, mstrelan

I realize this may be out of scope but #2813895: Combine entity type labels in an array → has been open for a while and would be a breaking change in any existing system. However the conversion to attributes might be the perfect time to make the grouping since people will have to re-write their annotations anyway (so it's not more or less breaking than what we're already doing).

I only just came across that issue so sorry if this is a bit late in the process.

The relevant PHPStorm issue to get this fixed for IDE users is https://youtrack.jetbrains.com/issue/WI-26078/Automatically-added-use-st...

Moved into new coding standards template and added myself as supporter.

Generalised the title to PHPDoc annotations so we don't need to list all @ annotations that it may support.

Removed the returnEarly function from the documentation proposal since it distracted from what the docs are focusing on which is the naming convention of the cases.

If we did accept a larger baseline and bump the level, do you think it would make sense to then take a horizontal approach instead of a vertical approach to reducing the baseline size? e.g. Working by rows in your table, instead of by columns (which is what we're doing now).

It does feel like a lot of those rows would be easy wins but also with a high return. There are definitely some rows where reward vs effort vs disruption would definitely be difficult.

Yeah I think that would absolutely be the way to go. I do think there's a challenge that they may not be simple find/replace, so I'm not sure how large we can make batches. For example we may introduce a return type in the PHPDoc of an interface and then fix our classes to adhere to them (where we can introduce it in PHP). That in turn may cause PHPStan to say "Hey these type assertions you're making here are no longer needed" and we can remove those. So there's connection between the different rules.

The benefit is that the higher level of PHPStan can give us the confidence that if the types are correct at a local level (e.g. we make sure to accept string|\Stringable) they'll also help us out on the project level. That should be a big help to reviewers for these kinds of issues even if they're not copy/paste.

Or, another idea - add a level 9 job to core's CI pipeline, but allow it to fail. That will at least allow us to know if we're introducing new errors, whilst not blocking on preexisting problems.

I saw the suggestion and like it in theory but I'm struggling with a viable way of making it work. One of the differences with PHPStan vs tools like CSpell/PHPCS is that we always want to run it on the entire codebase because changes in one file can result in behaviour changes in any other file. So with that in mind I think this route would still require us to commit and update the baseline in case of errors, but there'd be a discussion of which errors are then acceptable.

If we don't commit (or update) the baseline then it's going to be really difficult to say what part of the code caused the new errors and what errors were introduced in previous commits. I think that probably leads to more frustration.

The only way I can come up with to do this and not commit the baseline would be to generate the baseline from the base commit of the PR and then run it against HEAD of the PR, so only the change in errors would be reported over which reviewers could make a decision of whether it's acceptable or not. So basically run PHPStan twice in the CI. The downside of this is that it does make people reliant on the CI for their PHPStan work and does not allow to easily perform these checks locally (unless they also run it twice locally). A composer install between runs might be needed because updated dependencies can affect type information.

Thanks for the input everyone! And thanks alexpott and phenoxproxima for sharing your experiences with level 9 in the recipe fork.

Responding to the recipe example

If I look at the commits it's a bit difficult for me to see where exactly the friction is coming from (don't get me wrong, I do see the friction). There's two points I think I could summarize it as (and please correct me if I'm wrong): firstly a learning curve, secondly Drupal's use of arrays.

The learning curve is very understandable and I do think it's something we'll have to guide the Drupal community through. We're essentially going from a very loosely "do what you want" language to a very strictly typed language. Adding @var annotations or preferably assert statements (since they fail your test if your assumption is wrong) is a part of the journey we'll have to take. Regardless of our strategy with PHPStan levels, that's something we'll have to go through at some point. The good news is that PHPStan will tell us if asserts are wrong (and thanks to Drupal using Bleeding Edge will also tell us if @var statements are likely wrong). That provides us with a way to fix issues at the call site ("I know this is type X") while allowing us to fix the origin of the bug (the actual type annotation) and then have PHPStan tell us which call-sites no longer need the asserts/annotations.

The second thing is Drupal's use of arrays. This is the only thing that ended up in the baseline. It's probably the hardest thing to fix in Drupal regardless of levels because it'll require re-thinking how we pass around our render data and define a more well-defined structure for it. This is exactly why this issue proposes a singular exception to the strict rules of PHPStan (from the issue summary):

Exceptions added
In order to not frustrate Drupal's use of render arrays and instead allow people to slowly start adding types to those we ignore the error ... has no value type specified in iterable type array. We do not disable the type requirement for all iterable types because specifying them does have a lot of value outside of render arrays in improving the type-safety of loops in code.

We do not disable generics altogether because there's a lot of places (like Drupal's Entity API which is now partially patched by PHPStan Drupal) where generics can really add value in telling downstream code what type of value exists within a container. Introducing generics without immediately validating them with PHPStan is likely to cause us even greater pain in the future, hence why they're only disable for arrays and we keep them for other classes (where assertions/@var annotations can more easily be written if needed).

Why running PHPStan on a lower level is a fallacy

It's a bit challenging that PHPStan called its configuration steps "levels" because they make people think of them like "levels in a video game" where if you beat one level, the next one becomes easier. However, that's not really the case for PHPStan. I personally see them more as "groupings of complexity" (although depending on your code-base some higher levels might be easier than lower levels).

The challenge with Drupal's current approach is that while we're slowly fixing some issues at the lower levels, we're steadily introducing new things to fix at higher levels which are just sitting there waiting. The value across the levels also isn't equal. For example we all agree that type-hints are incredibly useful but these aren't checked until level 6 (that's the same level that introduces the generics that many people stumble over). It's not until level 8 that PHPStan will help us out in preventing "calling some method on NULL" which are errors that, if they slip through tests, will case a white screen of death for a site. The only difference between level 8 and 9 is PHPStan being strict about "mixed" which is a good thing because without that rule people might see "mixed" as a solution over finding the more specific union they're probably actually interested in. PHP's mixed is akin to TypeScript's any, which the TypeScript community has long since learned is dangerous. To remedy that they've introduced unknown which changes any from "could by anything" to "you must figure out what this is", that's basically all PHPStan's level 9 does over level 8.

Some statistics

To back-up my claim that even though we're slowly chipping away at lower levels, we're introducing issues at higher PHPStan levels at a higher rate that we'll want to fix in some future, I've analysed ~2100 commits from 10.1.x until 11.x (around the end of March) for Level 1 and Level 9 of PHPStan and plotted their difference. This is based on the analysis script that I shared in this issue earlier but with all errors grouped into a specific category/explanation.

The scripts used to do this as well as the raw result data can be found in https://github.com/Kingdutch/analyse-drupal-phpstan

As we can see we're slowly chipping away at the 600 errors that are in our current baseline. Meanwhile we're introducing about 2 errors per commit on average, which dwarfs the work that we're doing at lower levels as we climb from about 55000 errors to nearly 60000 errrors.

This is to me what spurs my motivation to try and change our approach to how we tackle code quality since I believe the benefits that preventing the errors shown by PHPStan can bring us truly outweight the effort needed to prevent them.

In an attempt to make discussing the different levels of PHPStan based on data rather than my own or others' feelings I've analysed commit ec28f25d1e on the 11.x branch at level 1 through 9 and ran the results through the analysis script. The resulted categorisation is listed in a table below to show which errors and how many of them there are for the different levels of PHPStan.

Based on the table above I would love to hear the value people assign to the different categorised errors and whether it's worth preventing them. For example I value preventing null-based bugs because I've seen them as errors a lot and similarly believe that "mixed" is not a good type to use in 90% of circumstances. However if people disagree with me that might give us a more data and decision driven approach to finding which level we should land on going forward. Additionally if that's not 9 it may give us some ideas of what criteria we want to adhere to to move to higher levels in the future and how to ensure the higher levels don't grow as we fix lower level issues.

I think since Enum's are very class-like, this makes the most sense directly after Classes: https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... →

While I'm happy that a lot of debate has sparked about how we can already add types to existing interfaces, the prevailing thoughts still seems to be "We should fix types first and then we can raise PHPStan".

I personally wouldn't feel comfortable even adding the types to existing interfaces, especially not in a large MR until we've verified that the PHPDoc types for the interfaces are correct, that is something that PHPStan can help us with.

For example in the canary MR the following change was made (unchanged PHPDoc added for needed context):

   /**
    * Creates new handler instance.
    *
    * Usually \Drupal\Core\Entity\EntityTypeManagerInterface::getHandler() is
    * preferred since that method has additional checking that the class exists
    * and has static caches.
    *
    * @param mixed $class
    *   The handler class to instantiate.
    * @param \Drupal\Core\Entity\EntityTypeInterface $definition
    *   The entity type definition.
    *
    * @return object
    *   A handler instance.
    */
-  public function createHandlerInstance($class, EntityTypeInterface $definition = NULL);
+  public function createHandlerInstance(string $class, EntityTypeInterface $definition = NULL);

PHPStan (at a sufficient level) would complain about the mismatch between string in the code and mixed in the PHPDoc.

The implementation of EntityTypeManager looks as follows:

    if (is_subclass_of($class, 'Drupal\Core\Entity\EntityHandlerInterface')) {
      $handler = $class::createInstance($this->container, $definition);
    }
    else {
      $handler = new $class($definition);
    }
    if (method_exists($handler, 'setModuleHandler')) {
      $handler->setModuleHandler($this->moduleHandler);
    }
    if (method_exists($handler, 'setStringTranslation')) {
      $handler->setStringTranslation($this->stringTranslation);
    }

    return $handler;

is_subclass_of can take a class instance but also a class string. The preferable input is likely to be a class-string (since a class instance wouldn't actually be used). So a decision has to be made here on an interface level what the intended inputs are. Most likely this is a class-string for an EntityHandlerInterface. That provides EntityTypeManager with a deprecation path forward for its else statement that other classes could follow.

Using the expressiveness of PHPStan's extended types in the PHPDoc and PHPs more limited type system in the PHPCode, unless we want to make a conscious change of how the entity type manager works, the interface probably should be changed to:

  /**
   * Creates new handler instance.
   *
   * Usually \Drupal\Core\Entity\EntityTypeManagerInterface::getHandler() is
   * preferred since that method has additional checking that the class exists
   * and has static caches.
   *
   * @param class-string<\Drupal\Core\Entity\EntityHandlerInterface> $class
   *   The handler class to instantiate.
   * @param \Drupal\Core\Entity\EntityTypeInterface $definition
   *   The entity type definition.
   *
   * @return object
   *   A handler instance.
   */
  public function createHandlerInstance(string $class, EntityTypeInterface $definition = NULL);

(Even while writing this I had originally added \Drupal\Core\Entity\EntityHandlerInterface instances as valid type because the current code would work when it's provided a handler instance; however the code likely doesn't make sense in that way. That points me to the requirement for human judgement with an automated system likely encoding unwanted types that require more breaking changes later).

I'm all in favor of starting to add better type annotations to existing interface arguments, regardless of our decision on PHPStan level :D However, I do think the above demonstrates unfortunately we probably need to do so in a non-automated fashion in reviewable chunks.

With that said, since this is an issue close to my heart, I'm happy to volunteer as a first reviewer for anyone that sets up a PR like the above (easiest to ping me in the Drupal Slack).

Change makes sense to me :D This way PHPStan is happy on higher levels. The proposed return type matches the return type of the logger.

Kingdutch → created an issue.

I've rebased the code on top of 11.x and re-applied the changes from 📌 Add revoltphp/event-loop dependency to core Active and ✨ Implement a Result type in Drupal Core Needs review so that their code is up-to-date according to the reviews from those issues.

By larowlan

I feel strongly either way about the use of namespaced functions. I would like to see some more documentation here of examples of what operations might be.

e.g. some async, some delayed, some deferred

I'm not entirely sure what examples to provide. Delayed and Deferred functions are examples of async functions. The semantics of what is "delayed" vs "deferred" are those matching with the Revolt Event Loop.

There is no hard requirement that an operation is actually asynchronous either. In case the operations themselves are actually synchronous then the form of stream (by scheduling with EventLoop::defer) ensures that other operations can still run in between synchronous operations as needed. In case the operations themselves suspend then thanks to the event loop this automatically provides places for other work to occur. The important part for stream is that we want to process tasks that are done as soon as possible, so that they may be out of order if a task started first takes longer.

I created an example of this in kingdutch/revolt-playground github repository. One way to show what would happen for synchronous operations would be to extend that example with placeHolderSync that uses sleep for a certain period of time, rather than using the EventLoop::delay function.

I hope that example can help clarify of what kind of additional documentation you'd like to see :D

I've applied the suggested changes to the documentation and removed the sentence about the list example, instead showing it as code which I hope helps it more easily click with developers.

Rebased the MR on top of the 47 new commits on the 11.x branch. Also added the baseline file to CSpell as requested in #37 and #40.

I ran the same analysis script I attached in #29 to give an indication of how many new errors (according to the higher rule level) were added in those 47 commits. I've attached the analysis output and manually added (+x) increase, (-x) decrease, or (~) equal indicators for each rule.

Analysing 59398 (+141) ignored PHPStan errors
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
..................................

2028 (+2) functions without a return type specified.
533 (~) methods on interfaces without a return type specified.
19091 (+27) methods on other classes without a return type specified.
171 (+2) unreachable pieces of code.
105 (-1) if-statements that will always be TRUE.
3 (~) if-statements that will always be FALSE.
60 (+1) partial conditions always TRUE/FALSE.
346 (~) undefined property accesses on an object.
642 (~) undefined property accesses on other things.
4110 (+25) method calls on values that may be NULL.
413 (~) method calls on values that may be FALSE.
1034 (+2) property access on values that may be NULL.
186 (~) property access on values that may be FALSE.
1487 (-1) values returned that don't match the return type.
427 (~) invalid types supplied to foreach.
69 (~) binary operations resulting in error.
3650 (+6) calls to undefined method (usually value is not of specific enough type).
2263 (+3) unsupported operation on mixed type.
19 (-2) calls to undefined function/method.
72 (~) calls to assert which isn't needed.
11 (~) calls to is_array which are always FALSE.
33 (~) calls to is_array which are always TRUE.
2 (~) unnecessary calls to is_bool.
2 (~) unnecessary calls to is_callable.
2 (~) unnecessary calls to is_int.
13 (~) unnecessary calls to is_null.
5 (~) unnecessary calls to is_numeric.
4 (~) unnecessary calls to is_object.
1 (~) unnecessary calls to is_resource.
30 (~) unnecessary calls to is_string.
3 (~) unnecessary calls to is_subclass_of.
48 (+1) unnecessary calls to method_exists.
5 (~) unnecessary calls to property_exists.
573 (+3) unnecessary PHPUnit Assert.
1287 (+2) functions with missing parameter type.
4987 (+20) method with missing parameter type.
229 (~) other statements are always TRUE.
391 (+3) other statements are always FALSE.
656 (+2) drupalLogin calls may receive FALSE.
6990 (+26) incorrect type of parameter provided for method call.

7417 (+10) ignored errors without classification.

Actually, the needs review bot and open MR should tell us how often there are conflicts or other issues, right? For that concern.

I'm not entirely sure. A merge conflict occurs when two parallel changes touch the same lines of code and git is not able to determine how to reconcile those. This MR makes + 295436/− 1079 changes to the baseline (since we include all currently known possible errors), so it essentially touches every line in the existing baseline and any other change will have a conflict with it once.

The normal number of changes in the baseline are much lower than 300k lines. With that in mind, the chance of two MRs touching the same lines of code in the large baseline go down significantly.

This is true for the _current_ file, but cloning with no depth limit will still pull in all the prior revisions. Not really gonna change anything, but unless we rewrite history or people routinely clone without depth limit (could be an idea for CI if it isn't already) then this is "here to stay" once it happens.

Unless I've missed something the only open concern (besides the potential for merge conflicts) is that because of the way git works (it captures history) the increase to the repository size with the new baseline size would be permanent for the lifetime of the project. This would affect clones which do not limit depth (by default clones fetch all history depth); we have taken steps to ensure this doesn't affect production deployments which don't use git.

I don't have a good solution for this and with access to 4G/5G and fiber in all my workplaces I think I might be slightly too privileged to have a proper say in this matter. Git has improved support for very large repos (measured in Gigabytes, so much larger than where Drupal is at) but these do require steps taken on the consumer side and are not something we can do from the repository side of things.

1. PDO doesn't have any async support, we have an issue to add a mysqli driver to core, specifically to enable this issue. So we need a non-async code path for those drivers based on a capability check.

I feel like this is missing the beauty of Fiber's in our mental model :D The whole point of Fibers is that we can have things that are async which look exactly the same as sync code: it solves the "What color is your function" problem (see "What problem do fibers solve?" in Christian Lück's great post.

So where we should end up (and if we don't we did something wrong) is that code calling Entity::loadMultiple($ids) shouldn't care how we load the data (whether that's sync or async) because thanks to Fibers the code writing it can be sure that even if the loading itself happens asynchronously (and other things are done in the meantime) the code in this specific place won't run past Entity::loadMultiple($ids); until those entities have loaded.

This means that to execute an async query, we need to open an additional MySQL connection each time, or re-use a connection from which a query has previously returned.

Especially with database caching, but also just in general, Drupal can easily execute dozens or hundreds of small database queries on each page request for authenticated users (or just on a cache miss). I think we would need/want to avoid a situation where we have say more than 5-10 connections open per request, since in a stampede we could end up hitting 'too many connections' limits. As soon as we have whatever limit of connections open (say 5) we'd be unable to execute any more until one returns, and we might not have anything else to do except issue more database queries. However, if we only execute async queries for longer database queries (entity queries and views etc.), then all the other ones can be handled by the main, non-async connection (whether that's using the mysqli driver or PDO) even if there are five, slow, async queries running.

The other reason is that some database queries (again, especially with the db cache but not only) are directly blocking the rest of the request, including in situations where most caches will be warm, something like a key value query, cache tag checksums, path alias, or route lookups. In those cases, we want those (usually sub 1ms) queries to finish as soon as possible so we can move onto the next blocking thing, and wouldn't want to go back into the event loop to do prewarming or whatever in between.

I think in the form of a ConnectionPool this is a problem that's long since been solved in other programming languages and we shouldn't re-invent the wheel here. I also don't think we should differentiate between the type of query (because the workload is inherently unpredictable) and all queries should go through the connection pool; especially since there's no query that shouldn't happen.

From a caller's perspective I don't even think we should know about the ConnectionPool, that should be the database driver's responsibility. Calling code just makes a database call and that'll suspend until data is available and will resume when data is present. Whether that suspension is because there's no connection free or whether the connection is waiting for data from a different connection shouldn't matter.

For scheduling what code actually gets to load data the EventLoop helps us with priorities based on how things are added to it: EventLoop::queue happens immediately when the eventloop gets controlled; EventLoop::defer happens first in the next tick; EventLoop::delay and EventLoop::repeat happen only after deferred callbacks have run. Responses to streams are also treated as higher priority as far as I understand.

Updated the issue summary with the remaining tasks to show tasks in progress. At least with the current proposed implementations it appears no work for PHPUnit is needed. If tests want to test something specifically that doesn't block the main thread at some point then they'll have to run EventLoop::run() in the test themselves.

I think with the learnings from 📌 [PP-2] Migrate BigPipe and Renderer fiber implementation to use Revolt Event Loop Postponed I'd propose a different approach:

All database queries should always be async. This is possible with Fibers (and the Revolt Event Loop) because the code that requires the result of the database query will not be executed until the result is returned. This ensures that while any database query is happening, unrelated code can run (e.g. a cache prewarmer or another bigpipe task). In case a piece of code needs to make multiple database queries and wants to do them at the same time then a primitive such as stream or concurrently can be used to control how they're executed. That would lead to similar patterns as implemented in the BigPipe changes.

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

Kingdutch → created an issue.

https://github.com/goalgorilla/open_social/pull/3813

Crediting slowflyer for reporting the matter :)

Kingdutch → created an issue. See original summary → .

I did some local experimentation with PHPStorm by opening it on the 11.x branch. This allows PHPStorm to settle on 2,23GB of memory used according to the activity monitor. I then switched to the 3426047-bump-phpstan-to-level-9 branch for this issue. The indexing time that PHPStorm needed was near instant (basically a progress bar flash) and memory changed to 2.63GB. I'm not sure how scientific this is and whether all 0.4GB of memory pressure can be attributed to the baseline or whether PHPStorm does other things when switching branches that require memory.

I added ".phpstan-baseline.php" to "Exclude files" under "Settings > Directories" and retried the experiment. I opened PHPStorm on the 11.x branch which settled at 2.25 GB. This time no re-indexing seemed to be triggered and memory usage changed to 2.36GB.

The other issue landed! Marking this as "Needs work" because it needs a rebase.

In #3426548-21: Convert the PHPStan baseline from NEON to PHP → mstrelan mentioned

Since this file is php do IDEs like PhpStorm try to analyse it? It can obviously be excluded but does that involve manual steps?

This needs some more investigation to impact of the large file. My initial response was:

I can not answer for all IDEs but only for PHPStorm. PHPStorm does seem to index the file. As far as I found there's nothing we can do against this with things we ship in the repo. I also don't know if it's a problem (i.e. in what ways it affects PHPStorm usage).

There are settings in PHPStorm that allow excluding files which are global settings that apply to all projects. So in case the analysis of the file by PHPStorm causes problems for someone it's a one-time fix.

longwave's feedback from #33 was previously addressed in the commit between #30 and #31 assuming the packaging works how we thought it did on Slack.

#26 crossposted with #25. Moving back to needs review after change in core/scripts/dev/commit-code-check.sh

Moving this back to RTBC as per #23.

#24 is a race condition with a mistake for 6996 which was in my push between #19 and #20 but fixed between #21 and #22. GitLab reports that 6996 is fully mergeable at the moment (contrary to the review bot)

Since this file is php do IDEs like PhpStorm try to analyse it? It can obviously be excluded but does that involve manual steps?

I can not answer for all IDEs but only for PHPStorm. PHPStorm does seem to index the file. As far as I found there's nothing we can do against this with things we ship in the repo. I also don't know if it's a problem (i.e. in what ways it affects PHPStorm usage).

There are settings in PHPStorm that allow excluding files which are global settings that apply to all projects. So in case the analysis of the file by PHPStorm causes problems for someone it's a one-time fix.

Rebased the three branches :D The diff in the new format for the changes on the 10.3.x branch was very readable 🤩

diff --git a/core/.phpstan-baseline.php b/core/.phpstan-baseline.php
index a42a313689..664664da34 100644
--- a/core/.phpstan-baseline.php
+++ b/core/.phpstan-baseline.php
@@ -49,7 +49,9 @@
 	'path' => __DIR__ . '/includes/theme.maintenance.inc',
 ];
 $ignoreErrors[] = [
-	'message' => '#^Call to an undefined static method Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:\\:registerLoader\\(\\)\\.$#',
+	'message' => '#^Call to deprecated method registerLoader\\(\\) of class Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:
+This method is deprecated and will be removed in
+            doctrine/annotations 2\\.0\\. Annotations will be autoloaded in 2\\.0\\.$#',
 	'count' => 1,
 	'path' => __DIR__ . '/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php',
 ];
@@ -1356,7 +1358,9 @@
 	'path' => __DIR__ . '/modules/migrate/src/MigrateException.php',
 ];
 $ignoreErrors[] = [
-	'message' => '#^Call to an undefined static method Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:\\:registerLoader\\(\\)\\.$#',
+	'message' => '#^Call to deprecated method registerLoader\\(\\) of class Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:
+This method is deprecated and will be removed in
+            doctrine/annotations 2\\.0\\. Annotations will be autoloaded in 2\\.0\\.$#',
 	'count' => 1,
 	'path' => __DIR__ . '/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php',
 ];
@@ -2380,12 +2384,6 @@
 	'count' => 1,
 	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Config/ConfigInstallTest.php',
 ];
-$ignoreErrors[] = [
-	'message' => '#^Call to deprecated method expectError\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
-https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#',
-	'count' => 2,
-	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php',
-];
 $ignoreErrors[] = [
 	'message' => '#^Variable \\$expected_driver might not be defined\\.$#',
 	'count' => 2,
@@ -2443,12 +2441,6 @@
 	'count' => 1,
 	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php',
 ];
-$ignoreErrors[] = [
-	'message' => '#^Call to deprecated method expectErrorMessage\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
-https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#',
-	'count' => 1,
-	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Render/Element/MachineNameTest.php',
-];
 $ignoreErrors[] = [
 	'message' => '#^Variable \\$value in isset\\(\\) always exists and is not nullable\\.$#',
 	'count' => 1,
@@ -2471,7 +2463,9 @@
 	'path' => __DIR__ . '/tests/Drupal/Tests/BrowserTestBase.php',
 ];
 $ignoreErrors[] = [
-	'message' => '#^Call to an undefined static method Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:\\:registerAutoloadNamespace\\(\\)\\.$#',
+	'message' => '#^Call to deprecated method registerAutoloadNamespace\\(\\) of class Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:
+This method is deprecated and will be removed in
+            doctrine/annotations 2\\.0\\. Annotations will be autoloaded in 2\\.0\\.$#',
 	'count' => 1,
 	'path' => __DIR__ . '/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php',
 ];