Fix Drupal.Semantics.FunctionTriggerError coding standard

Added parent issue and related issue.

Thanks naveenvalecha.
Here's a patch which runs just the TriggerError sniff, and does not do any phpunit testing. "Build Sucessful" is what it will show when done.

798 @trigger_error standards messages. Not too bad. 3mins run time.

Many of the messages contain text like "and will be removed in 9.0.0". As this is not part of the required standard for trigger_error, but is very useful info. I suggest that this is simply moved into the %extra-info% part of the message. Even though this is free text, it would be helpful to change "in" to "from" so that we start getting used to the standard which is used in @deprecated comment tags.

Thanks for the feedback. I'll check core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php and see what I can do.

I have created #3049093: Cater for sprintf() inside @trigger_error call to fix this.

@Lendude I've identified the problem and added a patch to fix it, plus test data coverage, on #3049093: Cater for sprintf() inside @trigger_error call

Hi xjm,
I think you have answered your question in your comment in #13. The issue summaries of #2908391: Add a rule for expected format of @deprecated and @trigger_error() text and #3024461: Adopt consistent deprecation format for core and contrib deprecation messages do both match the new coder rules added. However, the agreed standard was that @trigger_error() would not force the second part of the text "and will be removed" whereas the docblock @deprecated tag does force this. I drew attention to this question in #71 before the consultation period started, but no further objections were raised.

No Core work has been committed to fix the messages, and I'd be quite happy to add that extra part into the sniff I wrote. But it would need full discussion on #3024461: Adopt consistent deprecation format for core and contrib deprecation messages before we did that.

Thanks for the alert. The fix for sprintf has been committed on #3049093-7: Cater for sprintf() inside @trigger_error call
Are you using the full release of coder, or can you try the -dev release which may have fixed this.

Just checked and the existing fix does not cater for this, so could you open a new issue on the coder queue please
https://www.drupal.org/project/issues/coder

Here's a re-roll, due to a committed change in phpcs.xml.dist
This patch is not to be committed, it will just show the current state of core with regard to the new TriggerError sniff.

There are now 826 warning messages for TriggerError in core.

However, work on fixing these should be postponed until any possible change in the standard is agreed and the sniff ammended. The standard was all agreed by 13 March at comment #68 on #3024461-68: Adopt consistent deprecation format for core and contrib deprecation messages but since implementaion of the coder rule there has been further disagreements - read from comment #86

In preparation for resuming this issue, here is the same patch as #19 but with composer.json changed to load coder 8.x-3.x-dev not 8.3.2
Not sure if this will work, but it it needed for Klausi's suggested next steps in #2908391-53: Add a rule for expected format of @deprecated and @trigger_error() text

That did not load the dev version, it just ran with 8.3.2. The patch to composer.json seemed to be applied ok but did not trigger a rebuild of the dependencies. Maybe the composer.lock file also needs to be updated? I don't have the means to do that currently.

Another try

The acual testbot build .yml file contained my change

assessment:
    validate_codebase:
      phplint: {  }
      csslint:
        halt-on-fail: false
      eslint:
        halt-on-fail: false
      phpcs:
        sniff-all-files: false
        halt-on-fail: false
        coder-version: ^8.3@dev
    testing: {  }

but coder 8.3 - dev was still not loaded.

Here's an attempt to require the specific commit in Coder which has the latest changes. Not sure if this is going to work though ...

So the patch to composer.json did not affect the build environment. We get

Applying patch 3048495-25.fix_trigger_error_standard.patch
...
Finished patch
... Starting update_build
Replacing build:assessment stage with /var/lib/drupalci/workspace/jenkins-drupal_patches-99586/source/core/drupalci.yml
Finished update_build
... Starting update_dependencies
Finished update_dependencies

with no change to coder. Maybe within crupalci.yml I can apply a patch to coder 8.3.2 to get the latest changes.

Before working on drupalci.yml I thought it may be that test dependencies are not recalculated if only /core/composer.json is changed, maybe it needs a change to the root level composer.json, Here's the patch in #25 with an additional (dummy) change to composer.json just to see what happens.

