Comments

jungle created an issue. See original summary.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

nitesh624’s picture

Assigned: Unassigned » nitesh624
nitesh624’s picture

StatusFileSize
new34.15 KB
nitesh624’s picture

Assigned: nitesh624 » Unassigned
Status: Active » Needs review
jungle’s picture

Status: Needs review » Needs work

Thanks 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 possible

Please see the parent issue and closed/fixed sibling issues for reference and for more.

nitesh624’s picture

Assigned: Unassigned » nitesh624
nitesh624’s picture

StatusFileSize
new40.89 KB
new8.88 KB

impletemented suggestion a per #6

nitesh624’s picture

Assigned: nitesh624 » Unassigned
Status: Needs work » Needs review
nitesh624’s picture

StatusFileSize
new37.9 KB
new4.96 KB

Status: Needs review » Needs work

The last submitted patch, 10: 3123210-10.patch, failed testing. View results

nitesh624’s picture

StatusFileSize
new39.24 KB
new2.2 KB
nitesh624’s picture

Status: Needs work » Needs review
jungle’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
sanjayk’s picture

Assigned: Unassigned » sanjayk
sanjayk’s picture

Assigned: sanjayk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new37.48 KB

I have reroll the patch

hardik_patel_12’s picture

StatusFileSize
new38.36 KB
new2.15 KB

Replacing \Drupal::theme() with \Drupal::service('theme.manager') in ThemeTest file. Kindly review a patch.

hardik_patel_12’s picture

StatusFileSize
new2.88 KB

Kindly 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.

hardik_patel_12’s picture

Issue tags: -Needs reroll
hardik_patel_12’s picture

StatusFileSize
new3.19 KB
new1.29 KB

Adding change record and deprecation error also. Kindly review a new patch.

Status: Needs review » Needs work

The last submitted patch, 20: 3123210-20.patch, failed testing. View results

hardik_patel_12’s picture

Status: Needs work » Needs review

Unrelated failed test case comes , retested the last patch.

hardik_patel_12’s picture

StatusFileSize
new7.45 KB
new4.05 KB

There are a few more instances that can be injected:

grep -nri '\Drupal::theme()' core | grep -v .module | grep -vi test | grep -v .api.php | grep -v authorize.php | grep -v .inc
core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php:264:    $regions = \Drupal::theme()->getActiveTheme()->getRegions();
core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php:305:    \Drupal::theme()->alter('page_attachments', $attachments);

Injecting theme manager service in HtmlRenderer.php file and adding Change Record and deprecation error also.
Kindly review a new patch.

pratik_kamble’s picture

Issue tags: +DIACWJuly2020
jungle’s picture

Title: Replace usages of \Drupal::theme() with IoC injection » Replace non-tests usages of \Drupal::theme() with IoC injection
Related issues: +#2780397: Replace non-test usages of \Drupal::theme() with IoC injection

Adding 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.

Constructors for service objects, plugins, and controllers
The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

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.

jungle’s picture

Status: Needs review » Needs work
jameszhang023’s picture

Assigned: Unassigned » jameszhang023

Working on this.

jameszhang023’s picture

Working on this.

jungle’s picture

Title: Replace non-tests usages of \Drupal::theme() with IoC injection » Replace non-test usages of \Drupal::theme() with IoC injection
jameszhang023’s picture

Assigned: jameszhang023 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new6.84 KB
new1.66 KB

Review please, thanks.

jungle’s picture

Status: Needs review » Reviewed & tested by the community

@Hardik_Patel_12, @jameszhang023, thanks for the working on this.

$ git grep "Drupal::theme()" | grep -iv test.php | grep -iv .module | grep -iv .inc
core/tests/Drupal/KernelTests/AssertContentTrait.php:      return \Drupal::theme()->render($callback, $variables);

No more \Drupal::theme() in scope

$ git grep "('theme.manager')" | grep -iv test.php | grep -iv .module | grep -iv .inc
core/lib/Drupal.php:    return static::getContainer()->get('theme.manager');
core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php:    return \Drupal::service('theme.manager');

No more \Drupal::service('theme.manager') in scope

