- 🇺🇸United States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.
Seems now that the service requires arguments it will need a BC layer.
In the constructor they should default to NULL with a trigger_error that it will be required in D11
Which then requires tests for the message.Change record will also have to be added announcing this service arguments are required.
- 🇮🇳India prem suthar Ahemdabad- Gujrat , Jodhpur - Rajsthan
Re-roll the Patch For 10.1x Branch.
- First commit to issue fork.
- Status changed to Needs review
over 1 year ago 5:29am 16 December 2023 - 🇺🇸United States angrytoast PNW
Updating to a MR against 11.x and incorporating the BC
trigger_error
warnings per #21 and https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → .Clarification on tests; as of 2023-12-15, https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → states
Tests are not required where the test would only be asserting that a deprecation message is triggered
The suggested tests would fall under only asserting a deprecation message? Is it required?
- Status changed to Needs work
over 1 year ago 4:30pm 16 December 2023 - 🇺🇸United States smustgrave
So kinda,
Relooking at this I'm wondering if we have to do the backwards compatibility dance. If a contrib module could extend this class they won't have the new services we are passing into it so would need some trigger_errors
And those would need simple test cases for the exception message.
- Merge request !5841Swap \Drupal:: static calls to dependency injection → (Open) created by angrytoast
- Status changed to Needs review
over 1 year ago 7:58pm 16 December 2023 - 🇺🇸United States angrytoast PNW
Gotcha, updated the MR with a basic kernel test to assert the deprecation messaging and drafted up a change record.
That looks like it should be enough? Putting back in needs review.
- Status changed to RTBC
over 1 year ago 3:26pm 19 December 2023 - last update
over 1 year ago CI error - last update
over 1 year ago 29,722 pass - last update
over 1 year ago 29,722 pass - last update
over 1 year ago 29,722 pass - last update
over 1 year ago 29,722 pass - last update
over 1 year ago 29,722 pass - Status changed to Needs work
over 1 year ago 8:17am 30 December 2023 - 🇳🇿New Zealand quietone
I'm triaging RTBC issues → . There is no issue summary here! The only information is the title.
An issue summary really is needed no matter how simple one may think the issue is. It is what helps us at every stage of an issue to complete it and keep track of details.
--This looks like it should be part of a meta. I say that because we normally do not make such changes by file, they are scoped by the type of change. In this case there would be an issue for changing all instances of logger and all instances of state.
After a very brief search I found #2729597: [meta] Replace \Drupal with injected services where appropriate in core → where there are existing issues for logger and state. The work here needs to move to #2981326: Replace non-test usages of \Drupal::logger() with IoC injection → and 📌 Replace non-test usages of \Drupal::state() with IoC injection Needs work .
If you are unsure about scope, you can read about Drupal standard practice for how to scope issue → . And there is always the option to ask in Drupal slack.