Problem/Motivation

Follow up #2729597: [meta] Replace \Drupal with injected services where appropriate in core

Proposed resolution

Replace all of them with IoC injection where possible

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 3158753-17.patch1.83 KBranjith_kumar_k_u
#5 interdiff_2-5.txt5.84 KBsuresh prabhu parkala
#5 3158753-5.patch8.71 KBsuresh prabhu parkala
#2 3158753-2.patch1.83 KBhardik_patel_12

Comments

Hardik_Patel_12 created an issue. See original summary.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Active » Needs review
StatusFileSize
new 3158753-2.patch1.83 KB

Kindly review a patch.

msuthars’s picture

msuthars
Location Pune
commented
Assigned: Unassigned » msuthars
msuthars’s picture

msuthars
Location Pune
commented
Assigned: msuthars » Unassigned
Status: Needs review » Needs work

@Hardik_Patel_12 I reviewed the patch and found some changes:
1. FileWidget is pending for Ioc service, you can inject messenger service in it.
2. FileModuleTestForm is pending for Ioc service, you can user $this->messenger() in it.
3. PrepareModulesEntityUninstallForm is pending for Ioc service, you can user $this->messenger() in it.
4. FormTestControllerObject is pending for Ioc service, you can user $this->messenger() in it.
5. PagerTestController is pending for Ioc service, you can user $this->messenger() in it.
6. BatchUserAction is pending for Ioc service, you can user $this->messenger() in it.

suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Needs work » Needs review
StatusFileSize
new 3158753-5.patch8.71 KB
new interdiff_2-5.txt5.84 KB

Updated as per #4. Please review.

hardik_patel_12’s picture

hardik_patel_12 commented

@msuthars , thanks for the review . As you have suggested some changes in #4 , can you kindly look my following points

1. In FileWidget file \Drupal::messenger( call is under static method , so no need to replace this.
2. We are replacing IoC injection in non test files only.
3. In PrepareModulesEntityUninstallForm file \Drupal::messenger( call is under static method , so no need to replace this.
4. Same as point 2.
5. Same as point 2.
6. In BatchUserAction \Drupal::messenger( call is under static method , so no need to replace this.

msuthars’s picture

msuthars
Location Pune
commented
Assigned: Unassigned » msuthars
msuthars’s picture

msuthars
Location Pune
commented
Assigned: msuthars » Unassigned
Status: Needs review » Reviewed & tested by the community

@Hardik_Patel_12 thanks to make clear about my suggestions. The patch #2 looks good to me other then that, moving to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record 
+++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
@@ -31,16 +32,17 @@ class FileWidget extends WidgetBase {
-  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, ElementInfoManagerInterface $element_info) {
+  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, ElementInfoManagerInterface $element_info, MessengerInterface $messenger) {
...
+    $this->messenger = $messenger;

We need to think about BC here. I.e. allow a null and then trigger a deprecation and use \Drupal if it is. Which means this needs a CR.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review

@alexpott , we are not injecting messenger service in FileWidget.php file as \Drupal::messenger call is in static method. Can you please review patch at #2 and points suggested in #6.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Replace usages of \Drupal::messenger() with IoC injection » Replace non-test usages of \Drupal::messenger() with IoC injection

We do this for non-test code under this issue.

Per the parent issue, rescoping this to do it for non-test code.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 3158753-17.patch1.83 KB

Rerolled #2 for 10.1

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Based on the title what is the correct approach #2 or #5 or a combo of both.

Also this was tagged for change record so moving back to NW.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.