🇳🇱Netherlands @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 #1624564: Coding standards for "use" statements →

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',
 ];

I don't think modifying the baseline further or moving it out of the repository is a good idea. It would make the development experience of running PHPStan locally (and updating the baseline) too cumbersome.

While I do understand that 15 MB is a lot on the scale that Drupal is operating, we should also place the size of the file in perspective. I think our primary goal should be that people who deploy Drupal to production shouldn't suffer its cost. For that we have a good proposal which I've committed to the branch (although it does need testing which I can not do against a dev branch in a fork). I believe that as long as the file is not bundled in the archives on Drupal.org or in the dist version of Composer that's on Packagist then we've achieved that goal.

It's unrealistic to try and save the same 15MB when dealing with the raw source repositories (e.g. a git clone or --prefer-source composer install) because those are likely scenarios in which you want to have the baseline present. For an initial clone of drupal/drupal the new baseline file would represent a 7% increase in repo size; for an inital clone of drupal/core the new baseline file would represent an 8% increase in repo size (counting the existing baseline as 0-size in both cases).

However that size would only be downloaded once, since it's a text file that git can efficiently update rather than having to re-download it on every pull request.

Additionally we know that this is a file where our aim is to make it smaller quickly (because we'd be addressing real issues and improving the developer experience of anyone using Drupal) and it's not a file that will stay this big indefinitely.

Fresh git clones of both drupal/drupal and drupal/core to show the size of the repo's inital clone for comparison to the increase caused by the baseline.

$ git clone git@git.drupal.org:project/drupal.git
Cloning into 'drupal'...
remote: Enumerating objects: 967424, done.
remote: Counting objects: 100% (12915/12915), done.
remote: Compressing objects: 100% (2246/2246), done.
remote: Total 967424 (delta 11040), reused 10823 (delta 10660), pack-reused 954509
Receiving objects: 100% (967424/967424), 230.15 MiB | 4.31 MiB/s, done.
Resolving deltas: 100% (699506/699506), done.
Updating files: 100% (17353/17353), done.

$ git clone git@github.com:drupal/core.git
Cloning into 'core'...
remote: Enumerating objects: 767457, done.
remote: Counting objects: 100% (19190/19190), done.
remote: Compressing objects: 100% (8354/8354), done.
remote: Total 767457 (delta 10741), reused 18556 (delta 10149), pack-reused 748267
Receiving objects: 100% (767457/767457), 186.46 MiB | 13.95 MiB/s, done.
Resolving deltas: 100% (542285/542285), done.
Updating files: 100% (17264/17264), done.

Adjusted the script to gain a bit more insights at level 9 :)

The lines that say "always" or "unnecessary" is where PHPStan allows us to remove code :D
Operations on "may be NULL" or "may be FALSE" are code-paths that may manifest in bugs.

Analysing 59257 ignored PHPStan errors
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
.................................

2026 functions without a return type specified.
533 methods on interfaces without a return type specified.
19064 methods on other classes without a return type specified.
169 unreachable pieces of code.
106 if-statements that will always be TRUE.
3 if-statements that will always be FALSE.
59 partial conditions always TRUE/FALSE.
346 undefined property accesses on an object.
642 undefined property accesses on other things.
4085 method calls on values that may be NULL.
413 method calls on values that may be FALSE.
1032 property access on values that may be NULL.
186 property access on values that may be FALSE.
1478 values returned that don't match the return type.
427 invalid types supplied to foreach.
69 binary operations resulting in error.
3644 calls to undefined method (usually value is not of specific enough type).
2260 unsupported operation on mixed type.
21 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.
47 unnecessary calls to method_exists.
5 unnecessary calls to property_exists.
570 unnecessary PHPUnit Assert.
1285 functions with missing parameter type.
4967 method with missing parameter type.
229 other statements are always TRUE.
388 other statements are always FALSE.
654 drupalLogin calls may receive FALSE.
6964 incorrect type of parameter provided for method call.

7407 ignored errors without classification.

I made a small script (attached as txt file) that allows us to use the PHP baseline to get some insight into what type of errors we're dealing with, which can be a bit more precise than my previous estimates. I still believe that we should embrace the baseline to make sure we don't increase the count of errors we're introducing in new code (and a lower PHPStan level simply isn't going to do that).

However, I am open to seeing if we can get better insight into the contents of the baseline and see if we can resolve some issues using PHPDoc annotations in parallel. I'd also be willing to invest some time in that if that helps acceptance of a baseline size and this issue landing sooner.

