Account created on 30 April 2013, almost 12 years ago
#

Merge Requests

More

Recent comments

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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
🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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 ?

🇳🇿New Zealand quietone

@jonathan1055, thanks for the detailed review!

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

🇳🇿New Zealand 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!

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

Simply headers. Coding standards meetings 2025-02-25

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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 .

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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.

🇳🇿New Zealand quietone

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'
🇳🇿New Zealand quietone

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'?

🇳🇿New Zealand quietone

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

🇳🇿New Zealand quietone

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

Added change record.

🇳🇿New Zealand quietone

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

Production build 0.71.5 2024