Problem/Motivation

In #3047866: Remove usage of deprecated \Drupal::entityManager() in core we added these deprecation notices in core/modules/comment/src/Plugin/views/field/NodeNewComments.php:

  public function __construct(array $configuration, $plugin_id, $plugin_definition, Connection $database, EntityTypeManagerInterface $entity_type_manager = NULL, EntityFieldManagerInterface $entity_field_manager = NULL) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->database = $database;
    if (!$entity_type_manager) {
      @trigger_error("Not passing the entity type manager to the NodeNewComments constructor is deprecated in drupal:8.8.0 and will be required in drupal 9.0.0. @see https://www.drupal.org/node/3047897");
      $entity_type_manager = \Drupal::entityTypeManager();
    }
    if (!$entity_field_manager) {
      @trigger_error("Not passing the entity type manager to the NodeNewComments constructor is deprecated in drupal:8.8.0 and will be required in drupal 9.0.0. @see https://www.drupal.org/node/3047897");
      $entity_field_manager = \Drupal::service('entity_field.manager');
    }

It looks like a little too much copy/paste: the second deprecation notice should refer to "entity field manager" instead of "entity type manager".

Proposed resolution

Change "entity type manager" to "entity field manager" in the second deprecation notice.

Update both deprecation notices to current standards:

  • Change "drupal 9.0.0" to "drupal:9.0.0".
  • Add E_USER_DEPRECATED to both calls to trigger_error().

See the Drupal core deprecation policy.

This only applies to the 8.9.x branch, since the parameters are required, and the deprecation notices removed, in Drupal 9.

Remaining task(s)

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

Issue tags: +Novice

I forgot to tag this as a Novice issue.

This tag means that experienced contributors should spend their time on more challenging tasks. Let a novice contributor use this issue to get experience making a patch and working with the issue queue.

jyotimishra-developer’s picture

Assigned: Unassigned » jyotimishra-developer
jyotimishra-developer’s picture

Assigning to myself, as i am working on this.

jyotimishra-developer’s picture

jyotimishra-developer’s picture

Assigned: jyotimishra-developer » Unassigned
Status: Active » Needs review
benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

@jyotimishra123:

It looks as though this is your first contribution to Drupal. Welcome to the community of Drupal contributors!

The patch is in the correct format. You followed the naming convention for the patch. You remembered to assign/unassign yourself and to change the issue status. All good.

I think you are ready to move on to issues where the patch is more consequential. Perhaps you can participate in the upcoming Drupal 9 Porting Weekend May 22-23, 2020. See also Preparing yourself for Drupal 9 porting weekend.

The patch does what it is supposed to do. RTBC.

mikelutz’s picture

Status: Reviewed & tested by the community » Needs work

This is good, but we need to fix the message formats while we are in here. There needs to be a : between drupal and 9.0.0 in both messages.

benjifisher’s picture

Title: Fix incorrect reference in NodeNewComments constructor » Update deprecation notices in NodeNewComments constructor
Issue summary: View changes

@mikelutz:

Thanks for pointing that out. Since we are expanding scope, I am updating the issue title. I think this is still a novice task.

mohrerao’s picture

updated messages from drupal 9.0.0 to drupal:9.0.0

mohrerao’s picture

Status: Needs work » Needs review
andypost’s picture

Status: Needs review » Needs work

It missing E_USER_DEPRECATED in trigger error

mohrerao’s picture

@andypost, Thanks for the swift response. Added E_USER_DEPRECATED in trigger error

mohrerao’s picture

Status: Needs work » Needs review
benjifisher’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

@mohrerao:

It looks as though this is your first contribution to Drupal core. Thanks for helping! Everything I said in #7 applies here.

I am also adding a link to the deprecation policy in the issue summary. The wording of the error messages here does not quite match the examples in the policy, but I think that we have fixed the important parts: the type of error, the format of the project/version, and (most important IMO) the correct missing argument.

Back to RTBC.

jyotimishra-developer’s picture

Hi @benjifisher, Hope you doing well!!
Thank you so much for your appreciation.

xjm’s picture

Version: 8.9.x-dev » 8.8.x-dev

Oops.

Normally we'd relegate any malformed deprecations to 9.1.x only (for removal in D10), but this is code that's already been changed in 9.0, and we were still were throwing an error -- just the wrong kind of error.

