Closed (fixed)
Project:
Drupal core
Version:
9.1.x-dev
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
29 Mar 2020 at 04:06 UTC
Updated:
4 Aug 2020 at 08:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #3
nitesh624Comment #4
nitesh624Comment #5
nitesh624Comment #6
jungleThanks for working on this.
It's not just replacing
\Drupal::theme()with\Drupal::service('theme.manager')It's to replace
\Drupal::theme()and\Drupal::service('theme.manager')with IoC injection where possiblePlease see the parent issue and closed/fixed sibling issues for reference and for more.
Comment #7
nitesh624Comment #8
nitesh624impletemented suggestion a per #6
Comment #9
nitesh624Comment #10
nitesh624Comment #12
nitesh624Comment #13
nitesh624Comment #14
jungleComment #15
sanjayk commentedComment #16
sanjayk commentedI have reroll the patch
Comment #17
hardik_patel_12 commentedReplacing \Drupal::theme() with \Drupal::service('theme.manager') in ThemeTest file. Kindly review a patch.
Comment #18
hardik_patel_12 commentedKindly ignore patch #17, replacing \Drupal::theme() and \Drupal::service('theme.manager') with IoC injection where possible , that's why no interdiff. Kindly review a new patch.
Comment #19
hardik_patel_12 commentedComment #20
hardik_patel_12 commentedAdding change record and deprecation error also. Kindly review a new patch.
Comment #22
hardik_patel_12 commentedUnrelated failed test case comes , retested the last patch.
Comment #23
hardik_patel_12 commentedThere are a few more instances that can be injected:
Injecting theme manager service in HtmlRenderer.php file and adding Change Record and deprecation error also.
Kindly review a new patch.
Comment #24
pratik_kambleComment #25
jungleAdding a relevant fixed issue #2780397: Replace non-test usages of \Drupal::theme() with IoC injection, after reading the parent issue again, I realised i did not read the "Proposed resolution" carefully in the parent issue, Especially, We do this for non-test code under this issue.. Sorry about that.
So I am rescoping this to do for non test code.
As to BC concerns, per #2780397: Replace non-test usages of \Drupal::theme() with IoC injection and per Drupal 8 and 9 backwards compatibility and internal API policy (backend), BC should be ignored, so the CR is unnecessary, in #23
Thanks everyone for your efforts so far.
Comment #26
jungleComment #27
jameszhang023 commentedWorking on this.
Comment #28
jameszhang023 commentedWorking on this.
Comment #29
jungleComment #30
jungleFiled a relevant issue #3160307: ThemeManagerInterface getActiveTheme parameters do not match ThemeManager
Comment #31
jameszhang023 commentedReview please, thanks.
Comment #32
jungle@Hardik_Patel_12, @jameszhang023, thanks for the working on this.
No more
\Drupal::theme()in scopeNo more
\Drupal::service('theme.manager')in scopeIt's good to go to me.
Comment #33
alexpottIn Drupal 9 this argument needs to be optional. In Drupal 10 we can require it. We need to do something like:
Comment #34
jungle@alexpott, Thanks for your review/confirmation. Then BC matters, Actually, I misunderstood the doc, "Constructors for service objects, plugins, and controllers...", here are consumers of "service objects"
Then let's work again from #23.
Prefer
$theme_manager === NULLinstead of!$theme_managerThe right CR(Change record) url is https://www.drupal.org/node/3159762. Change https://www.drupal.org/node/3136769 to https://www.drupal.org/node/3159762
The right CR url is https://www.drupal.org/node/3159506. Change https://www.drupal.org/node/3136769 to https://www.drupal.org/node/3159506
Comment #35
jameszhang023 commentedThanks @alexpott and @jungle for your review. This patch is based on #23, please review again, thanks.
Comment #37
jungleForgot to remove
!$?Comment #38
jameszhang023 commentedOh, sorry, thanks you @jungle for finding the problem. Please review again.
Comment #39
jungleThanks @jameszhang023. Back to RTBC.
Comment #40
alexpottCommitted 4cb745d and pushed to 9.1.x. Thanks!
Given a type hint of
ThemeManagerInterface $theme_manager = NULLthere is no difference betweenif ($theme_manager === NULL) {andif (!$theme_manager) {apart from about lines and working out if the === means anything. In this case it does not. That might possibly change if https://wiki.php.net/rfc/objects-can-be-falsifiable lands but at this point that feels unlikely.Changed the use statements to be alphabetical on commit.