Here's the output of the current iteration of the script. I'd love to know if there's a preferred way of tackling these (e.g. I could add all return types to Drupal's functions as I expect them to be relatively straightforward, but that might be a patch with 2000 changed lines.

Analysing 59257 ignored PHPStan errors
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
.................................

2026 functions without a return type specified.
533 methods on interfaces without a return type specified.
19064 methods on other classes without a return type specified.
169 unreachable pieces of code.
106 if-statements that will always be TRUE.
3 if-statements that will always be FALSE.
59 partial conditions always TRUE/FALSE.
346 undefined property accesses on an object.
642 undefined property access on other things.

36309 ignored errors without classification.

There was a thread in Slack about the issue raised in #24

I think there needs to be a conversation about adding a 15 megabyte file to the core directory and at the very least decent instructions about how to not deploy this to your site.

drumm mentioned "Composer does no archiving, its just automating downloading zip files from from GitHub & ftp.drupal.org, and git clone git.drupalcode.org & elsewhere". That means that the proposed solution by Mglaman of adding the following to .gitattributes under core/ would ensure the file is not distributed through composer or Drupal.org archives.

/.gitattributes export-ignore
/.phpstan-baseline.php

This does still let the baseline be downloaded when people install with composer using the --prefer-source flag which I would say is proper behaviour since the baseline is part of the drupal/core source.

11.x branch updated. Also updated the documentation and CR to match.

Created 10.2.x and 10.3.x branches with MRs :)

The baseline name has been changed to be a dot-file, the copy-paste instructions have been adjusted accordingly.

I've created the following documentation page which outlines how PHPStan is used in Drupal core: https://www.drupal.org/docs/develop/development-tools/phpstan/phpstan-in... → . I attempted to write it in a way so that it's not outdated when ✨ [PP-1] Bump PHPStan to level 9 and accept a large baseline Postponed lands.

I've added a reference to the new documentation in the CR for this issue: https://www.drupal.org/node/3426891 →

I've updated the previous CR that andypost linked to also reference the CR for this issue: https://www.drupal.org/node/3258232 → . The formatting may need some work, I'm not sure if there's a best practice for those kinds of references.

Using the following code to time analysis of bd4b8b98a7 (the commit in the MR before the change) and the change itself.

Full analysis with baseline in place and without generating a new baseline

vendor/bin/phpstan clear-result-cache --memory-limit=-1 && time vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1

Full analysis without baseline in place and generating the baseline from scratch (neon/php used depending on commit tested).

# neon
vendor/bin/phpstan clear-result-cache --memory-limit=-1 && rm core/phpstan-baseline.neon && touch core/phpstan-baseline.neon && time vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/phpstan-baseline.neon

# php
vendor/bin/phpstan clear-result-cache --memory-limit=-1 && rm -f core/phpstan-baseline.php && echo "<?php return [];" > core/phpstan-baseline.php && time vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/phpstan-baseline.php

Three runs each (m:ss):

• (neon) bd4b8b98a7 full analysis without generating - 1:58/1:55/2:00
• (neon) bd4b8b98a7 full analysis generating baseline - 1:52/1:50/1:37
• (php) 3426548-convert-the-phpstan full analysis without generating - 1:41/1:41/1:33
• (php) 3426548-convert-the-phpstan full analysis generating baseline - 1:31/1:38/1:33

A draft CR has been created. I'm not sure which docs would need to be updated.

Thanks for sharing your concern!

Without a specific example of the problematic GraphQL stack-trace it's hard to comment on the specific issue (perhaps sharing this in the GraphQL issue queue if you haven't done so already can help us find improvements there). For now I'm going to assume that the trouble you experience with GraphQL is specifically because we build the schema in one place and then execute against the schema later in another place. While doing that we use a user-land promise style which might complicate this a lot.

There's also the fact that Drupal's exception handler delegates to Error::decodeException which only handles the top level exception and truncates any nested exceptions

For example the following code in a PHP sandbox

try {
    throw new \RuntimeException("Foo");
}
catch (\Exception $e) {
    throw new \Exception("Exception thrown", 0, $e);
}

Properly shows both stack-traces

Fatal error: Uncaught RuntimeException: Foo in /in/Q5PRm:4
Stack trace:
#0 {main}