So that made no difference. Now, try to patch coder in drupalci.yml

Success! This is now running the latest version of the trigger_error sniff, patched during the host_command section of the drupalci build. The results show 765 coding standards violations, compared to 855 with the old code.

Correct issue summary copy/paste typo to reference @trigger_error function not @deprecated doc tag.

OK here is an updated version of the #63 patch with the drupalci.yml coder patch removed. It should give the same results now that core uses coder 8.3.4

I will have a look at that other issue and see. Many of the fixes need "Drupal 8.4.0" changing to "drupal:8.4.0" etc, so a script could be developed for that kind of change.

I may even see if I can utilise the phpcs automatic fixing to assist in making some of the changes, not to add this into coder permenantly, but use it to generate patches which we then apply manually to kick-start the fixing process.

The results above show 767 @deprecated messages, up from 765 yesterday. The sooner we can get this into core the better.

Taking Gábor's patch as a starter, modifying for the current standard and adding a few more fixed trigger_error messages, here's a first attempt at fixing some of the violations. This only checks the new trigger_error standard, and I know that the full tests will need to be run to ensure that the tests for expected deprecations also match the new messages. But that can be done when the main work is complete, as the full test run takes much longer than just running phpcs.

Yes the patch will be huge, so I would also lean towards your option 2. Get some fixes committed, then it is easier to see the remaining ones.

We need to remember to run the legacy tests that check expected deprecation messages before any commit, to make sure that we have fixed both the source and the test at the same time. Maybe I can add that into the drupalci.yml change. Don't want to run the entire test suite as that uses unnecessary resource (and is slow)

I have had sucess on #3048498-17: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard where I have automated the fixing of common faults using the PHPCS fixable error functionality. 389 fixed out of 960 = 40.5% and I expect be able to use that functionality for the Trigger_error fixes too.

OK yes I could do. However, I have done more work on #3048498-20: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard and that automated patch now fixes 55% of the problems (520 / 930). Would you like me to break out that issue so we can fix that 55% first? I will also work on developing automated fixes for this trigger_error issue, but I've not started it yet. We'd have a bigger win with that other issue. Happy either way, just wanted to point it out.

[edit - unintentional change of status, that seems to happen a lot on d.o. when an issue page is refreshed to view a new comment]

Unintentional change of status above, that seems to happen a lot on d.o. when an issue page is refreshed to view a new comment.
Putting back to Needs Work.

Re-ran patch #33 (the plain one with no code fixes) which now shows core is up to 793 trigger_error faults. I am going to work on automating the fixes for these.

I am working on writing some automated fixes for @trigger_error like I did for @deprecated.

Changing parent issue to the general coding standards issue, as the original parent was a one-off, is closed, and is no longer the best parent.

Patch #46 will show all the @trigger_error coding standards messages.

Ignore #46, it produced no phpcs output due to

ERROR: Ruleset /var/www/html/core/phpcs.xml.dist is not valid
- On line 343, column 3: expected '>'
- On line 344, column 1: Premature end of data in tag ruleset line 2

because I missed the trailing backslash in the tag.

Right, now have something to work with: 876 trigger_error messages, split as follows:
TextLayout - 640
See Url - 72
Deprecation-version - 90
Removal-version - 74

Here's a patch, same as #47, but for Core 9.1 and 9.0
There are some @trigger_error coding standards faults in D9 as we have not fixed any of these yet, in any core version.

Why is this being ignored?

