Fix Drupal.Semantics.FunctionTriggerError coding standard

Once again I can't push after rebasing. I am uploading a patch.

Once again I can't push after rebasing.

When that happens to me, I always have success with git push --set-upstream drupal-3048495 HEAD -f
So basically the command from "Or push your current local branch from your Git clone" followed by a -f (for force, use it Luke...)

Probably to late for this occasion, but you might wanna give it a go next time.

The last submitted patch, 176: 3048495-176.patch, failed testing. View results →

Wasn't terrible to review as most of the files were 1 line.

Changes look good and like that it will capture good trigger_error messages.

I haven't seen it before for other coding standard tickets but would a change record be worth it?

Spokje → closed merge request !2294

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I am assigning to myself because I have local changes that I am not able to push. I'll un-assign once that is fixed.

In the meantime there are two new errors that need a link to the CR. The issue, ✨ Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way Fixed has two CR's but neither of them directly address the removal of the constructor parameter. I'm not sure if one of the CR's should change or a new one is needed. Suggestions?

Here are the errors,

FILE: /var/www/html/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 88 | ERROR | The trigger_error message 'Injecting  __CLASS__  with the "cache_factory" service is deprecated in drupal:10.1.0, use "variation_cache_factory" instead.'
    |       | does not match the relaxed standard format: %thing% is deprecated in %deprecation-version% any free text %removal-version%. %extra-info%. See
    |       | %cr-link% (Drupal.Semantics.FunctionTriggerError.TriggerErrorTextLayoutRelaxed)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/core/lib/Drupal/Core/Render/RenderCache.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 50 | ERROR | The trigger_error message 'Injecting  __CLASS__  with the "cache_factory" service is deprecated in drupal:10.1.0, use "variation_cache_factory" instead.'
    |       | does not match the relaxed standard format: %thing% is deprecated in %deprecation-version% any free text %removal-version%. %extra-info%. See
    |       | %cr-link% (Drupal.Semantics.FunctionTriggerError.TriggerErrorTextLayoutRelaxed)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 25.68 secs; Memory: 6MB

Tests are passing so time for reviews.

I'm afraid the commit of 📌 Fix version number in deprecation messages for #2551419 Fixed , ironically an issue by @quietone, now clashes with this MR and it needs Yet Another Rebase (Yar!....)

Rebased the MR

All green with the new rule added. So assuming this is good.

Added some feedback.

Tests are passing, back to NR.

Threads appear to be addressed.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased MR, back to RTBC

Looks like new non-compliant deprecation messages sneaked in 😅

Since I was responsible for the faulty new ones, it seems fitting to fix those myself.

Think this can go back to RTBC, since the changes are checked by the new PHPCS rule?

Yes, that make sense. And I read the last 3 commits to confirm. This is still RTBC.

This needed a rebase. Because there were a lot of conflicts, I am setting back to Needs Review.

Reading the commits after 198 and changes still look good.

Committed fd2c134 and pushed to 11.x. Thanks!

I'm not sure we need to enable the new coding standard in 10.1.x but we might choose to backport the fixed strings for consistency. Not sure.

Great to see this committed. Thank you everyone who contributed.

As someone who worked on the original coding standard, the Coder sniff and the early patches on this issue, I would recommend back-porting the strings enabling the rule in 10.1. One of the original purposes of the standard was to be able to scan source code and get metrics on how many things are deprecated and how many cause triggerError in a particular version of core. That would help with 10 -> 11 upgrades.

@jonathan1055 10.2.x will be cut from the 11.x branch (which really should be named "main" but we can't do that cause reasons).

I think we should backport the message changes - usually we do not backport string changes due to translations but these messages do not get translated, and it will make other backports easier.

We don't usually enable new coding standards in patch releases and I see no need to do so here, given any new deprecations will only appear in 11.x.

10.2.x will be cut from the 11.x branch (which really should be named "main" but we can't do that cause reasons).

OK, thanks for explaining that. Yes, therefore fix the strings in 10.1 but no need to implement the Coder rule.

The commit in #201/202 did not make any changes to phpcs.xml.dist. Earlier patches and MRs contained

--- a/core/phpcs.xml.dist
+++ b/core/phpcs.xml.dist
@@ -123,6 +123,7 @@
   <rule ref="Drupal.Semantics.FunctionT">
     <exclude name="Drupal.Semantics.FunctionT.NotLiteralString"/>
   </rule>
+  <rule ref="Drupal.Semantics.FunctionTriggerError"/>
   <rule ref="Drupal.Semantics.FunctionWatchdog"/>
   <rule ref="Drupal.Semantics.InstallHooks"/>
   <rule ref="Drupal.Semantics.LStringTranslatable"/>

but this is not present in the commit. So the new Coder rule has not yet been implemented in Core 11.x
What was the source of the commit in #201? Was it MR4216? That had 56 changed files but the commit seems to have only 20.

Actually the rule is enabled in core
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/phpcs.xml.dis...
It's just that it was not in the commit shown in #201. Sorry for the noise.

@jonathan1055 GitLab only shows 20 files changed per page, if you go to page 3 then you can see the changes to phpcs.xml.dist: https://git.drupalcode.org/project/drupal/-/commit/fd2c13413a20941c10dd0...

@longwave Thank you. I should have spotted that.

Coding standards changes go in the release notes.

Probably too late to consider backporting this now, let's just mark this as fixed in 10.2.0.

Automatically closed - issue fixed for 2 weeks with no activity.