🇳🇿New Zealand @quietone

quietone → created an issue.

quietone → created an issue.

For consideration, I removed the use of FormattableMarkup to build the message in AssertContentTrait.php. This is only done for methods that are not deprecated.

Rebased and grep currently shows

$ grep FormattableMarkup -r -l | grep Kernel
core/tests/Drupal/KernelTests/AssertContentTrait.php
core/tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.php
core/tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php
core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php
core/tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php
core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php

And modified to remove the files where FormattableMarkup is to remain;

$ grep FormattableMarkup -r -l | grep -v tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php | grep -v tests/Drupal/KernelTe
sts/Core/Theme/TwigMarkupInterfaceTest.php | grep -v tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php | grep -v tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.ph | grep Kernel
core/tests/Drupal/KernelTests/AssertContentTrait.php
core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php

quietone → created an issue.

quietone → created an issue.

Rebased. Restoring RTBC.

@nod_, thanks. The commit-code-check script fails locally for me as well. I think this is due to a bug in phpstan that I found in #11, https://github.com/phpstan/phpstan/issues/12494

Rebased and also needed to make a change to TestSqlIdMap.php

This changes about 1300 lines, which is larger than the scope guidelines suggest but this is only formatting changes.

quietone → created an issue.

Another one fixed.

This really is enabling the sniff for all of core.

smustgrave → credited quietone → .

There are two comments in the duplicate issue that support keeping the module in core.

Actually, the other issue is the one to keep open, for reasons given over there in #3476879-7: [Policy] Move Contact module to contrib → .

@beautifulmind, thanks for triaging this issue. This issue is the one that should remain open. It contains a summary of committer discussion and comments from two of the 3 maintainers of the Contact module.

I am restoring the status.

