@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.
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.
Rebased and put up a new MR against canvas namespace. Fixed a few issues due to interface changes, etc. Seems to be some failures in HEAD, otherwise I think this should still be green. I guess this can move to NR?
mstrelan → changed the visibility of the branch 3483307-loading-from-lb-fields to hidden.
mstrelan → changed the visibility of the branch 3483307-add-support-for to hidden.
Re #43 we have this for node list now since ✨ Add 'View' to operations links where available Active
Rebased and fixed a conflict with 📌 Disable components by default sourced from block plugins provided by core in the Standard profile Active
Thanks for the pointers. Removing the top and left styles resolves this.
The other difference I see when removing the picker style completely is that the browser allows the open select list to extend outside of the browser window if there is space available on the screen. This feels much nicer. I'd be in favour of removing it entirely, but could settle on just removing the negative margins.
Oh I found it.
In \Drupal\KernelTests\KernelTestBase::tearDown
// Delete test site directory.
$callback = function (string $path) {
@chmod($path, 0700);
};
This is passed to \Drupal\Core\File\FileSystem::deleteRecursive and it's applied to every file. Perhaps we could just fix that to now chmod symlinks? I'm curious why we need to chmod a file that is going to be deleted anyway, if we have permission to set the permissions then surely we can just delete it?
If I'm following this right, the code in question is this:
foreach (scandir("core/modules/user/tests/modules/user_hooks_test") as $item) {
$target = "$this->siteDirectory/$item";
if (!file_exists($target)) {
symlink(realpath("core/modules/user/tests/modules/user_hooks_test/$item"), $target);
}
}
For a start, symlink() expects $target as the first parameter, but I think we're just using the wrong name for the variable. The target is where the symlink points to, which is the file in core/modules. The link we're creating is written to $this->siteDirectory which should be in sites/simpletest, which should be gitignored.
If I'm mistaken, can you point to where the test is writing to a file that is checked in to the repository?
Tested by making the same change to core/phpcs.xml.dist and running composer phpcs. Compared the list of reported files with the MR and confirm they match up. PHPCS job is passing now so confirming that is resolved. Spot checked the changes for any discrepancies but all looks ok, including odd looking test cases that have strings beginning with white space characters. Also grepped in core/tests/Drupal/Tests/Component for "phpcs" and "@codingStandardsIgnore" to see if there were any ignores to remove but there were not.
Rebased and resolved conflict due to 🐛 Why does DbImportCommandTest install modules? Active
Rebased and resolved conflicts due to 📌 Fix LongLineDeclaration in Functional tests Active
Added the CR. Note it only affects classes that extend a base class using the trait and override this method.
Have opened an MR for 11.2.x, although not sure if should backport this, since we didn't do the same for #3547124: Remove system module from kernel tests that don't need it → , which is the source of the conflict.
I put up an alternative more advanced solution using @phpstan-type and @phpstan-import-type in MR !13352. Not sure if core is ready for this though so feel free to run with the original if that's desired.
Added some comments, I think at least we need to update the return description. Maybe add @see \Drupal\user\PermissionHandlerInterface::getPermissions explaining that's what a permission looks like.
OK, I was trusting the MR overview page saying it merges cleanly. I tested manually and can confirm it applies with patch -p1 but not with git apply.
$ git fetch && git reset --hard origin/11.x
HEAD is now at 5cbac86546d
✨
Add 'View' to operations links where available
Active
feat: Add 'View' to operations links where available
$ curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | git apply
error: patch failed: core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php:16
error: core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php: patch does not apply
$ curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | patch -p1
patching file core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
Hunk #1 succeeded at 17 with fuzz 1 (offset 1 line).
Have rebased and force pushed now.
I think there is a bit of boilerplate we can remove, have marked up in the MR. Also wondering about a base class, see comment in the MR. These are not blockers though so I'd leave the status as NR, except I think the $session->wait() needs attention, so setting to NW.
mstrelan → changed the visibility of the branch 3524379-history to hidden.
Rebased and replicated changes from CommentLinkBuilderTest to the new HistoryCommentLinkBuilderTest
Currently it passes with 384 MB so the current requirement is somewhere between 256 and 384. If we set it to -1 and the process starts requiring more than 512 MB (or possibly much more) then that's a good indicator we might have a problem. Maybe phpcs started scanning node_modules or vendor or something like that.
Rebased for the changes in 📌 Deprecate comment libraries and move to history module Active but this is now postponed on 📌 Deprecate route comment.new_comments_node_links Active
Clean rebase, not sure why the bot complained. Restoring RTBC as I didn't do anything else.
$ git fetch origin
$ git rebase origin/11.x
Successfully rebased and updated refs/heads/3542528-deprecate-route-comment.newcommentsnodelinks.
Added my thoughts to the MR. Setting NW because I think the copied tests should be trimmed down in this issue.
#13 - please see the parent issue / meta, the approach can be discussed there. TL;DR a new text_with_summary module is created for the text_with_summary field type, the module is then moved to contrib and deprecated in core. Then we remove it from core in the next major. This allows sites that rely on this field type to keep using it if they want to.
Some of the tests were random fails, I fixed a couple more, and there are some remaining.
This is pretty straightforward, have put up an MR that fixes it.
I can reproduce this on 11.x with the example module in #5. Updating steps to reproduce.
I tested 3535024-fix-with-window and it seems to be working well. The issue I had was with a component that has a date field and a multi-value tags field (with #3546869). Using the "add another item" or "remove" button on the tags would occasionally result in Error 500: DateTime::createFromFormat(): Argument #2 ($datetime) must be of type string, array given. I haven't been able to reproduce the error with MR !122 applied.
This is not so much about restricting at the moment, but about providing a default component in a slot. For example, adding a Slide Deck component would automatically add one (or more) Slide components in the slides slot. Assuming the Slide has its own default values for its fields, we don't need to define those in the Slide Deck.
This needs a rebase, but also I'm not sure we should include the most recent commits. There's a reason I left them out originally, in fact I commented about them in #12 so this is rather counter-productive.