Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Issue #2151123 by joelpittet, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Remove theme_system_modules_incompatible()
Problem/Motivation
theme_system_modules_incompatible() is just a div wrapping text and probably doesn’t need to be a theme function. It’s only used once in core.
Proposed resolution
Remove theme_system_modules_incompatible() and refactor the render array that uses it. @see \Drupal\system\Form\ModulesListForm::buildRow().
Remaining tasks
- Patch
- Patch review
- Manual testing
Steps to test
- Change the version in any module .info.yml to something other than 8.x. Any of these work: 7.x, 9.x, 8.9.
- Clear caches.
- Visit /admin/modules.
User interface changes
n/a
API changes
The removal of the theme_system_modules_incompatible() theme function.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | after.jpg | 222.58 KB | peri22 |
| #12 | before.jpg | 206.71 KB | peri22 |
| #11 | interdiff.txt | 1.58 KB | joelpittet |
| #11 | 2151123-remove-theme_system_modules_incompatible-11.patch | 2.42 KB | joelpittet |
| #11 | incompatible-module-row.png | 299.16 KB | joelpittet |
Comments
Comment #1
star-szrAdding a commit message to the issue summary so the folks who already worked on #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.
Comment #2
star-szrActually adding the commit message this time :)
Comment #3
joelpittetSplit from the system module twig conversion meta.
Comment #4
star-szrComment #5
star-szrThis would be a great one for a new contributor to review and do manual testing on to make sure we're preserving the same markup (whitespace doesn't matter). Posting the before/after HTML in
<code>tags is quite helpful or if they are big chunks of markup they can be attached as .txt files.I suspect that testing this might be as easy as dropping a 7.x module in /modules and navigating to admin/modules.
Comment #6
star-szrAdding a placeholder for testing steps to the issue summary. This should have clear steps to reproduce.
Comment #7
star-szrManually tested by changing the Display Suite .info.yml:
It checks out but the markup is slightly different.
Before:
<div class="incompatible">This version is not compatible with Drupal 8.x and should be replaced.This module requires PHP version 5.6.* and is incompatible with PHP version 5.4.26.</div>After:
<div class="incompatible form-wrapper" id="edit-modules-display-suite-ds-description">This version is not compatible with Drupal 8.x and should be replaced.This module requires PHP version 5.6.* and is incompatible with PHP version 5.4.26.</div>Is it time for #type wrapper? :)
Assigning to @joelpittet to get his take on this. The markup change certainly doesn't break anything but it seems a bit undesirable to me to have that extra class and ID. Also updated the issue summary with testing steps.
Comment #8
star-szrAdding a fun little spin-off issue as related.
Comment #9
joelpittetWoah well that sucks.
I think it would be super confusing to have a container and a wrapper type.
I'd much rather make the
#type=>'container'be#type=>'form_container', and clean up the namespace for the generic#type=>'container'. It's confusing have a generic template and name for a specific use case like that.But for this specific one, why is there a div in the first place, let's just put that class on the containing cell with #wrapper_attributes and call it a day?
Comment #10
joelpittetIgnore my last suggestion about #wrapper_attributes... that works for #type table cells only.
How about if the 'incompatible' class was attached to the 'summary_attributes' in some how, also that '' same goes for that class too? Though from theming prespective 'incompatible' class should apply to the row so any element within the row could be targed through CSS and not just the title.
As it stands we have a div in a span... which is not cool!
Comment #11
joelpittetExample easier stylings.
Comment #12
peri22 commentedIt works.
I've attached 2 picture, before and after patch.
Comment #13
joelpittetAwesome thanks peri22:) Removing Tags. Hope this helps people over at #2227687: Slightly ugly error message if a module is incompatible with the version of core and the PHP version
Comment #14
sunGreat to see this! :)
Comment #16
webchickLovely.
Committed and pushed to 8.x. Thanks!