Next Exception: Exception thrown in /in/Q5PRm:7
Stack trace:
#0 {main}
  thrown in /in/Q5PRm on line 7

Process exited with code 255.

That same implementation in Drupal would only log the \Exception which is thrown in the catch block and doesn't actually contain the useful information. Ensuring that Drupal prints the entire chain of exceptions would be a big developer experience win and might solve (some of) your debugging issues.

With those two things in mind there's two changes for the implementation that we're discussing here.

Firstly I made sure not to wrap any caught exception (or exception contained in the Error result) so that Drupal doesn't hide the origin of the exception. Instead I throw them as-is.

Secondly the implementation we use here is built on Fibers which doesn't suffer the problem that GraphQL's promise-based implementation might have because PHP itself keeps track of where the work was postponed and can thus also restore the callstack. I created a small reproduction of what an exception would look like for the bigpipe code in my Revolt Playground. The resulting stacktrace is copied below and I've prepended numbers so I can annotate them below.

[1] at revolt-playground/src/Demo/Stacktrace/Command.php:43
[2] Kingdutch\RevoltPlayground\Demo\Stacktrace\Command->throwException() at revolt-playground/src/Demo/Stacktrace/Command.php:25
[3] Kingdutch\RevoltPlayground\Demo\Stacktrace\Command->Kingdutch\RevoltPlayground\Demo\Stacktrace\{closure}() at revolt-playground/Drupal/Core/Async/functions.php:41
[4] Drupal\Core\Async\{closure}() at revolt-playground/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php:597
[5] Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}() at n/a:n/a
[6] Fiber->resume() at revolt-playground/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php:497
[7] Revolt\EventLoop\Internal\AbstractDriver->invokeCallbacks() at revolt-playground/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php:553
[8] Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}() at n/a:n/a
[9] Fiber->resume() at revolt-playground/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php:94
[10] Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}() at revolt-playground/vendor/revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php:117
[11] Revolt\EventLoop\Internal\DriverSuspension->suspend() at revolt-playground/Drupal/Core/Async/functions.php:55
[12] Drupal\Core\Async\stream() at revolt-playground/src/Demo/Stacktrace/Command.php:28
[13] Kingdutch\RevoltPlayground\Demo\Stacktrace\Command->execute() at revolt-playground/vendor/symfony/console/Command/Command.php:279
[14] Symfony\Component\Console\Command\Command->run() at revolt-playground/vendor/symfony/console/Application.php:1031
[15] Symfony\Component\Console\Application->doRunCommand() at revolt-playground/vendor/symfony/console/Application.php:318
[16] Symfony\Component\Console\Application->doRun() at revolt-playground/vendor/symfony/console/Application.php:169
[17] Symfony\Component\Console\Application->run() at revolt-playground/index.php:21

1 and 2 is where the actual exception occurs.
3 is the indirection of wrapping our function call in a closure to be able to prepare it as a deferrable operation.
4-11 is Revolt internals which take care of the scheduling of the asynchronous operations.
12 is where we loop over the operations (thus invoking them) using stream
13-17 is Symfony's application and console command handling.

I wonder if we can run this in CI against files in `git diff --diff-filter=A --name-only` i.e. require level 9 for all new files but avoid the conflict hassles of the baseline?

The maintainer of PHPStan wrote a blogpost about why we shouldn't do that :D

It also makes intuitive sense because if I change the type of an interface then that's the only file that's changed. However, we know that changes in interfaces might affect all classes that implement them, so we can't rely on only analysing the interface.

I wasn’t quite sure how to test it. I suppose we could test that the isOk and isError types do what they say.

What I’ve previously done is write a few function calls around it with PHPStan at level 9 to make sure all the type annotations had the right behavior. We don’t really have infrastructure for that though and that should be solidified when we adopt it :)

See the linked PHPStan sandbox for that bit of code

I’ll write a CR proposal in the coming days unless someone is faster

Here's a first attempt at a proposed documentation change.

I've used similar language as the PER standard which says 'The term "class" refers to all classes, interfaces, traits, and enums.'. I think treating those all the same is a good idea because even the PHP manual says "Enumerations are a restricting layer on top of classes and class constants, intended to provide a way to define a closed set of possible values for a type.".

Updated the issue summary with the proposed resolution which aligns Drupal with the rest of the PHP ecosystem (PascalCase).

