ThemeInstaller depends on ModuleDependencyMessageTrait which is in the system module

Created on 27 October 2020, over 4 years ago
Updated 6 July 2025, 1 day ago

Problem/Motivation

lib/Drupal/Core/Extension/ThemeInstaller.php
15:use Drupal\system\ModuleDependencyMessageTrait;
23:  use ModuleDependencyMessageTrait;

This introduces a dependency on a module in a core component, which shouldn't happen.

Steps to reproduce

Look at the code ;)

I'm also getting failures due to this in kernel tests that don't install system module, but not in all cases -- not sure of what the exact conditions are to fail.

Proposed resolution

Remove the use of ModuleDependencyMessageTrait here. The way it's used is really weird:

          if ($incompatible = $this->checkDependencyMessage($module_list, $dependency, $dependency_object)) {
            $sanitized_message = Html::decodeEntities(strip_tags($incompatible));
            throw new MissingDependencyException("Unable to install theme: $sanitized_message");
          }

We're in an API class, calling something that produces UI messages, then stripped the tags from those messages to use them in an exception message.... that seems quite messy!

Remaining tasks

Add a helper method to ThemeInstaller to produce the different exception messages. Remove the trait from ThemeInstaller.

User interface changes

None.

API changes

None.

Data model changes

None

Release notes snippet

N/A

πŸ› Bug report
Status

Postponed: needs info

Version

11.0 πŸ”₯

Component

extension system

Created by

πŸ‡¬πŸ‡§United Kingdom joachim

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    The trait is now in the extension namespace.

    This could use an issue summary update and maybe looking at some other usages to see about the strip tags bit.

    That is a bit weird but I can see why we would reuse this, but I would want to compare to see if it is worth refactoring.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Let's close this as outdated, as the main point was about the weird dependencies. Now that trait has been moved, that's no longer a problem.

    I'll open a new issue about the strip_tags() thing.

Production build 0.71.5 2024