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 totrigger_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
Comment | File | Size | Author |
---|---|---|---|
#13 | 3137713-13.patch | 1.58 KB | mohrerao |
#10 | interdiff_5-10.txt | 1.47 KB | mohrerao |
#10 | 3137713-10.patch | 1.54 KB | mohrerao |
Comments
Comment #2
benjifisherI 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.
Comment #3
jyotimishra-developer CreditAttribution: jyotimishra-developer at Srijan | A Material+ Company for Drupal India Association commentedComment #4
jyotimishra-developer CreditAttribution: jyotimishra-developer at Srijan | A Material+ Company for Drupal India Association commentedAssigning to myself, as i am working on this.
Comment #5
jyotimishra-developer CreditAttribution: jyotimishra-developer at Srijan | A Material+ Company for Drupal India Association commentedComment #6
jyotimishra-developer CreditAttribution: jyotimishra-developer at Srijan | A Material+ Company for Drupal India Association commentedComment #7
benjifisher@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.
Comment #8
mikelutzThis 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.
Comment #9
benjifisher@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.
Comment #10
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedupdated messages from drupal 9.0.0 to drupal:9.0.0
Comment #11
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #12
andypostIt missing
E_USER_DEPRECATED
in trigger errorComment #13
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@andypost, Thanks for the swift response. Added E_USER_DEPRECATED in trigger error
Comment #14
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #15
benjifisher@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.
Comment #16
jyotimishra-developer CreditAttribution: jyotimishra-developer at Srijan | A Material+ Company for Drupal India Association commentedHi @benjifisher, Hope you doing well!!
Thank you so much for your appreciation.
Comment #17
xjmOops.
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
todrupal: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.Comment #19
xjmComment #20
mikelutz-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!
Comment #21
benjifisherI 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
Comment #22
xjmWe 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.)Comment #23
mikelutzYou 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.
Comment #24
mikelutzAgreed, 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.
Comment #25
mikelutzBTW the messages are still slightly off from the template, but again, better to trigger the error as a deprecation than not, imho.
Comment #27
xjmDiscussed with @catch and we agreed to backport it. Cherry-picked to 8.8.x. Thanks!
Comment #28
benjifisherLet 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 fortrigger_error()
isE_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 setE_USER_DEPRECATED
. No one ever looks for allE_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.Comment #29
mikelutzIt'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.
Comment #30
benjifisherI 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.
Comment #31
xjm