I used to be for UPPER_CASE because in a lot of places we're going to replace constants with enums. However, I think #17 lays out an excellent case for why enums are not constants and why, from a differentiation and readability point of view, PascalCase and following Symfony (see #11 & #12) and PSR-12 (see #2) is the right answer.

As an added bonus that means PHPCS won't shout at you if you move between Symfony or Drupal because you forget which uses which when contributing. It also prevents us from having things like the following in our codebases:

if ($value1 === SomeSymfonyEnum::Value && SomeDrupalEnum::OTHER_VALUE)

I hope no one wants the above in their codebase ^^'

+1 for PascalCase (or UpperCamelCase as some call it)

I missed Catch's feedback on the missing bool return types for isOk/isError which I've now added :D

Merging this is postponed on the two other issues landing but the proposed MR includes the code of the other two issues to show that altogether all the test coverage that exists is green and good to go.

As stated in the Revolt documentation "every application making use of cooperative multitasking needs a single scheduler (also called event loop), which this package provides." which is also backed up by Node.js's event loop implementation or the different native PHP event loop implementations that Revolt delegates to for us.

While working on converting the BigPipe mini-event loop into a set of tasks on Revolt's event loop this necessity was discovered in that the mini-event loop in the Renderer caused tasks to be resumed in placeholder tasks on the renderer. To fix this we'll have to convert both at the same time and I've adjusted this issue accordingly.

There are still Fiber references in the Drupal code-base because they are for suspending on single-tasks and thus don't act as mini-event loops. This causes them to work with the Revolt event loop code. We probably want to convert those to Revolt's event loops primitives in the future, but I've left everything that wasn't required for the test coverage to function outside of the scope of this issue to reduce the review surface.

I've split out the change from Neon to PHP into 📌 Convert the PHPStan baseline from NEON to PHP Fixed

That also includes the update to the GitLab CI to output the baseline in the PHP format there. The functionality to output the baseline as artifact was previously implemented in 📌 Incorporate improvements to how contrib runs PHPStan to core RTBC

This branch has been rebased on top of 11.x and the work from 📌 Convert the PHPStan baseline from NEON to PHP Fixed has been cherry-picked in to see the pipeline status in this one. That means this issue is now postponed on the other one :)

I wanted to start to implement this but see that this already exists, so it was feedback to update the existing implementation to the new format, not to implement it anew.

Kingdutch → created an issue.

Kingdutch → created an issue.

I'm skipping #8 for now until we know what the baseline will actually look like. At that point we can decide whether it needs to be excluded from generated archives. I think it's a good proposal and an easy addition though.

Replying to #10, #11, and #12, paraphrased:

What about merge conflicts in the baseline? They're tedious

I think there's two things I could bring up to hopefully alleviate this concern.

Firstly, I think the decision to use PHP for the baseline rather than NEON will help git reduce the number of conflicts. The NEON baseline is basically a giant list of YAML which I believe git treats as plaintext which often causes it to not know which array item marker (-) belongs to which rule. Example excerpt from Open Social below.

parameters:
	ignoreErrors:
		-
			message: "#^Method Drupal\\\\activity_basics\\\\Plugin\\\\ActivityAction\\\\CreateActivityAction\\:\\:create\\(\\) has no return type specified\\.$#"
			count: 1
			path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php

		-
			message: "#^Method Drupal\\\\activity_basics\\\\Plugin\\\\ActivityAction\\\\CreateActivityAction\\:\\:create\\(\\) has parameter \\$entity with no type specified\\.$#"
			count: 1
			path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php

		-
			message: "#^\\\\Drupal calls should be avoided in classes, use dependency injection instead$#"
			count: 1
			path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php

For PHP there are more clearly defined boundaries because the start and end of a rule are distinctly delineated by different tokens (the PHP open and close array syntax). An excerpt from the new PHP baseline from Drupal Core:

<?php declare(strict_types = 1);

$ignoreErrors = [];
$ignoreErrors[] = [
	'message' => '#^Call to function method_exists\\(\\) with \'Composer\\\\\\\\Composer\' and \'getVersion\' will always evaluate to true\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/../composer/Composer.php',
];
$ignoreErrors[] = [
	'message' => '#^Method Drupal\\\\Composer\\\\Composer\\:\\:drupalVersionBranch\\(\\) should return string but returns string\\|null\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/../composer/Composer.php',
];
$ignoreErrors[] = [
	'message' => '#^Parameter \\#2 \\$base_dir of method Drupal\\\\Composer\\\\Generator\\\\ComponentGenerator\\:\\:generate\\(\\) expects string, string\\|false given\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/../composer/Composer.php',
];

