Account created on 12 June 2008, over 17 years ago
#

Merge Requests

More

Recent comments

🇦🇺Australia mstrelan

@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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

Having it in a trait doesn't stop tests from also using alter, but they do need to be aware that the alter exists.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

@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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

Adding related issue for controllers

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

Published the CR

🇦🇺Australia mstrelan

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.)"

🇦🇺Australia mstrelan

I like that idea. We have auto retry on functional javascript tests, we could put it on the flakey group instead.

🇦🇺Australia mstrelan

Blocker is in

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

Fixed the fail, was just one method that didn't want to update.

🇦🇺Australia mstrelan

This has some failing tests, so obviously need to revert or adjust some of the changes.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

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.

  1. The $language property 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?
  2. 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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

So, it's like optional but recommended.

More like it's legacy from a time before typed parameters and properties.

🇦🇺Australia mstrelan

No, this has already been through the coding style review process and we use PascalCase for enums.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

Rebased against the canvas namespace

🇦🇺Australia mstrelan

mstrelan made their first commit to this issue’s fork.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

mstrelan changed the visibility of the branch 3483307-loading-from-lb-fields to hidden.

🇦🇺Australia mstrelan

mstrelan changed the visibility of the branch 3483307-add-support-for to hidden.

🇦🇺Australia mstrelan

mstrelan made their first commit to this issue’s fork.

🇦🇺Australia mstrelan

Re #43 we have this for node list now since Add 'View' to operations links where available Active

🇦🇺Australia mstrelan

mstrelan made their first commit to this issue’s fork.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

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?

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

Rebased and resolved conflict due to 🐛 Why does DbImportCommandTest install modules? Active

🇦🇺Australia mstrelan

Rebased and resolved conflicts due to 📌 Fix LongLineDeclaration in Functional tests Active

🇦🇺Australia mstrelan

Added the CR. Note it only affects classes that extend a base class using the trait and override this method.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

Seems like a good idea, just one question on the test

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan changed the visibility of the branch 3524379-history to hidden.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

MR applies cleanly, bot is drunk

🇦🇺Australia mstrelan

Rebased and replicated changes from CommentLinkBuilderTest to the new HistoryCommentLinkBuilderTest

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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

🇦🇺Australia mstrelan

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.
🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

mstrelan created an issue.

🇦🇺Australia mstrelan

Some of the tests were random fails, I fixed a couple more, and there are some remaining.

🇦🇺Australia mstrelan

mstrelan made their first commit to this issue’s fork.

🇦🇺Australia mstrelan

This is pretty straightforward, have put up an MR that fixes it.

🇦🇺Australia mstrelan

mstrelan made their first commit to this issue’s fork.

🇦🇺Australia mstrelan

I can reproduce this on 11.x with the example module in #5. Updating steps to reproduce.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

🇦🇺Australia mstrelan

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.

Production build 0.71.5 2024