It's good to go to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -97,8 +105,10 @@ class HtmlRenderer implements MainContentRendererInterface {
-  public function __construct(TitleResolverInterface $title_resolver, PluginManagerInterface $display_variant_manager, EventDispatcherInterface $event_dispatcher, ModuleHandlerInterface $module_handler, RendererInterface $renderer, RenderCacheInterface $render_cache, array $renderer_config) {
+  public function __construct(TitleResolverInterface $title_resolver, PluginManagerInterface $display_variant_manager, EventDispatcherInterface $event_dispatcher, ModuleHandlerInterface $module_handler, RendererInterface $renderer, RenderCacheInterface $render_cache, array $renderer_config, ThemeManagerInterface $theme_manager = NULL) {

@@ -106,6 +116,7 @@ public function __construct(TitleResolverInterface $title_resolver, PluginManage
+    $this->themeManager = $theme_manager;

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -64,12 +72,15 @@ class ViewEditForm extends ViewFormBase {
-  public function __construct(SharedTempStoreFactory $temp_store_factory, RequestStack $requestStack, DateFormatterInterface $date_formatter, ElementInfoManagerInterface $element_info) {
+  public function __construct(SharedTempStoreFactory $temp_store_factory, RequestStack $requestStack, DateFormatterInterface $date_formatter, ElementInfoManagerInterface $element_info, ThemeManagerInterface $theme_manager = NULL) {
...
+    $this->themeManager = $theme_manager;

In Drupal 9 this argument needs to be optional. In Drupal 10 we can require it. We need to do something like:

    if ($theme_manager === NULL) {
      @trigger_error('The theme.manager service must be passed to ' . __NAMESPACE__ . '\HtmlRenderer::__construct(). It was added in drupal:9.1.0 and will be required before drupal:10.0.0.', E_USER_DEPRECATED);
      $theme_manager = \Drupal::service('theme.manager');
    }
jungle’s picture

@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.

  1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -106,6 +116,11 @@ public function __construct(TitleResolverInterface $title_resolver, PluginManage
    +    if (!$theme_manager) {
    
    +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -64,12 +72,19 @@ class ViewEditForm extends ViewFormBase {
    +    if (!$theme_manager) {
    

    Prefer $theme_manager === NULL instead of !$theme_manager

  2. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -106,6 +116,11 @@ public function __construct(TitleResolverInterface $title_resolver, PluginManage
    +      @trigger_error('Calling ' . __METHOD__ . ' without the $theme_manager argument is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3136769', E_USER_DEPRECATED);
    

    The 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

  3. +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -64,12 +72,19 @@ class ViewEditForm extends ViewFormBase {
    +      @trigger_error('Calling ' . __METHOD__ . ' without the $theme_manager argument is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3136769', E_USER_DEPRECATED);
    

    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

jameszhang023’s picture

Status: Needs work » Needs review
StatusFileSize
new7.46 KB
new2.14 KB

Thanks @alexpott and @jungle for your review. This patch is based on #23, please review again, thanks.

Status: Needs review » Needs work

The last submitted patch, 35: 3123210-35.patch, failed testing. View results

jungle’s picture

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -80,8 +80,8 @@ public function __construct(SharedTempStoreFactory $temp_store_factory, RequestS
+    if (!$$theme_manager === NULL) {

Forgot to remove !$?

jameszhang023’s picture

Status: Needs work » Needs review
StatusFileSize
new7.46 KB
new819 bytes

Oh, sorry, thanks you @jungle for finding the problem. Please review again.

jungle’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @jameszhang023. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4cb745d and pushed to 9.1.x. Thanks!

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -64,12 +72,19 @@ class ViewEditForm extends ViewFormBase {
+    if ($theme_manager === NULL) {

Given a type hint of ThemeManagerInterface $theme_manager = NULL there is no difference between if ($theme_manager === NULL) { and if (!$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.

diff --git a/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
index 39a3ab8ab6..66af0a82d4 100644
--- a/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -15,9 +15,9 @@
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\Render\RenderEvents;
 use Drupal\Core\Routing\RouteMatchInterface;
+use Drupal\Core\Theme\ThemeManagerInterface;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\HttpFoundation\Request;
-use Drupal\Core\Theme\ThemeManagerInterface;
 
 /**
  * Default main content renderer for HTML requests.
diff --git a/core/modules/views_ui/src/ViewEditForm.php b/core/modules/views_ui/src/ViewEditForm.php
index a53b9ead32..c4a5a94b33 100644
--- a/core/modules/views_ui/src/ViewEditForm.php
+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -11,13 +11,13 @@
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Link;
 use Drupal\Core\Render\ElementInfoManagerInterface;
-use Drupal\Core\Url;
 use Drupal\Core\TempStore\SharedTempStoreFactory;
+use Drupal\Core\Theme\ThemeManagerInterface;
+use Drupal\Core\Url;
 use Drupal\views\Views;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException;
-use Drupal\Core\Theme\ThemeManagerInterface;
 
 /**
  * Form controller for the Views edit form.

Changed the use statements to be alphabetical on commit.

  • alexpott committed 4cb745d on 9.1.x
    Issue #3123210 by nitesh624, Hardik_Patel_12, jameszhang023, sanjayk,...

Status: Fixed » Closed (fixed)

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