This should help git better differentiate between different changes and reduce the number of merge conflicts that are actually flagged. It should also help developers better understand conflicts because they're dealing with PHP code, rather than the middle of a huge YAML list.

Secondly, however, if developers are manually fixing conflicts in the baseline file then I think we're doing a disservice to them. The baseline is the result of the changes in our code and is a file that's automatically generated by PHPStan so we really shouldn't be fixing conflicts manually. My recommendation here would be to educate developers to fix conflicts by regenerating the baseline file (during a rebase or merge commit):

git checkout <base-branch> -- phpstan-baseline.php
vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline core/phpstan-baseline.php 

PHPStan Baseline reviews are also pretty straightforward to know whether such a regeneration was successful. Any removals are improvements in code-type quality and will be caught by the code review itself, so they can be blindly accepted. Any additions need some attention and hopefully spark discussion for code improvements.

When we do end up going there, I'd want us to consider bringing in

phpstan/phpstan-strict-rules

I'm a big fan of the strict rules, but I think there's value in the higher PHPStan levels even without it. So I would propose we evaluate it in a follow-up issue :)

And at least having configs like:

reportUnmatchedIgnoredErrors: true
checkMissingIterableValueType: false
checkGenericClassInNonGenericObjectType: false

I can happily say that reportUnmatchedIgnoredErrors is enabled in PHPStan by default, so there's no need for us to add it to the config unless we want to turn it off (which we shouldn't).

For checkMissingIterableValueType I already had a short discussion with mglaman on Slack which I think is worth repeating here:

We can calculate the number of errors ignored by the baseline by counting the number of lines in the file (wc -l core/phpstan-baseline.php) subtracting 5 lines for setting up the array and returning it from the file and then dividing it by the 5 lines per rule that are used in the file.

This brings us to 78772 ignored errors. Of those 19445 contain the sentence "in iterable type array"; roughly ~300 further errors contain missing "in iterable type" in a non-array types. I still think there is value in fixing those non-array type iterables (think about things like field item lists) and thinking about how we introduce other iterable types. So my proposal would be to manually ignore errors of the type *in iterable type array* then we can still fix other iterable types but at least don't need to go out and fix all the render arrays that are being pushed around immediately.