There is a mistake in this commit, in ohpcs.xml.idst. Instead of adding this,

  <rule ref="Drupal.Commenting.DocComment">
    <exclude-pattern>core/tests/*</exclude-pattern>
  </rule>

it should have been

<rule ref="Drupal.Commenting.DocComment.DocComment.ShortNotCapital">
    <exclude-pattern>core/tests/*</exclude-pattern>
</rule>

Maybe this can be resolved in 📌 Enable 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard for callbacks Postponed ?

@jonathan1055, thanks for the detailed review!

I agree it would help if reviewers would resolve comments. Thanks.

smustgrave → credited quietone → .

smustgrave → credited quietone → .

smustgrave → credited quietone → .

Nice little improvement!

Thanks to @larowlan for manual testing. And to all the help at DrupalSouth 2025.

Committed 6d097f1 and pushed to 11.x. Thanks!

Adding credit to tinarey for figuring out the undefined error.

@mstrelan, thank you. I will delete the change record.

Per #31 the change record is not needed. Therefore, changed title so it can be deleted in 3 months.

One left to look into

quietone → created an issue.

Rebase

Thanks for working on this!

The "proposed resolution" section should be complete and explain the actual change to help the reviewer and committer. Also, this should have some research to support that the change is correct. That is, what is the issue that replaces \Drupal\Core\TypedData\ListDefinitionInterface with something else. Using git blame should make it simple to identify when the changes was made.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changing tags per Issue tags field → and Issue tags -- special tags →

Changing tags per Issue tags field → and Issue tags -- special tags →

Changing tags per Issue tags field → and Issue tags -- special tags →

Changing tags per Issue tags field → and Issue tags -- special tags →

Changing tags per Issue tags field → and Issue tags -- special tags →

If this is a core issue it will be fixed on 11.x first.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Drupal 9 is no longer supported. Does this problem exist on a supported version of Drupal?

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

quietone → created an issue.

Rebased and fixed all violations for callbacks. Any other violations will be handled in a separate issue.

Since it is now possible to enable the sniff in most of core, this does change the phpcs.xml.dist

Updated IS, made an MR with the suggestion.

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

This part of enabling"Generic.CodeAnalysis.UselessOverridingMethod", which is in the Problem/Motivation section.

The scope is correct, it is fixing the instances where an unnecessary override was detected by the sniff. There is no change to phpcs.xml.dist because of remaining overrides, which are not unnecessary, will require discussion, and span the code base.

Adding a parent to help keep track of changes for this conversion.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Rearrange with general first, Tag reference last, and alphabetical in between

Simply headers. Coding standards meetings 2025-02-25

Updating tags per the issue tag guidelines → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Updating tags per issue tag guidelines → . This is already in the 'menu system' component

Updating tags per the issue tag guidelines → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

There are existing issues about improving the permissions for menus. It would help to add those as related and to ensure there are no duplicates.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

quietone → created an issue.

The question about quotes resulted in doing a bit of research. I commented on the MR and updated the IS with the result.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Drupal 10 is in maintenance mode with a limited set of allow changes → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Drupal 10 is in maintenance mode with a limited set of allow changes → .

The remaining failures are expected because this issue is only for callbacks. The sibling issue needs to be committed first 📌 Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard except for callbacks Active .

But I wanted to see if these are correct in content and meet the special standard for callbacks.

Since SafeMarkup was replaced by FormattableMarkup I think we can continue with this issue. There is already a child issue here for Kernel tests. Another one is needed for Functional tests.

As I understand #51, the follow up was to manage the number of changes. Since then, a separate issue has been created for Kernel test, which will help manage the scale of the changes. So, I don't think the tag is necessary any more.

I read the test \Drupal\Tests\system\Functional\Render\PlaceholderMessageTest::testMessagePlaceholder to improve the descriptions for the @return descriptions in RenderPlaceholderMessageTestController.php.

Linting has passed, so back to NR.

Did cleanup here and removed the out of scope changes and the '@return void'.

I think what is left is to improve the comments in core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php, some of which are incomplete sentences.

OK. It is now forbidden.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Nice!

Responded to longwave's feedback.

@joseph.olstad, One thing I like about this is the passion you have for improving Drupal. What I don't like is the disparaging comments in #58. Those are not aligned with the Drupal value of "treating each other with dignity and respect". Overall, this issue helped me understand your situation and the upgrade challenges you have to manage.

For reference, the value I referred to is in the Drupal Values & Principles → .

Tested again with @returns in

• includes/common.inc
• modules/dblog/dblog.module
• modules/migrate/src/Row.php
• modules/views/src/Plugin/views/field/MultiItemsFieldHandlerInterface.php

And PHPStan detected all of them with the same error message as in the previous comment.

It is still not clear where this should go. It could be added as a new page in the guide. PHPUnit in Drupal → . That would encourage the practice now and, if desired, an issue can be opened in Coding Standards to formally accept the practice.

I read through the issue to create a a bare minimum proposed text for the changes, which follows. What do you think?

Assertion messages

The default assertion message provided by PHPUnit is sufficient for most situations. The page explains when a custom assertion message can be added and the format of the message.

When to add a custom assertion message

1. When the assertion in inside a loop.
2. In a custom assertion method in a trait or base test class. A custom assertion method begins with public function assert*();.
3. In a helper method.

Format of a custom assertion message

%subject% should %verb% %payload%

Definitions

%subject%

The additional context information

%verb%

The action.

%payload%

The value the%subject% should be.

Example

foreach ($expected_steps as $element_id => $step_value) {
  foreach (['s', 'i', 'h', 'd', 'm', 'y'] as $sub_field) {
    $name = $element_id . '[' . $sub_field . ']';
    $expected_step = $step_value[$sub_field] ?? 1;
    $input = $this->xpath("//form//input[@name='$name']");
    $this->assertCount(1, $input, "Duration input $name should appear exactly once.");
    $actual_step = (integer) $input[0]->attributes()->{'step'};
    $this->assertEquals($expected_step, $actual_step, "Duration input $name should have the correct step value.");
  }
}

  where

{subject} == "Duration input $name"
should == 'should' ;)
{verb} == 'have'
{payload} == 'the correct step value'

Tested on 11.x today

I changed the '@return' to '@returns' in \Drupal\views\Plugin\views\field\MultiItemsFieldHandlerInterface::getItems().

That resulted in this PHPStan error

 ------ ------------------------------------------------------------------------------------------------------------------ 
  34     Method Drupal\views\Plugin\views\field\MultiItemsFieldHandlerInterface::getItems() has no return type specified.  
         🪪  missingType.return                                                                                            
 ------ ------------------------------------------------------------------------------------------------------------------ 

So, although not a phpcs rule the @returns should not be creeping back in the code base.

Shall we close this as outdated or make a change to phpcs.xml.dist to add '@returns' to the the list of 'ForbiddenAnnotations'?

Came across this because it is on 11.1.x instead of 11.x

@vidorado, thanks for thinking about what else needs to be done. However, issues to remove a particular deprecation are not required. When the time code to remove deprecated code, we can simply grep the code base.

Closing as works as designed since we have practices in place that will resolve this before Drupal 12.0.0

I too am not sure about setting it true by default. I am keen to find out what others think.

Added change record.

@martijn de wit, thanks for the screen shots. The screen shots should be available to reviewers in the issue summary and with an explanation of what they represent. They should at least indicate if they are before or after the diff was applied. And I do wonder if 14 screen shots are needed.

@developer-rocha, the MR is made against 11.x, so testing should be done there. Your verify the version by following the MR link in the issue summary.