Committed to 8.9.x. Setting RTBC against 8.8.x because there is a slight disruption -- the change from drupal 9.0.0 to drupal:9.0.0 means that there's a slightly different expected deprecation message, which could disrupt contrib tests or the like. I'll get a second opinion on whether to backport it to 8.8.x this late.

  • xjm committed a168dc7 on 8.9.x
    Issue #3137713 by mohrerao, jyotimishra123, benjifisher, mikelutz,...
xjm’s picture

Title: Update deprecation notices in NodeNewComments constructor » [Backport, D8-only] Update deprecation notices in NodeNewComments constructor
mikelutz’s picture

-1 on the backport. This is a cosmetic change to tests, but not fixing an actual bug. We got a new contributor or two to fix it for the upcoming release, and I like it being tidy for the LTS, but I don't see a reason to backport it to a version about to enter security only support. Thanks, xjm!

benjifisher’s picture

I was thinking the same thing, but I am less familiar with the deprecation policy than mikelutz, so I hesitated to chime in.

The closest this issue comes to being a bug is that

  1. You might get an error notice about "entity type manager" instead of "entity field manager" (because of copy/paste error).
  2. You would get the wrong error level.
xjm’s picture

We could also potentially make a backport version that adds the E_USER_DEPRECATED, but not the colon. (Still haven't had a chance to chat with @catch about it.)

mikelutz’s picture

You know what, benji, I take back my earlier comment. +1 for backporting.

These deprecation messages didn't have the right code in their trigger error. So any tests using expectedDeprecation weren't catching it to begin with. xjms concern that we were breaking expected deprecation test is incorrect because we weren't throwing a deprecation.

So technically, we are adding a deprecation here, which we are way way way too late to do, but we thought we already did it, and the change to require the arguments is already in D9, so we need to start surfacing the deprecation message as far back as we can, so any extending classes are aware of it. I don't imagine this will actually impact many people, but better to break tests in 8.8 than to break code, and better to break tests in 8.8 than to have people think their code is d9 ready if it isn't.

mikelutz’s picture

Agreed, xjm, but if we weren't triggering the right error code, then the message doesn't matter, so just backport the whole thing, I think. But yeah, have catch weigh in too.

mikelutz’s picture

BTW the messages are still slightly off from the template, but again, better to trigger the error as a deprecation than not, imho.

  • xjm committed 3d8f0d1 on 8.8.x
    Issue #3137713 by mohrerao, jyotimishra123, benjifisher, mikelutz,...
xjm’s picture

Title: [Backport, D8-only] Update deprecation notices in NodeNewComments constructor » [D8 only] Update deprecation notices in NodeNewComments constructor
Status: Reviewed & tested by the community » Fixed

Discussed with @catch and we agreed to backport it. Cherry-picked to 8.8.x. Thanks!

benjifisher’s picture

Let me see if I understand.

I created this issue because I noticed a copy/paste error that could lead to an inaccurate error message. That is barely worth fixing for 8.9.x (LTS).

More important is what @andypost pointed out in #12: the missing E_USER_DEPRECATED. The default for trigger_error() is E_USER_NOTICE.

Because we suppress the error under normal usage (@trigger_error()) the only time anyone sees this error is when they are specifically looking for a particular error level: that is why we set E_USER_DEPRECATED. No one ever looks for all E_USER_NOTICE.


These deprecation notices were added in #3047866: Remove usage of deprecated \Drupal::entityManager() in core. I checked that patch: it includes one other deprecation notice, and that one includes E_USER_DEPRECATED.

I guess it is worth looking for other deprecation notices that do not have E_USER_DEPRECATED. I have to do some paid work now, but I will open another issue when I can spare some time.

mikelutz’s picture

It's one of the reasons why there is supposed to be a test for all deprecation error triggers. I can't tell you how many times I wrote a patch to add one and forgot the E_USER_DEPRECATED and only caught myself because I couldn't get my expected deprecation test to pass. We haven't always been as strict about the tests as we are now, but some constructor changes that trigger errors still don't get tests because they are considered trivial. We definitely didn't write tests for all the constructor changes around the entityManager split either, and that was intentional due to the massive scope.

But yes, your assessment is correct. Not having the E_USER_DEPRECATED makes the trigger_error essentially useless in its intended purpose, which is warning people that their code is not D9 compatible. These errors are important, particularly in constructor signature additions like this where static analysis and docblock annotations aren't able to inform.

benjifisher’s picture

I already added #3138591: [D8 only] Add missing E_USER_DEPRECATED to deprecation notices. I found 15 candidates, of which 2 seem like problems. Most of the others are probably OK.

xjm’s picture

Status: Fixed » Closed (fixed)

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