Did you mean to "needs review" this?
Yes, I was waiting for a green pipeline and then forgot to come back to it.
Core does use this from ResourceTypeRepository
Thanks for pointing that out, I was relying on usages detected by phpstorm but should have grepped for it. You first example is just setting the value, but my point was that I didn't think it was ever accessed, which was wrong. It is also accessed in ResourceObjectNormalizer::serializeField, but with a todo comment to remove it.
Agree that it shouldn't be nullable, although does it make sense for null to fall back to
\stdClass?
I think it's probably good to force an explicit decision to be made.
I think we could probably also update the typehint in \Drupal\jsonapi\Serializer\Serializer::selfSupportsDenormalization to expect class-string in this issue as well.
Surely we can be more specific than mixed, can we use a union type?
Reverted UpdatePathTestTrait::runUpdates and opened follow up:
📌
Remove $update_url param from \Drupal\Tests\UpdatePathTestTrait::runUpdates
Active
Only other suggestion is that we could also update the @param doc in DateFormatter::dateFormat to non-empty-string and potentially also expand the doc for $dateFormats property to something like array<non-empty-string, array<string, \Drupal\Core\Datetime\DateFormatInterface>>, but not sure we want to include that here, so setting to RTBC.
Can we possibly use the existing unit test at \Drupal\Tests\Core\Datetime\DateTest?
That's a good point in #35. There is actually a slight drop in numbers after a recent commit adding <T> to the conditional return.
Was needs work intentional? Maybe back to needs review for #34?
Agreed this works well in phpstorm. I also ran phpstan on max level before and after and there was no difference in the total number of violations, not sure what to make of that.
I notice this now closely matches \Symfony\Component\DependencyInjection\ContainerInterface::get, except that includes conditions for the $invalidBehavior param that we don't need to worry about here.
/**
* @template C of object
* @template B of self::*_REFERENCE
*
* @param string|class-string<C> $id
* @param B $invalidBehavior
*
* @return ($id is class-string<C> ? (B is 0|1 ? C|object : C|object|null) : (B is 0|1 ? object : object|null))
*
* @throws ServiceCircularReferenceException When a circular reference is detected
* @throws ServiceNotFoundException When the service is not defined
*
* @see Reference
*/
public function get(string $id, int $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE): ?object;
If we're happy with this change of directions then this needs an issue summary update, otherwise RTBC for me.
I was about to comment the same as #20. I think it's worth exploring.
In https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types there is this example:
/**
* @template T of object
* @param class-string<T> $class
* @return ($throw is true ? T : T|null)
*/
function getService(string $class, bool $throw = true): ?objectI wonder if we can do this:
/**
* @template T of object
* @param class-string<T>|string $id
* @return ($id is class-string ? T : object)
*/
public static function service($service)
I think you're right.
When this was introduced in #2663264: Queue's database backend is a core service but it depends on system install → we only called it once:
if (\Drupal::service('queue') instanceof QueueGarbageCollectionInterface) {
\Drupal::service('queue')->garbageCollection();
}
But this was incorrect, because the service is the QueueFactory, so in #2705809: Queue garbage collection is not correctly run on cron → it changed to this:
foreach (array_keys($queue_worker_manager->getDefinitions()) as $queue_name) {
$queue = $queue_factory->get($queue_name);
if ($queue instanceof QueueGarbageCollectionInterface) {
$queue->garbageCollection();
}
}
But now there may be multiple queues that are instances of DatabaseQueue, so the same garbage collection runs multiple times.
The question is whether ::garbageCollection should add a condition on the queue name, or if the cron hook should be updated to run once per queue type, in which case I think ::garbageCollection would have to be static.
While we're here, I'm wondering why this query doesn't live in \Drupal\Core\Queue\Batch:
// Clean up the queue for failed batches.
$this->connection->delete(static::TABLE_NAME)
->condition('created', \Drupal::time()->getRequestTime() - 864000, '<')
->condition('name', 'drupal_batch:%', 'LIKE')
->execute();
mstrelan → changed the visibility of the branch 3555562-theme-test-repeat to hidden.
Thanks @demeritcowboy, resetting the container worked. See https://git.drupalcode.org/issue/drupal-3555562/-/jobs/7139554 where it passed 100 times.
Setting to Needs review for MR !13642, but I wonder if there is a global fix that we could apply. Possibly \Drupal\Tests\BrowserTestBase::config should call Drupal::config directly.
I think this is "works as designed", maybe you can provide more info about the bug and your expectations?
Sleeping for 1 or 5 seconds doesn't seem to help either:
https://git.drupalcode.org/issue/drupal-3555562/-/jobs/7128867
NULL would indeed be better. For example, in \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::assertPageLoadComplete, we could update $this->loggedInUser && $this->loggedInUser->hasPermission() to just $this->loggedInUser?->hasPermission().
Core doesn't seem to have any cases where we are checking if the property is FALSE, only the loose empty check, but of course contrib would be the issue. I guess the same way would be to deprecate the $loggedInUser property and make a new $authenticatedUser property.
Thought we could fix this with WaitTerminateTestTrait but doesn't seem to be the case.
3555562-theme-test-repeat:
Run 1: 3/100 fails
https://git.drupalcode.org/issue/drupal-3555562/-/jobs/7128506
Run 2: 21/100 fails
https://git.drupalcode.org/issue/drupal-3555562/-/jobs/7128619
3555562-theme-test-fix:
Run 1: 12/100 fails
https://git.drupalcode.org/issue/drupal-3555562/-/jobs/7128649
Over the past 6 months or so I've triaged 150+ issues in the system.module queue. Many of these were incorrectly categorised, so have been relocated to other components. Others have been closed as duplicates, postponed for more info, or where possible I've tried to help to move them along, either by reviewing or working on MRs. I'm happy to volunteer to maintain this component, with the primary goal of keeping it as lean as possible.
Looks like it was a simple mistake of committing the development build, probably from npm run watch instead of npm run build, which passes --mode development to webpack. I've built the production version and created an MR.
Major is for updating the minimum supported version, no harm in changing a recommendation. I'm sure it happened several times throughout the 7.x and 8.x lifecycle.
In the other one I got told the comments are out of scope, so not sure what to do.
Apologies for that, as per #6 a lot of files were being reverted.
There are two commits worth noting:
- 757158e3 Revert DbImportCommandTest - the script reverted some changes to an array that wasn't
$modules, so it's worth checking there is nothing else like that. Ideally tests would fail if that were the case. - 62fd03de Revert MigrationPluginListTest - this test is explicitly installing modules that contain Drupal migrations. It's possible that some other tests should have modules installed, whether they are required for the test to pass or not. For example, they could be testing that something does not fail when the module is installed.
Accidentally pushed some commits from 🐛 Fix more incorrect phpdoc type hints Needs review so I've reverted those and updated the numbers in the IS, and replaced the list of fixed errors.
What is the next step? The docs say it should have the "Needs announcement for final discussion" tag added once it's been approved by the committee. That appears to have happened in June: 🌱 Coding Standards Meeting Wednesday 2025-06-30 0900 Active .
I think the script needs to be updated to commit after each module, otherwise later modules can reset the file.
Is there a particular reason this needs to go in system.module? There are plenty of other event subscribers in Drupal\Core\EventSubscriber.
Opened 📌 Remove unused modules from kernel tests RTBC
I don't have a strong opinion about scope but was wondering if doing all the ones where the module is loaded via a parent class in one issue was possible?
The bash script isn't that sophisticated. Perhaps it could be adjusted to grep for "extends KernelTestBase" to find those that aren't extending another base class, but I'll leave that for others if they want to work on it.
Reverted the two changes that include description updates. Not opening follow ups for these yet as they'll eventually be picked up in the existing meta.
Updated to use \Drupal::service. I agree with #12, it can always be cleaned up in contrib later.
Responded to one, hopefully that helps to explain why they are where they are. I don't intend to make further updates, so feel free to try move things around and run the commands. If things still pass feel free to push those changes.
They are selected at random from looking at a list of around 400 errors and picking ones that occurred several times or looked easy to fix. I have another batch ready to go but this feels like a good size. I don't think there is a way to group them that is particularly meaningful.
Two cents from me - I notice the composer.json is restrictive, which I think is good, and info.yml is open, which is kind of pointless. Patching info files is easy, but forcing composer to install an incompatible version is hard(er). So if you can't even download Linkit 7 with Drupal 15 then there's no real problem here. And if people are using tarballs then it's the wild west anyway.
@smustgrave did you try the steps in the IS? Try that against 11.x and again against this branch and you should see the diff of errors that are resolved. We could nitpick if the @var tags have moved to the right places, but if phpstan is happy now then I think that is satisfactory.
Also here are some updated steps to avoid touching the baseline file, I like to use --error-format=raw to get one error per line for easier grepping.
./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --level=2 --error-format=raw | grep "in PHPDoc tag @var does not exist."
That currently gives me 27 errors on 11.x and 0 in this branch.
Further to #11, another advantage is when code is refactored or deprecated and tests are removed, it's sometimes unclear if test modules are still in use any where, particularly if multiple tests are using the same test module for different purposes. It should be a lot clearer if the logic is in the test.
Regarding #[RunTestsInSeparateProcesses] and requiring it for contrib, I noticed you've used rector for the core conversions, but I think there is a lower barrier to entry for phpcs and phpcbf since it's already installed in drupal/core-dev. I had AI generate a fixable phpcs rule, tested it on a contrib module and it worked. Should we consider opening a coder issue for this?
These tests also have the option to not use the trait at all. They also don't need to do the awkward twiddling and can completely replace the alter method. I agree on that extra helper function though. The trait can have alter and alterTimeService and then tests that need to use alter for something else can call alterTimeService.
Having it in a trait doesn't stop tests from also using alter, but they do need to be aware that the alter exists.
I was trying to follow your suggestion before the edit and obviously you noticed that didn't work either :D
Form alter in the theme works, feels very GovCMS, but I guess it'll do. Thanks for your quick response.
@oily we don't need the test-only job here, it's only useful for bug reports as it demonstrates the bug existed in HEAD and is fixed in the MR. Also, pasting its output only pollutes the issue and leads to more scrolling. Better to link to the job output if it's relevant.
Left some comments. But shouldn't the descriptions be updated too to match what's being changed?
From my perspective that just opens up more for reviewers and committers to have to parse and agree on. You could also argue that if we're updating the doc types we should also be adding native typing too, but that's not in scope, so why are descriptions? Nevertheless I've taken a stab at this.
I'm not sure scanning all of core is the default operation (I'm assuming the IS means it OOMs when scanning all of core).
I had never considered that you could pass additional arguments. The default is to scan all of core, but you can indeed pass a path to scan instead.
I'm not sure about harcoding a specific memory limit like this. Can the developer still override it to something higher or lower?
AFAIK the supported way to set the memory limit for this command is COMPOSER_MEMORY_LIMIT=512M composer phpcs. I tested this on this branch with 16M and it OOM'd straight away, so I guess that's working.
$ COMPOSER_MEMORY_LIMIT=16M composer phpcs
PHP Fatal error: Allowed memory size of 16777216 bytes exhausted (tried to allocate 204800 bytes) in /usr/share/php/Symfony/Component/Process/Process.php on line 921
I also tried the inverse, by setting the memory limit in composer.json to 16M and 512M in the env var, but that also OOM'd.
TL;DR the developer can set it lower but not higher. So maybe -1 is the way to go? But in saying that, the command is not particularly useful for scanning anything other than core, since core has its own specific rules and path inclusions.
I think the user module could do a route alter to restore the deprecated route if the rest module is not installed, rather than the rest module doing it. But I will admit I haven't been following closely.
I'm not sure how to deprecate the route.
We only support deprecations on route aliases but when can't make user.login.http an alias of the new route because users without the rest module will not have the new route.
I had the same issue on 📌 Deprecate route comment.new_comments_node_links Active , deprecated the old route and create a new one with a different name.
Wonder if we should make this a duplicate of #2768765: Deprecate $pass_raw in UserCreationTrait and DrupalLogin documentation → and repurpose it to use a proper API.
Not sure the best way to find these, but this finds all files in src/Plugin dirs with the same signature:
$ find core -type d -path "*/src/Plugin/*" -exec grep -rn 'public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition)' {} \;
Note this doesn't include classes extending DeriverBase, ConstraintValidator or any Migration plugins with a ?MigrationInterface param. There are potentially others.
Ideally this is a job for rector as there are 267 exact matches.
This may be a duplicate or could be a child issue of 🌱 [meta] Add return typehinting to the test codebase Active or 🌱 Add return typehints to protected test helper methods Active .
I think the key consideration in that meta is "first where non-disruptive, then where possibly disruptive (base classes, traits, etc.)"
I like that idea. We have auto retry on functional javascript tests, we could put it on the flakey group instead.
Can you see if this patch works?
https://www.drupal.org/files/issues/2025-03-21/3468180--40--10.5-code-on... →
The variable passed to strtr is $variables['elements']['#configuration']['view_mode'], see https://git.drupalcode.org/project/drupal/-/blob/10.5.4/core/modules/blo...
I can see in 11.x this changed to $variables['elements']['content']['#view_mode'], in the BlockContentHooks class now.
This was fixed in 🐛 Undefined array key "view_mode" in block_content_theme_suggestions_block_alter Active , perhaps it should be backported?
Test still passes after this is removed.
Can confirm this is the only remaining usage.
Sorry if this has been discussed already, but should we be running phpstan analysis for 8.5? I see this in the output:
PHP runtime version: 8.4.13
PHP version for analysis: 8.3 (from config.platform.php in composer.json)Presumably IconPackManagerTest would have failed phpstan if it was running on 8.5. But then we might not catch issues for 8.3, so it's a double edged sword unless we run it for all supported versions.
Updated IS with the new path to find the file and proposed resolution pointing to tests for other actions.
Also not sure why it's in the forms system component, moving to base system because we don't have an actions component and this action is not in a module.
The example URL you provided does not have the itok query parameter. This is essential for derivatives to be created. Where are the URLs coming from? All of the media / responsive image / image formatters etc should work out of the box. Are you using something custom to generate the URLs? You can use \Drupal\image\ImageStyleInterface::buildUri to generate the URL correctly.
Fixed the fail, was just one method that didn't want to update.
This has some failing tests, so obviously need to revert or adjust some of the changes.
The interface requires that the $value param is mixed|null, so we can't narrow that in the concrete class, as parameters follow contravariance.
#16 suggests that we simply need to throw InvalidArgumentException if the wrong type is passed, which we're already partly doing, but we need a way to document this. I don't think we necessarily need to use inheritdoc, since there are nuances here. We can improve the existing descriptions to describe what types should be passed in, and mention that anything else will throw the exception.
Apart from the documentation changes I have some other concerns.
- The
$languageproperty is a concrete Language object but the proposed resolution is to check for the interface. Should the property be updated to allow an interface or should the we only allow the concrete object? - Should we deprecate any other other object that happened to work before? This is probably only relevant if we narrow down to the concrete Language class, but there is a non-zero chance this was used with other classes too.
Needs work because there is a patch, obviously needs a merge request started.
Added steps to reproduce, can confirm all 9 instances in the baseline are fixed in this MR.
I'm really not sure about using templates like this. I realise I'm in the minority using vim (and without whatever vim support would take advantage of this, there's probably something somewhere), but don't think I'm the only one. There's also reading docs on api.drupal.org or gitlab which people sometimes do.
I'm guessing this is specifically about ListInterface, which is the only class that uses the templates outside of where they are defined. This concern is understandable in that looking at that method in isolation doesn't tell you what T is. I think longwaves suggestion in #15 to use @phpstan-return and leaving the original @return for these addresses that concern, so I think we can make this RTBC. Please set back if you're not satisfied with that.
We could of course have a coding standards discussion for use of templates, e.g. use @phpstan- params when referring to templates, but I don't think we should block this on that issue.
So, it's like optional but recommended.
More like it's legacy from a time before typed parameters and properties.
No, this has already been through the coding style review process and we use PascalCase for enums.
mstrelan → made their first commit to this issue’s fork.
I'm confused about the code comments referring to "current time" and the implementation referring to "request time". Which one are we mocking?
Also needs work for a comment that obviously needs to be changed.