Problem/Motivation

ThemeManagerInterface getActiveTheme parameters do not match ThemeManager.

\Drupal\Core\Theme\ThemeManager::getActiveTheme():

  /**
   * {@inheritdoc}
   */
  public function getActiveTheme(RouteMatchInterface $route_match = NULL) {
    if (!isset($this->activeTheme)) {
      $this->initTheme($route_match);
    }
    return $this->activeTheme;
  }

\Drupal\Core\Theme\ThemeManagerInterface::getActiveTheme():

  /**
   * Returns the active theme object.
   *
   * @return \Drupal\Core\Theme\ActiveTheme
   */
  public function getActiveTheme();

Proposed resolution

Change \Drupal\Core\Theme\ThemeManagerInterface::getActiveTheme()'s signature to match ThemeManager's signature.

Remaining tasks

User interface changes

API changes

ThemeManagerInterface method signature changes from:

getActiveTheme()

to

getActiveTheme(RouteMatchInterface $route_match = NULL)

However this is allowable under the 1-1 rule for interfaces.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#4 interdiff-2-4.txt544 byteshardik_patel_12
#4 3160307-4.patch906 byteshardik_patel_12
#2 3160307-2.patch892 bytesjungle

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Signature mismatch between ThemeManagerInterface::getActiveTheme() and ThemeManager::getActiveTheme » Signature of getActiveTheme() mismatches between ThemeManagerInterface and ThemeManager
Status: Active » Needs review
StatusFileSize
new 3160307-2.patch892 bytes
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative, +Novice

Thanks for the issue and patch.

1) Patch applies cleanly.

2) Tests pass.

3) Looking for all getActiveTheme functions, I see:

./core/lib/Drupal/Core/Template/TwigExtension.php:  public function getActiveTheme() {
./core/lib/Drupal/Core/Theme/ThemeManager.php:  public function getActiveTheme(RouteMatchInterface $route_match = NULL) {
./core/lib/Drupal/Core/Theme/ThemeManagerInterface.php:  public function getActiveTheme();
./core/lib/Drupal/Core/Theme/ThemeInitialization.php:  public function getActiveTheme(Extension $theme, array $base_themes = []) {
./core/lib/Drupal/Core/Theme/ThemeInitializationInterface.php:  public function getActiveTheme(Extension $theme, array $base_themes = []);

where ThemeInitialization parameters match ThemeInitializationInterface and TwigExtension does not have a Drupal interface defining getActiveTheme. From this, only ThemeManagerInterface needs updating per the issue summary.

4) After patching, the parameters match.

5) Looking at other doc blocks with RouteMatchInterface params, it looks like it should be updated to include the namespace.

+++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
@@ -29,9 +31,12 @@ public function render($hook, array $variables);
+   * @param RouteMatchInterface|null $route_match

I would change RouteMatchInterface to \Drupal\Core\Routing\RouteMatchInterface.

6) Also, most core code uses The route match. rather than The route match object. so I would change this as well.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review
StatusFileSize
new 3160307-4.patch906 bytes
new interdiff-2-4.txt544 bytes

Updating @param comments for getActiveTheme function , kindly review.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Issue tags: +DCCO2020

Reviewing this for DrupalCamp Colorado contribution day.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Title: Signature of getActiveTheme() mismatches between ThemeManagerInterface and ThemeManager » ThemeManagerInterface getActiveTheme parameters do not match ThemeManager
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks for update. Marking RTBC based on the following and #3.

1) Patch still applies cleanly.

2) Tests pass and additional tests are not needed.

3) Changes in #4 address items in #3.

4) Title and issue summary have been updated for clarity.

dww’s picture

dww
we/he/they
commented

Does this need a CR?

Otherwise, RTBC +1.

Thanks,
-Derek

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I traced the lineage of this.

The original interface and theme manager went in with #2228093: Modernize theme initialization.

This issue existed in that original issue, so they were divergent from the onset.

I checked for both 'implements ThemeManagerInterface' and 'extends ThemeManagerInterface' in http://grep.xnddx.ru - and there were no results.

However, this doesn't mean there is another implementation in some-one's custom project.

And unfortunately, this change would break that - see here https://3v4l.org/JeoS4

So I don't think we can do this without a BC break.

I'll poll other committers, but I think this will have to be closed won't fix and filed under technical debt. Postponing to poll other committers.

edit this might come in under our 1:1 rule

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Postponed (maintainer needs more info) » Needs review

Using NR instead, as I think this might come in under the 1:1 rule

catch’s picture

catch
he/him
Primary language English
commented

I think this is OK under the 1:1 rule, especially given there's no sign of anyone implementing the interface.

The other option would be to add the parameter commented out, with a note that it will be part of the interface in 10.0.x and do the bc break then, but that seems unnecessary here.

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.

quietone’s picture

quietone commented

What is the next step here?

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Reviewed & tested by the community

Given the explanation in #10, moving back to RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yeah I think this is fine.

We needed a change record, which I've added here: https://www.drupal.org/node/3213807

Committed e821dc5 and pushed to 9.3.x. Thanks!

Not backporting to 9.2.x or earlier - don't think we need to and in the unlikely event that there is contrib/custom code broken by this, gives more time to find out.

  • catch committed e821dc5 on 9.3.x 
    Issue #3160307 by Hardik_Patel_12, jungle, Kristen Pol, larowlan, catch...
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

Status: Fixed » Closed (fixed)

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