Problem/Motivation

> phpcs --standard=core/phpcs.xml.dist --

FILE: /app/core/lib/Drupal/Core/Theme/ThemeInitialization.php
-------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------
 138 | WARNING | Unused variable $theme_engine.
-------------------------------------------------------------------------------------

#3269154: Remove BC layers from the theme system removed the related code and the variable is no longer used.

Steps to reproduce

run composer phpcs

Proposed resolution

remove the unused variable.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3309247-9.patch699 bytes_pratik_
#3 3309247-1.patch816 bytes_pratik_

Issue fork drupal-3309247

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

neclimdul created an issue. See original summary.

pratik_specbee made their first commit to this issue’s fork.

_pratik_’s picture

_pratik_
Primary language English
Location Indore
commented
Assigned: Unassigned » _pratik_
StatusFileSize
new 3309247-1.patch816 bytes
_pratik_’s picture

_pratik_
Primary language English
Location Indore
commented
Assigned: _pratik_ » Unassigned
_pratik_’s picture

_pratik_
Primary language English
Location Indore
commented
Status: Active » Needs review
neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Reviewed & tested by the community

Perfect thanks.

+++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
@@ -135,7 +135,7 @@ public function getActiveThemeByName($theme_name) {
-    if ($theme_engine = $active_theme->getEngine()) {
+    if ($active_theme->getEngine()) {
       // Include the engine.
       include_once $this->root . '/' . $active_theme->getOwner();

For committers, this include directly following it looks like it might expose the variable to the included file but the file being included is X.engine like twig.engine. I couldn't find any documentation of this as a feature and it doesn't really make sense because the engine knows what engine it is so cleaning this up seems safe.

I thought about making this minor but left it normal since there technically is a test failure on 10.x even if testbot's optimizations are missing it. I'm really not sure why the branch tests aren't catching it, maybe a bug? Anyways, future changes to this file would run into a code style failure.

Marking RTBC assuming things will come back green.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Reviewed & tested by the community » Needs work 
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755

Missed this. Testbot caught a bad file permission change.

@pratik_specbee everything else is fine, can you remove this chunk from your patch?

_pratik_’s picture

_pratik_
Primary language English
Location Indore
commented
Assigned: Unassigned » _pratik_

Thanks for repsonse @neclimdul. I will update it.

_pratik_’s picture

_pratik_
Primary language English
Location Indore
commented
Assigned: _pratik_ » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3309247-9.patch699 bytes
neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Reviewed & tested by the community

thanks

  • lauriii committed 434ceca on 10.1.x 
    Issue #3309247 by pratik_specbee, neclimdul: Unused $theme_engine...

  • lauriii committed b4b0f9b on 10.0.x 
    Issue #3309247 by pratik_specbee, neclimdul: Unused $theme_engine...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Also noticed this on #3308987: Remove references to Stable.

Committed 434ceca and pushed to 10.1.x. Also cherry-picked to 10.0.x. Thanks!

neclimdul’s picture

neclimdul
He/Him
commented

Awesome, thanks! I think I saw that in my search but assumed it didn't have any connection based on the title. 🤦

Status: Fixed » Closed (fixed)

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