Issue #2151119 by joelpittet, Cottser, idflood, InternetDevels, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Convert theme_system_themes_page() to Twig

Task

Convert theme_system_themes_page() to a Twig template.

Remaining tasks

  • Patch
  • Patch needs another review
  • Manual testing
  • Profiling

Steps to test

Navigate to /admin/appearance.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes

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

joelpittet’s picture

Status: Active » Needs review
FileSize
9.63 KB

Split from the system module twig conversion meta.

Status: Needs review » Needs work

The last submitted patch, 2: 2151119-1-twig-theme_system_themes_page.patch, failed testing.

joelpittet’s picture

star-szr’s picture

Status: Needs work » Needs review

Thanks @joelpittet!

ipo4ka704’s picture

The last submitted patch, 4: 2151119-4-twig-theme_system_themes_page.patch, failed testing.

joelpittet’s picture

Re-roll and leave the form alter were it was.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

idflood’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.91 KB

Reroll of path in #8.

idflood’s picture

Issue summary: View changes
Issue tags: -needs profiling

Here is the profiling of the patch in #10.

=== 8.x..8.x compared (52e3e4975b356..52e3e4dce8a32):

ct  : 56,131|56,131|0|0.0%
wt  : 350,578|351,401|823|0.2%
cpu : 334,639|335,689|1,050|0.3%
mu  : 15,056,104|15,056,104|0|0.0%
pmu : 15,183,712|15,183,712|0|0.0%


=== 8.x..theme-system-twig compared (52e3e4975b356..52e3e5149d9ad):

ct  : 56,131|56,553|422|0.8%
wt  : 350,578|351,361|783|0.2%
cpu : 334,639|335,033|394|0.1%
mu  : 15,056,104|15,104,792|48,688|0.3%
pmu : 15,183,712|15,234,608|50,896|0.3%

I've also quickly checked visually and the markup of admin/appearance . Seems to be fine but I did not check for the case where a theme doesn't have a screenshot.

InternetDevels’s picture

Fixed comments standards in the previous patch.

Status: Needs review » Needs work

The last submitted patch, 12: twig-theme_system_themes_page-2151119-11.patch, failed testing.

star-szr’s picture

Thanks @InternetDevels, we weren't changing/adding that line so we could just as well leave it as is. We lost the .html.twig template from the patch though, so that needs to be added back :)

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
8.94 KB
2.02 KB

Thanks @Cottser, for the answer.
Oops, we really missed it. Added it back ;)

star-szr’s picture

Issue tags: +Twig conversion
idflood’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Did some manual testing and everything works as expected.

The patch looks good and the markup also doesn't have any significant difference.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @idflood! I'm reviewing this and it's not quite ready yet, patch coming a bit later.

star-szr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.36 KB
7.37 KB

Summary of changes:

  • Moves logic for theme compatibility and theme notes into the template. This allows for a better match with the incompatible theme markup (we were missing div.incompatible which Seven for example styles differently).
  • Cleans up docblock in template (minor nitpicks and more clearly shows the nested structure), other minor documentation tweaks.
  • Removes unused empty screenshot markup from preprocess since this is in the template now.

I compared markup again and it looks good, including the no screenshot case.

joelpittet’s picture

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,61 @@
+            {# Display operation links if the theme is compatible. #}
+            {% if not theme.incompatible %}
+              {{ theme.operations }}
+            {% else %}
+              <div class="incompatible">{{ theme.incompatible }}</div>
+            {% endif %}

The double negatives are hard to read in code. Just swap the blocks and remove 'not'.

Otherwise I like these changes alot:)

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Hehe, agreed! It felt a bit wrong.

Tagging as a good task for a new contributor to make the change suggested in #20 and post a new patch and interdiff.

jamesrutherford’s picture

Status: Needs work » Needs review
FileSize
9.36 KB
818 bytes

Thanks for identifying an easy new contributor task! Here is the new patch and interdiff. Setting to needs review.

star-szr’s picture

Issue tags: -Novice

Changes look good, thanks @jamesrutherford!

joelpittet’s picture

Back to RTBC, did another markup review and it's looking good. Attached are the before and after markup and the normalized versions for the screenshot and easier review.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Shoot. Really sorry to say, but this one no longer applies. Can we get a quick re-roll?

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.32 KB

Quick re-roll as requested:)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Actually, sorry.. #22 applied just fine with patch -p1.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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