Problem/Motivation

On #3137713: [D8 only] Update deprecation notices in NodeNewComments constructor, we noticed some deprecation notices that were missing E_USER_DEPRECATED. See the discussion on that issue for the importance of setting the error code.

There are ten other deprecation notices that need a similar fix. The first two are in core/modules/config/src/Form/ConfigSync.php:

  public static function processBatch(ConfigImporter $config_importer, $sync_step, &$context) {
    @trigger_error('\Drupal\config\Form\ConfigSync::processBatch() deprecated in Drupal 8.6.0 and will be removed before 9.0.0. Use \Drupal\Core\Config\Importer\ConfigImporterBatch::process() instead. See https://www.drupal.org/node/2897299');
    ConfigImporterBatch::process($config_importer, $sync_step, $context);
  }

and

  public static function finishBatch($success, $results, $operations) {
    @trigger_error('\Drupal\config\Form\ConfigSync::finishBatch() deprecated in Drupal 8.6.0 and will be removed before 9.0.0. Use \Drupal\Core\Config\Importer\ConfigImporterBatch::finish() instead. See https://www.drupal.org/node/2897299');
    ConfigImporterBatch::finish($success, $results, $operations);
  }

All eight methods in core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php have similar code and also need to be fixed.

This issue is for D8 only since the deprecation notices, and the code that triggers them, have already been removed from Drupal 9.

Proposed resolution

In each call to trigger_error(),

  1. Add E_USER_DEPRECATED as a second argument.
  2. Change the references to "Drupal 8.6.0" and "9.0.0" to "drupal:8.6.0" and "drupal:9.0.0".

See the Drupal core deprecation policy for the definition and examples of the correct format for the deprecation notice.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#11 interdiff_8-11.txt1.74 KBankitsingh0188
#11 3138591-11.patch6.29 KBankitsingh0188
#10 3138591-10.patch1.85 KBankitsingh0188
#8 3138591-8.patch6.28 KBankitsingh0188
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

Here is a start:

$ grep -ri @trigger_error . | grep -vi E_USER_DEPRECATED 
core/modules/config/src/Form/ConfigSync.php:    @trigger_error('\Drupal\config\Form\ConfigSync::processBatch() deprecated in Drupal 8.6.0 and will be removed before 9.0.0. Use \Drupal\Core\Config\Importer\ConfigImporterBatch::process() instead. See https://www.drupal.org/node/2897299');
core/modules/config/src/Form/ConfigSync.php:    @trigger_error('\Drupal\config\Form\ConfigSync::finishBatch() deprecated in Drupal 8.6.0 and will be removed before 9.0.0. Use \Drupal\Core\Config\Importer\ConfigImporterBatch::finish() instead. See https://www.drupal.org/node/2897299');
core/modules/migrate/src/Plugin/migrate/process/DedupeEntity.php:@trigger_error('The ' . __NAMESPACE__ . ' \DedupeEntity is deprecated in
core/modules/migrate/src/Plugin/migrate/process/DedupeBase.php:@trigger_error('The ' . __NAMESPACE__ . ' \DedupeEntityBase is deprecated in
core/lib/Drupal/Core/Test/TestDiscovery.php:      // interfaces. They can be deprecated and will call @trigger_error()
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for statusCodeEquals is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for statusCodeNotEquals is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for responseHeaderEquals is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for responseHeaderNotEquals is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for responseHeaderContains is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for responseHeaderNotContains is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for responseHeaderMatches is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php:    @trigger_error('Support for responseHeaderNotMatches is to be dropped from Javascript tests. See https://www.drupal.org/node/2857562.');
core/includes/database.inc:  @trigger_error(
core/includes/database.inc:  @trigger_error(

At least the first two look like actual problems.

Probably the tests are OK.

Some of those are not deprecation notices, and the last two may be OK depending on the next couple of lines.

I will have another look within the next day unless someone beats me to it.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Parent issue: » #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code

I am not sure whether we need E_USER_DEPRECATED in core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php. Is generating E_USER_NOTICE in a test enough to make the test fail? That entire file was added in #2827014: Throw an exception when testing status code or response headers in functional JavaScript tests and the follow-up issue #2928522: \Drupal\FunctionalJavascriptTests\WebDriverWebAssert misses some deprecations. I do not see any discussion of the error level, but early versions of the patch (including the one in #2827014-41: Throw an exception when testing status code or response headers in functional JavaScript tests, which was committed and then reverted) threw exceptions instead of triggering errors.

I guess these errors are OK as is. I hope that someone can confirm when reviewing this issue.

I am adding #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code as a parent issue.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Component: other » config.module
Issue summary: View changes
Issue tags: +Novice

I checked the remaining deprecation notices listed in #2. Of the 15 uses of @trigger_error listed there,

  • 8 are in WebDriverWebAssert.php. See #3.
  • 4 do not include the full call to trigger_error(), and E_USER_DEPRECATED is used on a following line.
  • 1 is a code comment.

That leaves two to be fixed, both in core/modules/config/src/Form/ConfigSync.php. I will update the Proposed resolution in the issue summary and mark this as a Novice task. I am also setting the Component to "config.module" since that is where these deprecation notices are.

These two deprecation notices were added in #2788777: Allow a site-specific profile to be installed from existing config.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented

@benji_fisher - a normal trigger_error would still cause a test to fail, but the leading @ suppresses the triggered error, so the tests will complete. Our custom error handler finds and logs the E_USER_DEPRECATED errors inside the trigger_error call for testing purposes and to trigger tests to fail, but any other error code is ignored completely due to the error suppression by calling a function preceded with @

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Issue summary: View changes

@mikelutz:

It makes no difference whether the @trigger_error() is in the actual test class? The issues that added those deprecation notices to core/tests/Drupal/FunctionalJavascriptTests/WebDriverWebAssert.php are all about deprecation. It would be surprising if all the people working on that issue failed to notice that the errors did not work.

But I cannot think of why it would be any different in a test class, which is why I asked.

OK, I will add the 8 deprecation messages in that class to the issue description. 5x as much work, but still a Novice task.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Issue tags: +Drupal 9, +beta target

On Slack, @xjm suggested adding the "Drupal 9" and "beta target" issue tags.

ankitsingh0188’s picture

ankitsingh0188
he/him
Primary language English
CreditAttribution: ankitsingh0188 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Status: Active » Needs review
FileSize
3138591-8.patch6.28 KB

Created the patch with the latest code of the module.
Created a patch to Add missing E_USER_DEPRECATED to deprecation notices.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Issue summary: View changes
Status: Needs review » Needs work

@ankit.singh:

Thanks for taking on this issue! Even though I marked it as a Novice issue, it is pretty urgent to get this fixed, since our automated tests could miss code that uses these deprecated functions.

I reviewed the patch, and it correctly adds E_USER_DEPRECATED to all the places I identified. I repeated the test in Comment #2, and there are no other instances of @trigger_error() missing the error code.

You missed the second part of the proposed resolution, so I am setting the status to NW for that.

ankitsingh0188’s picture

ankitsingh0188
he/him
Primary language English
CreditAttribution: ankitsingh0188 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3138591-10.patch1.85 KB

Created the patch with the latest code of the module.
Created a patch to Change the references to "Drupal 8.6.0" and "9.0.0" to "drupal:8.6.0" and "drupal:9.0.0".

ankitsingh0188’s picture

ankitsingh0188
he/him
Primary language English
CreditAttribution: ankitsingh0188 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
FileSize
3138591-11.patch6.29 KB
interdiff_8-11.txt1.74 KB

Updated the patch with the latest code of the module.
Updated a patch to Change the references to "Drupal 8.6.0" and "9.0.0" to "drupal:8.6.0" and "drupal:9.0.0".

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks for the quick update!

Also, you are following the process well. Your patches are properly formed, and you remembered (both times) to set the issue status to NR.

I was about to say the following, but I see you already caught the mistake and uploaded a new patch:

However, the patch in #10 only fixes ConfigSync.php. Id does not include the changes for WebDriverWebAssert.php. I am setting the status back to NW for that.

Also, when you post an updated patch, please also attach an interdiff. In this case, ignore the patch in #10 and make an interdiff comparing to the patch in #8. There are instructions here: Creating an interdiff. Personally, I recommend using the interdiff utility. I think there is less room for making mistakes.

+1 for the interdiff comparing #8 to #11.

My last request is that you give more thought to your comments. The line "Updated the patch with the latest code of the module." is not helpful. If you made a mistake, as you did in #10, then say so, and explain why your interdiff compares to #8 instead of #10. Reviewers read these comments, and we should try to help them understand the history of the issue.

I checked that the patch in #11 fixes all the problems for t his issue. RTBC. I am updating the Remaining Tasks in the issue description.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Component: config.module » other

I swear I remember commenting on this issue before. Was there a similar issue? Very confused ATM...

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

It was #3137713: [D8 only] Update deprecation notices in NodeNewComments constructor.

Please tell me these aren't being filed on a per-module basis. See: https://www.drupal.org/core/scope#files

All of core should be fixed in a single patch.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

@xjm no, this issue is only one to fix all, as it was discussed in parent #3137713: [D8 only] Update deprecation notices in NodeNewComments constructor

  • xjm committed 25c5890 on 8.9.x 
    Issue #3138591 by ankit.singh, benjifisher, xjm, mikelutz, andypost: [D8...

  • xjm committed 03632d1 on 8.8.x 
    Issue #3138591 by ankit.singh, benjifisher, xjm, mikelutz, andypost: [D8...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Alright, I checked with the patch applied:

[ibnsina:maintainer | Thu 16:25:57] $ grep -rn "\@trigger_error" * | grep -v "E_USER_DEPRECATED" | grep -v "vendor"
core/includes/database.inc:699:  @trigger_error(
core/includes/database.inc:749:  @trigger_error(
core/lib/Drupal/Core/Test/TestDiscovery.php:283:      // interfaces. They can be deprecated and will call @trigger_error()
core/modules/migrate/src/Plugin/migrate/process/DedupeEntity.php:5:@trigger_error('The ' . __NAMESPACE__ . ' \DedupeEntity is deprecated in
core/modules/migrate/src/Plugin/migrate/process/DedupeBase.php:5:@trigger_error('The ' . __NAMESPACE__ . ' \DedupeEntityBase is deprecated

I confirmed that the first two and last two there are just weirdly formatted on multiple lines, and the middle is just a comment. Same results on 8.8.x. So, for the same reasons as #3137713: [D8 only] Update deprecation notices in NodeNewComments constructor, I've committed this to 8.9.x and cherry-picked it to 8.8.x. Thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +8.9.0 release notes, +8.8.7 release notes

Status: Fixed » Closed (fixed)

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