Disabling checkGenericClassInNonGenericObjectType would remove another roughly ~500 ignored errors. Though I would argue that use of generic typing in Drupal might increase (as we add @template annotation to indicate a class is generic over a certain type) and having that information available can make consuming such code a lot easier (because it ensures the return types don't need to be mixed). So I'd urge keeping the generic check enabled. Disabling it would for example negate almost all benefits of the types for ✨ Implement a Result type in Drupal Core Needs review .

On Slack alex.skrypnyk mentioned we probably want to have a core committer review this issue. Given the impact this issue has on the requirements for future code to be merged and reviewed, I agree. I believe the release manager tag is most appropriate (and they can escalate to "Needs committer feedback" if needed (since it's mentioned as use sparingly → ).

Forgot to remove the tag

Moving this from "Postponed" to "Needs work" for the "Needs tests" and updating the issue summary.

geek-merlin accidentally commented on the wrong issue 🐛 Entity field relationship queries for multi column field items stored in shared tables are broken Needs work but meant to reply to reply here to 27. Quoting for completeness.

Unblocking this as the other issue is stalled, and imho no blocker for working on this.

> and I don't actually understand what this example syntax would query for:
> tid.referenced_by:tags:node.nid

"Referenced by field tags of a node" ;-)

The added example in the Patch (tid.referenced_by:tags:node.nid) indeed translates to "Select the taxonomy term that has it's Term ID referenced by the tags field on Node for nodes with the following node ID".

Similarly the example my Mirroar in #12: $query->condition('tid.referenced_by:field_tags:node.field_article_type', 'news') can be explained as: "Select the terms where the Term ID is referenced by the field_tags field on nodes where field_article_type is news"

I worry here that just jumping straight to level 9 will mean contributors have more work to do when submitting MRs against existing code [...] In turn that means having to make decisions whether or not to explicitly add more things to the baseline.

I think that's a fair worry, specifically because they'll have to think about types in a way that we don't have to do now though. It's a trade-off of short-term pain for long term gain. The work they'd have to do now would otherwise be work we'd have to do at some point in the future.

The baseline is also very good at ensuring that these kinds of changes can happen locally without having to go and fix the whole file just because you're changing a single line.

sometimes we have to do things for backward compatibility reasons and I think satisfying PHPStan is not always possible without breaking BC?

I'd be curious to see examples of this and whether I can offer ways around it! In general PHPStan allows a lot of ways to add type information without actually changing the functioning of the code (e.g. through assert or @annotations).

What might happen is that Drupal becomes better at typing its code which will cause downstream projects to suddenly see PHPStan errors where there were none before. However, I personally don't consider that a breaking change because it's clarifying intention. As long as the PHP code keeps working without errors and doing what it promises then that would still be backwards compatible.

It will simply make impossible to contribute new code, since you'll have to develop huge workarounds to get sorted all the info which is missing from core codebase at the moment (description of iterable types, narrowing of types, etc etc).

I'd want to respectfully disagree. I definitely think there's a learning curve. We'll have to go through that as a community at some point, but if we do our lives will become easier. Better documentation on how to solve common type issues can help there and I'm willing to help write that if it can help speed up Drupal's move to strict-typing.

One habit that I see with developers is the impulse to add /** @var */ annotations to solve typing warnings when upstream types are not yet set. That is indeed something that becomes harder to maintain. A much better way of solving that is to use assert instead. It has the benefit of ensuring that your tests fail if what you're saying is false and PHPStan will let you know when the upstream types have improved and the assert is no longer needed.

If iterable types is truly a problem then we can disable that behaviour specifically with checkMissingIterableValueType: false but having the iterable types specified makes your foreach loops so much nicer and gives you type completion without having to do extra type annotations or asserts within your loop.

An average contrib module can get to L5 relatively simple, L6+ is more work in satisfying PHPStan than in anything else.

As a counter-example, I would consider the Open Social distribution a non-trivial codebase with about 69 top-level modules (and a whole bunch of nested modules). We've been running at level 8 for three years and counting and our code has become significantly higher quality with a measurable reduction in bugs as a result.

Kingdutch → created an issue.

This was failing on some coding standards (parts of which is PHPCS not yet understanding more complex PHPStan types). These were fixed in a commit amend.

I'm unable to reproduce this against the latest version of PHPCS.

class SniffTest {

  /**
   * Kitchen sink test.
   *
   * @return array<string, mixed>
   *   Array of results.
   */
  public function kitchenSink() : array {
    return ["result" => "foo"];
  }

}

I used to be against this because it led to codebases having `use` import statements for types that weren't used in code but only in comments. However:

1. That was often specifically for annotations which are already being replaced by Attributes
2. That was often because PHP did not yet support the type information within the language where now most PHPDoc comments will have a matching parameter or return type hint for which the use statement is already needed
3. A reference in a comment is still a "usage' because it indicates some kind of contract, so for being able to find "Where something is used" with static analysis tools it's still important in case of deprecations and such; PHPStan already does these things for us which further proves that this is no longer a problem

Given those developments within PHP, I'm also happy to +1 this proposal :D

I've asked for some help in the PHPStan discussions on GitHub and Ondrej was very kind in providing more insights: https://github.com/phpstan/phpstan/discussions/10667. It turns out I was running into a known issue https://github.com/phpstan/phpstan/issues/6732.

I've now pushed a workaround using a type annotation :)

I've created a branch to start work onto which I've cherry-picked the proposed changes for the issues that are blocking this. Those commits will need to be removed in a rebase when other issues land.

The proposal for the BigPipe rewrite is in https://git.drupalcode.org/issue/drupal-3425212/-/commit/cd1c3fb87507dc5... which only looks at the BigPipe implementation and does not yet do anything for the tests themselves which contain some Fiber specific code that will also need adapting.

Kingdutch → created an issue.

Added a merge request to show the proposed implementation. However this "Needs Work" because we can't quite tell PHPStan what we want to do :)

However, this allows async issues to start using the type and demonstrate its usefulness.

Kingdutch → created an issue.

Kingdutch → created an issue.

Updated the remaining tasks. I've created a child issue to add the dependency to the composer.json: 📌 Add revoltphp/event-loop dependency to core Active which now also contains the dependency evaluation.

Kingdutch → created an issue.

Kingdutch → created an issue.

quietone → credited Kingdutch → .

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

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

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

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

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

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

Kingdutch → created an issue.