--- a/core/modules/views/src/ViewsConfigUpdater.php
+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -243,6 +243,7 @@ protected function processEntityLinkUrlHandler(array &$handler, $handler_type, V
     $deprecations_triggered = &$this->triggeredDeprecations['2857891'][$view->id()];
     if ($this->deprecationsEnabled && $changed && !$deprecations_triggered) {
       $deprecations_triggered = TRUE;
+      // phpcs:ignore Drupal.Semantics.FunctionTriggerError
       @trigger_error(sprintf('The entity link url update for the "%s" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2857891.', $view->id()), E_USER_DEPRECATED);
     }

It should be fixed properly, and all it needs is a seperate sentence for the 'See %url%'

@trigger_error(sprintf('The entity link url update for the "%s" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2857891', $view->id()), E_USER_DEPRECATED);

passes the standard.

Same goes for the other two messages in that file.

Thanks kishor_kolekar,
Just a reminder, please add back the temporary change to /core/drupalci.yml which you can find in patch #53. This will mean your tests run only the code validation section, much quicker and save d.o. resource. Then only when we agree that the changes are good, we can remove that temp change and run the full phpunit suite.

Also, re @jungle in #56

Ends with a full stop? If you agree, to search "phpcs:ignore Drupal.Semantics.FunctionTriggerError" and replace with "phpcs:ignore Drupal.Semantics.FunctionTriggerError." would be quick probably.

Before you go too far adding the periods at the end, are we sure we actually want this? Core does not currently adhere to that rule anyway, and you can see in phpcs.xml.dist
<exclude name="Drupal.Commenting.InlineComment.InvalidEndChar"/>
Does phpcs understand the sniff if it has a period at the end? Maybe try it with just one of the // phpcs:ignore lines first, don't change them all until we agree. Thanks.

Your change to core/drupalci.yml does not look right. Please see how it was done in patch #53

please review the patch.

I will when it applies OK ;-)

Nice work @Rajandro now we can see what work needs to be done.
Look back to my comment in #60 and compare patch #55 to #58. You will see my hunch was right that we should not add a period at the end of each of the // phpcs:ignore comments, as that means they are not picked up by phpcs and the line is not ignored. I did try to save @kishor_kolekar from doing this unnecessary work.

When that is done, you can address the three messages which should not be ignore, from #57

Thanks @Rajandro this is much better. You have removed the period from the end of 30 'phpcs:ignore' tags, fixed a couple of spaces after // and moved one :ignore to the correct place in SkippedDeprecationTest.php. Good work.

However, there are a few things that still need to be done before you remove the drupalci.yml temporary change:

1. The three trigger_error calls in core/modules/views/src/ViewsConfigUpdater.php should not be ignored (as I said in #57) so you need to remove those three
   // phpcs:ignore lines in that file.
2. In core/modules/system/src/Controller/SystemController.php the trigger_error has lost the @ (this was not your error, it was done in patch #58 by accident) and should be put back in.
   

   -      @trigger_error('The extension.list.module service must be passed to ...
   +      trigger_error('Calling ' . __METHOD__ . '() without $module_extension_list ...
   

3. in core/lib/Drupal/Core/DependencyInjection/DeprecatedServicePropertyTrait.php should this really be ignored? There must be a drupal version in which this deprecation was added, can that be found and added?
   

   +++ b/core/lib/Drupal/Core/DependencyInjection/DeprecatedServicePropertyTrait.php
   @@ -20,6 +20,7 @@ public function __get($name) {
        if (isset($this->deprecatedProperties[$name])) {
          $service_name = $this->deprecatedProperties[$name];
          $class_name = static::class;
   +      // phpcs:ignore Drupal.Semantics.FunctionTriggerError
          @trigger_error("The property $name ($service_name service) is deprecated in $class_name and will be removed before Drupal 10.0.0.", E_USER_DEPRECATED);
          return \Drupal::service($service_name);
        }
   

4. in core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php and core/tests/Drupal/KernelTests/Core/Theme/RegistryLegacyTest.php the @expectedDeprecation text has been changed (in some earlier patch above) from 'Unsilenced deprecation: Theme functions are deprecated in drupal:8.0.0' but the text is not changed anywhere else. The original message passes standards so there is no need for this change, and it is out of scope and should be removed.
5. In core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php the @expectedDeprecation text is changed from "Passing a 'transactions' connection option to Drupal\Core\Database\Connection::__construct" but only adds '()'. This message did not fail standards before. However there is no actual @trigger_error for this message in any other D91 file. Has the actual deprecation already been removed? Either way, we do not need to make this change.

Thanks for working on this.

Thanks @rajandro. I've compared the patches and checked the interdiff, and #73 points 1, 2, 4 and 5 have all been addressed.
Regarding #73.3 I am now not sure about this. The version may not be specific as this is a generic function which looks like it is used for many deprecations. I have queued the patch for testing.

That's good.

1. Just two errors with the new @trigger_error message, remove the period at the end of the see url in ViewsConfigUpdater.php
2. I also spotted another unrequired change in core/lib/Drupal/Core/Theme/Registry.php where the text has been changed
   

   -  trigger_error(sprintf('Theme functions are deprecated ...
   +  trigger_error(sprintf('Usage of theme functions is deprecated ....
   

   The message followed standards before, so we should not be changing it here. Sorry I did not spot that before.

Thanks.

Thanks munish.kumar.

@Rajandro you can't just add // phpcs.ignore to get round the problem. If you look closely it is because the standard requires "is" not "are". This is possibly a failing of the 'relaxed' standard, and that could be improved. However, for this issue maybe we should go back to the changed message that was in patch #75 (that is, forget my point #78.2 and put back the change as it was in patch #75)

Thanks @Rajandro this looks good. Comparing #75 to #85 is as expected. All clean regarding coding standards now.

So now looking at the 156 test failures from #82

Remaining self deprecation notices (4)
  1x: The operator defaults update for the "test_content_moderation_state_filter_base_table" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2869168
    1x in ModerationStateAccessTest::testViewShowsCorrectStates from Drupal\Tests\content_moderation\Functional

Most (if not all) must be to do with this change:

--- a/core/modules/views/src/ViewsConfigUpdater.php
+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -300,7 +300,7 @@ protected function processOperatorDefaultsHandler(array &$handler, $handler_type
     $deprecations_triggered = &$this->triggeredDeprecations['2869168'][$view->id()];
     if ($this->deprecationsEnabled && $changed && !$deprecations_triggered) {
       $deprecations_triggered = TRUE;
-      @trigger_error(sprintf('The operator defaults update for the "%s" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2869168.', $view->id()), E_USER_DEPRECATED);
+      @trigger_error(sprintf('The operator defaults update for the "%s" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2869168', $view->id()), E_USER_DEPRECATED);
     }

It would be worth trying to fix those in the next patch which runs the full test without the drupalci.yml changes. I have just searched for 2869168 in the entire 9.1.x code source and you'll find it mentioned in core/modules/views/tests/src/Kernel/ViewsConfigUpdaterTest.php and in core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php. Same goes for 2857891. Those messages need to be altered to match the same changes as the new @trigger_error text.

Interesting that this shows 156 places where core 9.1 is using the deprecated code but hiding the messages.

Thanks. It should be easy, it is just a case of altering the expected deprecation text, as explained in #87

Thanks @rajandro. Yes you fixed the messages for 2857891 and 2869168 in /DeprecationListenerTrait.php and that has had a big improvement, 156 failures down to just 4. It looked like those two were going to cover the majority of the failures.

Done a quick scan of the test failures.

• Fail 1 KernelTests\Core\Database\ConnectionTest involves 2278745
• Fail 2 KernelTests\Core\Theme\RegistryLegacyTest involves 1831138 'Theme functions are deprecated' -> 'Usage of theme functions is deprecated'
• Fail 3 Tests\views\Kernel\ViewsConfigUpdaterTest 2857891 x2, 2869168, 2900684
• Fail 4 Tests\Core\Theme\RegistryLegacyTest involves 1831138 'Theme functions are deprecated'

You can search for those numbers to find the places in the codebase where the expected deprecation messages need to be altered to match the new text. Hope that helps to give you a starter for where to find the lines to change.

Thanks @rajandro, this is good. I presume the interdiff is just named wrong, I've compared and I think it is from 85 to 93 (not 92)

The only thing I want to check is

--- a/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -116,8 +116,8 @@ public static function isDeprecationSkipped($message) {
-      '%The entity link url update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2857891.%',
-      '%The operator defaults update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2869168.%',
+      '%The entity link url update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2857891.%',
+      '%The operator defaults update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2869168.%',

I don't know exactly how the DeprecationListener works, but it would appear that the period after the two urls should be removed to match the actual message. But then, I don't know why the tests did not fail, unless of course those listeners are not actually checked for. The % is just a wild-card and I do not think that the period in .% is part of the pattern matching. Needs someone with more knowledge of DeprecationListenerTrait to answer this.

I re-tested patch #93 and it needs a re-roll

Hi ravi.shankar
When you do the re-roll, please can you also include add in the changes to drupalci.yml that are at the top of patch #85. This will mean we get quicker test results and do not waste resources on drupal.org testbot. Then only when the coding standards all pass, we can re-run without the drupalci.yml changes, to actually run the full test suite. Thanks.

Thanks for the re-roll, that's great. At least the patch applies now and with the drupalci changes we get quick coding standards output and do not waste resources.

However, you can see there are 12 trigger_error coding standards. These must be new faults in the core source code, added since patch #93 on 17 June. This shows the benefit of trying to get these coding standards fixed and then the Coder sniff enabled for Core. We now have to fix these 12 messages here, instead of on the various issues where they were introduced.

To test this sniff locally, you can run

phpcs --standard=Drupal --sniffs=Drupal.Semantics.FunctionTriggerError -s --colors .

Without any patch, we get

---------------------------------------------------------
A TOTAL OF 46 ERRORS AND 5 WARNINGS WERE FOUND IN 32 FILES
---------------------------------------------------------

and with patch #100 we get

---------------------------------------------------------
A TOTAL OF 10 ERRORS AND 2 WARNINGS WERE FOUND IN 9 FILES
---------------------------------------------------------

So at least we are fixing 39 messages. Just these 12 new ones to sort out. Who wants to take this on? Just assign yourself, so that we don't double up and duplicate effort.

Hi ankithashetty, thanks for the new patch, which fixes the 12 new warnings from #101. I have made some changes and decided to show them in three separate interdif files, to make it easier to see what has been done.

1. Your changes are all good apart from two, where you have added
// phpcs:ignore Drupal.Semantics.FunctionTriggerError when in fact the message can be corrected and we do not need to ignore it. I have made these corrections (in FormattableMarkup.php and TaxonomyIndexTid.php) and this is shown in 3048495-103.interdif-102-103-do-not-ignore.txt

2. Since your patch on 9th September there are now four new coding standards warning (one can be fixed, the other three have to be ignored). I've done this - see 3048495-103.interdif-102-103-four-new-warnings-corrected.txt

3. I have also spotted some other minor things, grammar improvements, making messages consistent, etc, and these are shown in 3048495-103.interdif-102-103-grammar-fixes.txt

There are a couple of other points outstanding, but I want to test this patch first, so let's see how it runs on drupal.org.

There's a lot going on at 9.1.x My branch was up-to-date as at 15 Oct 13:53, but the very next commit 15 Oct 14:00 (just 7 minutes later) was on #3172438: Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead. which caused the re-roll work. Talk about close timing!

Thanks for the review.

Sniffers returns 1 error

Yes, but confusingly that is nothing to do with this patch, and I have reported it on #3008028-112: Migrate D7 i18n menu links where the fault originated. This is odd, because the core/drupalci.yml file has sniff-all-files: false and that file was not touched in the patch, so should not have been checked.

I think wording needs change to "is required in drupal:10.…"

OK, I have changed that.

Also now that all texts pass the standards patch #106 will run the full set of tests, to check for any place where expected deprecations in tests are not matching the new messages.

Patch #109 fixes the message in the four failed tests from 106. Also I have removed two dynamic exception messages from DeprecationListenerTrait isDeprecationSkipped() as these are tested explicitly. Those were added in 9.0 but they may not be needed to be ignored anymore in 9.1+

OK those dynamic deprecations definitely do still need to be ignored, sorry for the noise. I have put them back in DeprecationListenerTrait.php isDeprecationSkipped(). The pattern was causing confusion earlier (see #95) where the final dot looked like part of the deprecation message but was actually useless as it was a single match-anything character. So I have removed them, and also changes the delimiter to ~ which is very obviously not a wildcard or anything in these two deprecation messages.

-      '%The entity link url update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2857891.%',
+      '~The entity link url update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2857891~',

For info, to test these locally I used --filter=PathPluginTest which is quick and contains both of the deprecations.

All the tests pass, but we now have four new @trigger_error deprecation messages to fix, all in a recent commit in #3174662-89: Encapsulate \PDOStatement instead of extending from it. It shows the benefit of getting this patch committed.

Note that I fixed two of the warning messages by changing the string variable interpolation into using sprintf(). There is an open issue in Coder to allow variable interpolation within the trigger_error text - #3178199: Cater for variable interpolation strings in @trigger_error sniff

I missed altering two expected deprecations in the tests.

All @trigger_error messages adhere to coding standards and all tests pass, so I consider this ready for review by someone else now.

The one coding standard messsage is unrelated and I've reported it on #3178338: Fix coding standard fail committed to core 9.1 and 9.2

Thanks @longwave, great review.

1. Should we just use phpcs:disable to ignore this sniff across this entire file?

   Yes definitely. I had not noticed that the single ignore had been added 8 times and that there are no other deprecations that should be checked. Fixed

2. The string interpolation is a bit weird

   I agree. I think it ended up that way because initially I just took what was inside the main string in double quotes. Fixed.

3. bindColumn and bindParam are methods and so should both end in (), as we are touching these lines let's fix this here too.

   Agreed. Done. For accuracy and completeness I have also added () to the deprecation message in function __call($method, $arguments) because that $method argument will only be the plain string

4. Does "Declaring theme functions..." sound better?

   Yes it does. Fixed.

I've made these required changes to the expected deprecation tests, but only run those specific tests locally (and they pass). Running the full core test suite locally takes too much time and resource, but hopefully all should pass on d.o.

Plus all the dots should be escaped. See the other dynamic messages skipped above in the code

In addition to escaping the dots, there should be no final dot for these two messages.

-      '%The operator defaults update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2869168~',
+      '%The operator defaults update for the "\w+" view is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described in the change record. See https://www.drupal.org/node/2869168.%',

That was the source of confusion in #95 and #111 above. The deprecation standard requires no dot at the end of the url (so that the actual usable url could be parsed out if required). The message now has no trailing dot (see core/modules/views/src/ViewsConfigUpdater.php) but the deprecation tests were not failing even when the expected message had a trailing dot. Then I realised that this ending dot simply matches any character as in standard regex syntax.

From #122

Plus all the dots should be escaped. See the other dynamic messages skipped above in the code.

Please can you escape the dots as per Alexpott's request in #122 and mentioned again in #124

Hi Pooja,

In #125 the test failed because you have lost one of the changes in core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php

In #128 the additional four test failures was because you removed dots from the messages unnecessarily, and then these did not match with the expected deprecation messages in the tests.

In #130

-      @trigger_error('Calling LibraryDiscoveryParser::__construct() without the $libraries_directory_file_finder argument is deprecated in drupal:8.9.0. The $libraries_directory_file_finder argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3099614', E_USER_DEPRECATED);
+      @trigger_error('Calling ' . __METHOD__ . '() without the $libraries_directory_file_finder argument is deprecated in drupal:8.9.0 and this argument will be required in drupal:10\.0\.0\. See https://www.drupal.org/node/3099614', E_USER_DEPRECATED);

The dots in the actual @trigger_error calls should not be escaped. You only need to escape the two dynamic text messages as explained in #120, #124 and #127. You have done it for every @trigger_error call. Also the patch does not apply, but I've not investigated exactly what is wrong.

Are you happy to continue with this task? That's fine if you do want to, but it is taking a lot of your time, and if you'd prefer me to help get it back to a working patch with all tests passing, just let me know. Or if you want to ask questions, please do, instead of just posting another patch which is not going to work. Happy to help you get this sorted :-)

Jonathan

Thank you Pooja and msuthars. This looks nearly right, although the interdiff was no help, as I could not see from which point it was diff'd from. I was expecting only a small set of changes. I manually compared the patches and I think you have fixed the problems introduced since #125.

However, as alexpott said in #122 and I repeated in #124:

all the dots should be escaped.

You need to escape all the dots, so it should be:

 in drupal:9\.0\.0 and is removed from drupal:10\.0\.0\. Module-provided Views configuration should be updated to accommodate the changes described in the change record\. See https://www\.drupal\.org/node/2869168%',

For the next patch, please could you create your interdiff against #123, as requested by alexpott, as that was the last full-reviewed patch. If you have difficulty in making interdiffs, please let me know and I can help, but I don't want to take the work from you if you are doing it.

Yes, the patch #138 looks good. I'm happy for this to be RTBC based on @longwave's RTBC from #119 everything has been addressed that was raised since then.

The interdiff was slightly odd with that single deletion and addition in core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php. I checked the actual patch and there is only one change, and that matches what we had before, so I think it was just a bug in how the interdiff was created, because that code block is repeated incorrectly in the interdiff.

The number of phpcs:ignore lines in this patch, in our own home-grown code as opposed to forked stuff, makes me think we shouldn't add this rule at all.

Just to get some perspective on this, using a quick search in /core there are 227 calls to @trigger_error.*E_USER_DEPRECATED over 84 files. This patch fixes 26 of those and ignores another 26 (over 19 files). That means the standard is already being followed for 77% of the calls and we can increse this to 88% with the patch, and only have to ignore 12%

The individual ignored lines are probably ignored for a variety of different reasons. Some may be able to be changed to follow the standards - the 'ignore' lines were added possibly casually by a variety of developers contributing to the above patches, where the easy fix was not possible. Some of these might be able to be fixed.

IIRC, one of the main points of this rule, when Gabor encourged its introduction, was to make it easy to exract deprecation data for analysis of versions, etc. In those 88% of calls this will be useful and can be done. In the remaining 12% there woud not be any info to gain anyway, because the drupal version is not included.

Should we start to look in detail at those 19 files which have the ignore, to see what can be done to improve the 88% level?

The last patch was at 9.2 and the issue is now at 9.4. Also #3226902: @trigger_error deprecation sniff is too strict - allow the message to start with a variable is fixed, so here is a plan:

1. Convert this issue to a fork with MR, to save having to create interdiffs each time
2. Change drupalci.yml to not run phpunit tests for quicker response and to save resources
3. Start with initial commit at patch #138 which had general approval
4. Fix new deprecation coding standards faults introduced since last patch
5. Remove as many phpcs:ignore as possible, and demonstrate coding standards failures
6. Change drupalci.yml to load latest Coder -dev and show that those warnings are no longer displayed
7. Run phpunit tests and correct any deprecation text assertions

:-( There are 39 new deprecation failures since the last patch. This is getting tedious to stay on top of.

#153 steps 1,2 and 3 done. Added step 4 to correct new deprecation messages.

The current 9.4.x branch has

A TOTAL OF 68 ERRORS AND 16 WARNINGS WERE FOUND IN 60 FILES

With the commit as at patch #138 (which was clean) we get

A TOTAL OF 29 ERRORS AND 10 WARNINGS WERE FOUND IN 32 FILES

I have now fixed the 39 new failures.

This commit fixes the messages I missed earlier. For some as-yet unknown reason running phpcs on my local machine ignores the file.module file completely and does not attempt to check it. Renaming to file.module.inc detected the problems, so could fix them. Then I renamed the file back to file.module. That needs investigation, but its a distraction for now.

OK. That's clean, so #153 step 4 is done. Now remove some of the phpcs:ignore Drupal.Semantics.FunctionTriggerError lines where the message is just a variable. This will show some failures because we are still using Coder 8.3.14 which does not have the that latest fix yet.

#153 step 5 is done.
As expected we get failures where the message is a variable. Now to atempt to run with the latest Coder 8.3.x dev

I would definitely recommend backporting to 9.x otherwise things like #3033165: Parse more information out of deprecation messages will have less value.