Problem/Motivation

To remove dependency to Classy, we need to ensure core theme aren't relying on any markup in Classy. #3096349: Create test for confirming Themes do not depend on Classy templates has added test coverage to ensure that markup isn't being inherited from Classy. Currently, lots of templates have been added as ignored.

Proposed resolution

Ensure all templates have been removed from the skip lists, and copies of those templates exist in core themes.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Status: Active » Postponed
Parent issue: » #3050389: [META] Remove dependency to Classy from core themes

This should be postponed until #3050389: [META] Remove dependency to Classy from core themes has been resolved.

lauriii’s picture

Status: Postponed » Active

This was actually postponed on #3096349: Create test for confirming Themes do not depend on Classy templates, not the meta issue 😜

bnjmnm’s picture

Status: Active » Needs review
FileSize
149.63 KB

The patch may be 149k, but it is a gentle read. Other than the changes to ConfirmClassyCopiesTest, which was modified to also check for templates, the other changes are removing template names from the skip-list in ThemesNotUsingClassyTemplatesTest and making copies of template files, where some calls to attach_library are changed when it requests classy libraries. The only thing to watch out for with the template copies is for some files, git --find-copies does not compare the copied file to the version in Classy. In those cases, the patch will show the attach_library() call being added instead of changed.

Status: Needs review » Needs work

The last submitted patch, 4: 3113608-4.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
149.63 KB
149.8 KB

The test failed due to it being an 8.x patch being test on 9.x. Here are individual versions for 8 and 9.

Status: Needs review » Needs work

The last submitted patch, 6: 3113608-6_9X.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review

The failure in the 9X patch is an unrelated Media Library test that is known to intermittently fail. Queueing a re-test.

lauriii’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
    @@ -85,6 +85,11 @@ public function testClassyCopies($theme, $path_replace, array $filenames) {
    @@ -167,6 +172,89 @@ public function providerTestClassyCopies() {
    

    Confirmed that all templates have been listed here correctly 👍

  2. Confirmed that both, removing templates and changing attach_library calls to reference Classy libraries lead into test failures in all themes.
  3. Generated a test only patch which forces comparison to Classy templates. In this patch it's easy to see that all the template modifications are correct. 👍
  4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
    @@ -169,344 +169,33 @@ public function providerTestThemesTemplatesNotClassy() {
    +          // Registered via views_theme() in views.module, but does not
    +          // represent an actual template.
               'views-form-views-form',
    

    Nit: Any thoughts on hard coding this before the file_get_contents() call?

bnjmnm’s picture

Nit: Any thoughts on hard coding this before the file_get_contents() call?

Good call, that would be fine in this case since it's unlikely the skip arrays will change after this. Hard coding is simpler and less redundant.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
150.07 KB

Here's the test patch I mentioned earlier on #9. This should be only used for reviewing the changes.

Changes in #10 address my earlier feedback. 👍

The last submitted patch, 10: 3113608-10_8X.patch, failed testing. View results

bnjmnm’s picture

(note that the test failure reported in #12 was due to the 8.x patch being tested on 9. Added an 8 test.

alexpott’s picture

Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 7c6f55e and pushed to 9.0.x. Thanks!
Committed f33429b and pushed to 8.9.x. Thanks!

One more step :)

  • alexpott committed 7c6f55e on 9.0.x
    Issue #3113608 by bnjmnm, lauriii: Add copies of Classy templates to...

  • alexpott committed f33429b on 8.9.x
    Issue #3113608 by bnjmnm, lauriii: Add copies of Classy templates to...

  • alexpott committed 02d5e65 on 9.0.x
    Issue #3115153 by lauriii: Follow-up to #3113608: Bartik template...

  • alexpott committed e9a0e67 on 8.9.x
    Issue #3115153 by lauriii: Follow-up to #3113608: Bartik template...

Status: Fixed » Closed (fixed)

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