Problem/Motivation

In #2350823: Use the Classy theme in the Testing profile we "temporarily" changed the testing profile to use classy theme (instead of stark) as we cleaned the CSS classes out of core and into classes.

Meanwhile, there are a number of issues that are trying to fix bugs in core's markup, many for various form elements.

Drupal 8 frontend backwards compatibility (BC) policy says we can't change templates for stable or classy until D9.

Comments like #2443815-111: #description_display broken for details elements seem to regularly paralyze attempts to fix legitimate bugs, since no one understands what to do once the "stable and classy should remain intact" comment is posted.

How are we actually supposed to fix the bugs in core? The 'testing' profile uses the classy theme. So if we try to fix a bug, we're supposed to update the tests, but if we can't fix the classy templates, the tests will never pass. Deadlock ensues.

Sample of bugs that have come to a halt as a result of this policy:

Proposed resolution

Ensure that our test suite runs against a theme with base theme: false so that our tests actually test the core module templates and CSS, not a theme that our BC policy prevents us from fixing.

In particular, return the testing profile to stark and update all the tests as necessary to compensate for the new, stripped down markup.

Drupal 8.8.x release will be the last release for adding new deprecations (for Drupal 9). Therefore, we should try to get at least the code that deprecates this behavior into Drupal 8.8.x. This would essentially allow us to continue working on this in Drupal 8.9.x.

We introduce a new optional ::$defaultTheme property for choosing default theme for tests extending Drupal\Tests\BrowserTestBase and use an install profile that does not specify a default theme (no system.theme config with a default key), of which the testing install profile is the prime example. The optionality of that property (again, for install profiles without a default theme) is deprecated and will be made required in Drupal 9. This is similar to what we are doing for the theme info.yml base theme property in #3065545: Deprecate base theme fallback to Stable.

This change would allow test cases to pick between Classy, Stable and Stark. If Classy gets deprecated in Drupal 9, test authors would have a way to convert their tests to a theme which support will continue.

Explicit vs implicit

Our BC policy allows tests to break between majors. At the moment, most tests use classy, because that is the default theme in the testing install profile. During the Drupal 9 cycle, classy is going to be deprecated to be removed from Drupal 10. Probably in Drupal 9.1 or 9.2.

If we can avoid new tests from being written against classy before it gets deprecated, then we can also avoid them needing to be rewritten during the Drupal 9 cycle.

This is why it’s very valuable to nudge Drupal developers towards explicitly specifying the theme they’re going to be testing against while using the testing install profile. They can still choose to stick to defaults during the Drupal 8 cycle (that’d be classy), but if they do explicitly specify the theme to be used, their tests will not need to change for a long time to come.

Future default value? Would it be opt-in or opt-out?

Today, the default theme when writing tests is classy, which is going away in the future.
The patches so far in this issue are indicating that there will be no default. But “no default” is just one of the possible choices. There are 3 possible choices — see #68, or preferably, see #3081131: Decide default theme for testing, which is the dedicated follow-up issue for determining what the future default should be, and whether it should be opt-in or opt-out.

Remaining tasks

  1. THIS ISSUEThis issue only introduces the deprecation, and skips it. Why only this scope? To ensure this lands in time before 8.8.0. Only this portion of the patch is truly essential to land before 8.8's alpha deadline.
  2. FOLLOW-UP #3082655: Specify the $defaultTheme property in all functional tests then unskips the deprecation error and makes all core tests compliant. This already results in most tests being converted from classy to stark. But it does so without changing a single assertion. Why only this scope? Because it keeps it easier to review and allows even a novice user to do this work.
  3. FOLLOW-UP After that, #3083275: [meta] Update tests that rely on Classy to not rely on it anymore takes a critical look at the tests that the previous step added protected ::$defaultTheme = 'classy'; to. Every test that has that yet is not explicitly testing Classy markup should have its assertions updated, so that it can switch to stark (or perhaps stable).
  4. FOLLOW-UP In the meanwhile, #3081131: Decide default theme for testing discusses which theme to use by default in Drupal 9.

API changes

None.

User interface changes

None.

Data model changes

None.

Release note snippet

Tests using the testing install profile (which is the default) will be required to specify which theme they're testing. The same is true for any other install profile that does not specify a default theme (i.e. that does not come with system.theme config with a value for the defaultkey). Read more in the change record for deprecating the use of Classy as the default for the testing profile.

CommentFileSizeAuthor
#121 2352949-118v2.patch8.77 KBJaesin
#118 2352949-118.patch20.59 KBJaesin
#118 116-118-interdiff.txt790 bytesJaesin
#116 2352949-116.patch8.74 KBJaesin
#116 111-116-interdiff.txt2.55 KBJaesin
#111 2352949-111.patch9.41 KBWim Leers
#111 interdiff.txt4.17 KBWim Leers
#105 2352949-105.patch10.32 KBWim Leers
#105 interdiff.txt841 bytesWim Leers
#104 2352949-104.patch10.42 KBWim Leers
#103 2402533-103.patch22.84 KBWim Leers
#103 interdiff-100-103.txt1.76 KBWim Leers
#103 interdiff-99-100.txt2.81 KBWim Leers
#99 2352949-98.patch10.11 KBWim Leers
#99 interdiff.txt3.73 KBWim Leers
#98 2352949-97.patch10.27 KBWim Leers
#98 interdiff.txt7.52 KBWim Leers
#93 interdiff.txt2.8 KBWim Leers
#92 2352949-92.patch9.86 KBWim Leers
#82 2352949-82.patch9.91 KBWim Leers
#82 interdiff-78-82.txt1.39 KBWim Leers
#82 interdiff.txt16.36 KBWim Leers
#81 2352949-81.patch26.2 KBWim Leers
#81 interdiff.txt17.69 KBWim Leers
#78 2352949-78.patch9.89 KBWim Leers
#78 interdiff.txt1.08 KBWim Leers
#77 2352949-77.patch9.84 KBWim Leers
#77 interdiff.txt1 KBWim Leers
#75 interdiff.txt711 bytesWim Leers
#74 2352949-74.patch8.9 KBWim Leers
#72 2352949-72.patch8.26 KBWim Leers
#72 interdiff.txt1.65 KBWim Leers
#69 2352949-69.patch6.68 KBWim Leers
#69 interdiff.txt5.01 KBWim Leers
#56 2352949-56.patch6.71 KBWim Leers
#56 interdiff.txt792 bytesWim Leers
#54 interdiff.txt1.24 KBWim Leers
#54 2352949-54.patch6.58 KBWim Leers
#52 2352949-52.patch6.49 KBWim Leers
#52 interdiff.txt739 bytesWim Leers
#48 2352949-47.patch5.81 KBWim Leers
#48 interdiff.txt1.3 KBWim Leers
#46 2352949-46.patch5.14 KBWim Leers
#46 interdiff.txt603 bytesWim Leers
#45 2352949-45.patch5.12 KBWim Leers
#45 interdiff.txt955 bytesWim Leers
#44 2352949-44.patch5.11 KBWim Leers
#44 interdiff.txt949 bytesWim Leers
#42 2352949-42.patch4.94 KBWim Leers
#42 interdiff.txt2.38 KBWim Leers
#41 2352949-41.patch3.28 KBWim Leers
#41 interdiff.txt500 bytesWim Leers
#40 2352949-40.patch2.8 KBWim Leers
#40 interdiff.txt906 bytesWim Leers
#38 2352949-38.patch1.93 KBWim Leers
#32 2352949-32.patch89.81 KBlauriii
#20 in-progress-2352949-use-stark-for-tests-20.patch51.18 KBakalata
#18 in-progress-2352949-use-stark-for-tests-18.patch1.87 MBakalata
#16 in-progress-2352949-use-stark-for-tests-16.patch51 KBakalata
#14 in-progress-2352949-use-stark-for-tests-14.patch730.97 KBakalata
#12 in-progress-2352949-use-stark-for-tests-12.patch32.24 KBakalata
#8 2352949-remove_classy_as_testing_profile_theme-8.patch19.58 KBakalata
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MKorostoff’s picture

Issue summary: View changes
tstoeckler’s picture

Component: profile.module » base system

Hmm... we don't have a "testing system" component. This isn't really specific to simpletest module, so tagging as base system. "profile.module" is the D6 (and sadly D7) module that used to be in core.

dawehner’s picture

Mh, are we sure that this is a total sane idea? I think having some kind of test coverage for the different kind of classes we output is a good idea.
If we don't rely on classy anymore, we won't have implicit test coverage for those.

tstoeckler’s picture

IMHO if we want to test the Classy theme, we should add dedicated tests for it and not test its markup in hundreds of tests randomly all over core.

xjm’s picture

Title: Remove Classy testing profile dependency » [meta] Remove Classy testing profile dependency
Status: Postponed » Active

Agreed @tstoeckler. We should test Classy when we're testing it, and not test Classy when we're testing something unrelated.

Let's make this a meta.

xjm’s picture

dawehner’s picture

dawehner: I think most of the "coverage" is incidental TBH. I.e. tests that assert some exact markup, but just want to assert that something is there not whether it has specific classes

This convinced me entirely.

Writing dedicated tests for class would not be that hard, because you "just" need to setup the stuff, render it and check it.

akalata’s picture

My tests aren't running locally, so seeing waht the bot says. And yes, I expect it to fail!

akalata’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 2352949-remove_classy_as_testing_profile_theme-8.patch, failed testing.

akalata’s picture

Assigned: Unassigned » akalata
Issue tags: +drupaldevdays

I'm going to try and get this started after lunch!

akalata’s picture

Status: Needs work » Needs review
FileSize
32.24 KB

Post in-progress test conversion.

Status: Needs review » Needs work

The last submitted patch, 12: in-progress-2352949-use-stark-for-tests-12.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
730.97 KB

More in-progress test conversion.

Status: Needs review » Needs work

The last submitted patch, 14: in-progress-2352949-use-stark-for-tests-14.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
51 KB

Let's try that again.

Status: Needs review » Needs work

The last submitted patch, 16: in-progress-2352949-use-stark-for-tests-16.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
1.87 MB

Reroll

Status: Needs review » Needs work

The last submitted patch, 18: in-progress-2352949-use-stark-for-tests-18.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
51.18 KB

And trying this again (I'm out of practice)

Status: Needs review » Needs work

The last submitted patch, 20: in-progress-2352949-use-stark-for-tests-20.patch, failed testing.

akalata’s picture

Assigned: akalata » Unassigned

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Want to use stark for testing modal dialogs? Nope. #2936535: Modal dialog errors in stark theme

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dww’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +blocker

Was pointed here over in #3031198-58: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption. I opened that issue to address our collective despair trying to fix markup bugs in core and hitting the obvious deadlock case:

- Bug fixes must include tests (for all the right reasons).
- Tests run against classy (what this issue is trying to solve).
- Thou shalt not touch classy (per Drupal 8 frontend backwards compatibility (BC) policy).
- Deadlock.

It's at least a major problem (if not critical) that our entire test suite runs against a theme that we have a policy of not changing. It means we simply can't fix bugs (unless they cross this "this is so critical that we'll make exceptions to our BC policy for it" threshold).

I have a working patch at #3031198-54: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption that adds a 'classy_dev' theme to core and ports our test suite to use that. Maybe I can repurpose it to try switching to stark.

Bumping this to major and adding the 'blocker' tag since I personally know this is blocking many bugs in the core queue. I put an initial sample of 6 into the summary, along with other updates to clarify why this is such an urgent problem to solve.

Thanks,
-Derek

lauriii’s picture

Issue tags: +Drupal 9
lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Drupal 9 +Drupal
FileSize
89.81 KB

TLDR; Here's a first patch where I've used data-drupal-testing-selector to be able to assert contents of specific elements. I'm expecting this patch to lead into lots of test failures still.

One of the reasons Classy was used for testing at first was that we didn't have a vision of how we would like to do assertions of specific elements when there are no selectors.

I discussed with @Wim Leers and his initial proposal was to use very specific xpath selectors including conditions. For example: body/div[2]/div[3]. We didn't feel comfortable with the approach because even small markup changes could force us to change lots of test selectors.

I proposed that we would use data-drupal-selector for this, but Wim was concerned that module/theme authors might start using these as selectors and as consequence, it could become difficult for us to make changes to these selectors. To address that, he proposed we would standardize on something like data-drupal-testing-selector. We also discussed implications of this selector ending up in site markup, since adding lots of unused markup could potentially lead into lots of more data having to be transferred to clients. Wim proposed that we would create a response filter that would remove these from the rendered output on production sites. I had concerns that this could be confusing for themers because something would be visible in the template but doesn't get included in rendered content in production. In the end, we thought it would be enough at least for now if we ensured that these testing attributes don't end up in themes extending on Stable or Classy.


I don't think this issue should block the child issues trying to do bug fixes. The child issues should first seek a non-BC breaking way to fix the bug. This means that the bug gets fixed in the module markup, but reverted in Stable so that Stable and Classy templates don't have to be modified. To add test coverage for the bug fix, we could manually change the theme to Stark and add @todo to remove that code in this issue.

lauriii’s picture

Issue tags: -Drupal +Drupal 9
lauriii’s picture

Version: 8.6.x-dev » 8.8.x-dev
lauriii’s picture

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

Drupal 8.8.x release will be the last release for adding new deprecations (for Drupal 9). Therefore, we should try to get at least the code that deprecates this behavior into Drupal 8.8.x. This would essentially allow us to continue working on this in Drupal 8.9.x.

I recommend that we introduce a new property for choosing default theme for tests extending Drupal\Tests\BrowserTestBase. For now, the property would be optional and the default value would be Classy. The optionality of that property is deprecated and will be made required in Drupal 9 as part of this issue. This is similar to what we are doing for the theme info.yml base theme property here: #3065545: Deprecate base theme fallback to Stable.

This change would allow test cases to pick between Classy, Stable and Stark. If Classy gets deprecated in Drupal 9, test authors would have a way to convert their tests to a theme which support will continue.

As part of the initial patch, we should add this property to every core test extending Drupal\Tests\BrowserTestBase to not trigger deprecation warnings. We can work on changing the theme to Stark in follow-up issues.

lauriii’s picture

We should update the issue summary if we agree that #35 is the way to go.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Thank you, your comment is much much clearer than what I would have posted! 🙏👏

I need to handle something else first, but this is my next priority! Assigning to me for clarity.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

This change would allow installation profiles to pick between

I think "installation profiles" should have read "tests" here :)

Attached is the absolute minimal viable patch that implements the plan outlined in #35. This should trigger many, many deprecation errors… but should allow all tests to pass nonetheless.

Next steps:

  1. If this triggers the expected deprecation errors, I'll add it to the list of skipped deprecations in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations().
  2. Figure out how to remove classy from the list of dependencies in testing.info.yml without it breaking tests 🤓

Will continue this in the morning.

Status: Needs review » Needs work

The last submitted patch, 38: 2352949-38.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
906 bytes
2.8 KB

That seems to have worked. This skips that deprecation.

Wim Leers’s picture

While #40 is still running (and it seems to pass most tests already, so that should be mostly okay), let's meanwhile already do the next step: remove classy from the testing profile's dependencies.

Wim Leers’s picture

#41 should again have significantly more failures. After some digging, I think this approach makes most sense, and follows the pattern already established by \Drupal\Core\Test\FunctionalTestSetupTrait::installModulesFromClassProperty(). 🤞

The last submitted patch, 40: 2352949-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

The 40 fails in #40 are interesting. Many of them are happening in either update path tests or tests that were using an install profile other than testing, with a different default theme than classy.

This should make the tests using an install profile other than testing pass tests.

Wim Leers’s picture

#41 and #42 contain a single coding standard violation, hence we only see "Build Successful" 🤓

#41 had 445 failures, #442 had 45 failures (5 more than #40).

This fixes that sole violation.

Wim Leers’s picture

The failing update path tests are super hard for me to debug because … those same update tests also fail for me on HEAD :(

But the Quick Edit test failures I did manage to figure out: it's because no admin theme is specified anymore.

Wim Leers’s picture

Make \Drupal\Tests\minimal\Functional\MinimalTest pass again — I missed the minimal profile in #44.

I also attempted to fix \Drupal\simpletest\Tests\SimpleTestTest in this reroll — but since that one can't be run using phpunit I'm letting DrupalCI verify that for me.

Wim Leers’s picture

#47 Interesting remark! But all the other config creation in \Drupal\Core\Test\FunctionalTestSetupTrait::initConfig() doesn't do that either, why should this one?

Berdir’s picture

> #41 and #42 contain a single coding standard violation, hence we only see "Build Successful" 🤓

That's... not how it works :)

Build successful happens when something fails so bad that the results can't be parsed. Fatal errors, too many errors, or looking at https://dispatcher.drupalci.org/job/drupal_patches/8218/console, it might be the nightwatch test fail that we can't handle?

Wim Leers’s picture

#50: Okay. That's how I would swear how it used to work. But I'll fully admit that this is super opaque, and I definitely do not fully understand it :)

Wim Leers’s picture

#46 is down to 25 failures. #48 is down to 20.

The hint in #50 helped me realize that I indeed have to update TestSiteInstallCommand, which I wasn't sure about at first. Thanks, @Berdir! (Crediting you.)

Status: Needs review » Needs work

The last submitted patch, 52: 2352949-52.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
6.58 KB
1.24 KB

#48 was down to 20 … plus failures in nightwatch tests. Now those are passing, and just like @Berdir indicated, that allows us to get actual test results again 🥳

The fix for Quick Edit tests in #46 was wrong though. This is what actually matches the behavior in HEAD.

Status: Needs review » Needs work

The last submitted patch, 54: 2352949-54.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
792 bytes
6.71 KB

The missing ingredient: taking the default from core/modules/system/config/install/system.theme.yml into account!

Status: Needs review » Needs work

The last submitted patch, 56: 2352949-56.patch, failed testing. View results

lauriii’s picture

This is pretty close to what I was thinking. However, I didn't take into account the fact that some tests might want to rely on the installation profiles default configuration. For example, when testing demo_umami, it would make sense to always set it up with the umami theme.

Requiring theme property still makes sense when not testing a specific installation profile. The reason for this is that contrib tests might want to rely on some version of stable so that core updates don't result in breaking tests. When it comes to core, tests should always rely on stark to ensure that when markup is changed, that gets reflected in tests.

We have at least two different paths we could take from here, but I'm open to other ideas as well. Here are the two different ways forward I could think of now:

  1. Allow setting theme property to false to allow test cases to rely on installation profile configuration. The downside of this is that this might be confusing since most people would probably not have the expectation of having to set this property.
  2. Only require theme property when the installation profile is set to testing. The downside of this is that it could make it difficult to understand when the property is needed and when it isn't.
lauriii’s picture

Discussed this at length with @Wim Leers. It seems like there was some confusion around who would be responsible for specifying the theme used for testing.

We came to the conclusion that all test classes will have to specify the theme used in their test by using the new $theme property. This is due to the fact that in the future we are going to have multiple versions of stable, and therefore it isn’t safe to assume any default theme.

As a result of this change, all tests will have to set the $theme property. We will recommend tests to use classy for all existing tests. However, for new tests, this could be set to stable or stark depending on maintainer preference and depending on what is being tested. stark could be used for example, if markup and CSS are irrelevant to what is being tested, but also if only module markup and CSS are being tested (and not core markup and CSS).

We thought that this makes sense for tests that are using the default testing installation profile. The testing installation profile is mostly just an implementation detail. However, when testing with another installation profile it would make sense to use the theme used by the installation profile. This seems like a fairly rare use case to optimize the DX for while sacrificing some explicitness and clarity, so we may want to avoid optimizing this now.

Berdir’s picture

Why can't we just set the default of that property to classy in BrowserTestBase, then tests that want to can override it without breaking every single test out there, that seems like a lot of work for contrib maintainers and custom code for little gain?

lauriii’s picture

Why can't we just set the default of that property to classy in BrowserTestBase, then tests that want to can override it without breaking every single test out there, that seems like a lot of work for contrib maintainers and custom code for little gain?

It is likely that classy will be removed from core in future: #2352949: Deprecate using Classy as the default theme for the 'testing' profile. As a result, contrib and custom tests should migrate to stable or stark.

In future there will be multiple versions of Stable (stable, stable9 and stable10 and so on). To make upgrading easier, we will support at least two versions simultaneously. Because of this, it's not possible for BrowserTestBase to assume any theme as default. For the same reasons we are making the base theme setting required in theme info.yml: #3065545: Deprecate base theme fallback to Stable.

dawehner’s picture

For me it comes down to explicitness. Can someone figure out what the setup of a test is by looking at the test class.

  • Adding $theme helps to figure out when there is no install profile is set.
  • When someone sets a install profile they, at least for me, they explicitly tell how they want the environment to be setup. Many installation profiles are connected with themes to provide a coherent experience, just look at umami for example, so from my point of view if you specify the installation profile you expect to test the theme at the same time.

*

Berdir’s picture

> Because of this, it's not possible for BrowserTestBase to assume any theme as default.

I still think we should set a default to classy, possibly with a @trigger_warning() and not introduce a hard break for all existing tests?

And even then, I'm not convinced that "not possible to assume" is true. Even if we remove classy and even when we then have multiple stable themes, we could simply default to the current version of stable and update it? We'll want to test core with that version too and maybe have some specific tests for other versions?

I think it's perfectly valid to document that this will be continuously updated and might break and if someone doesn't want that, they can force it to whatever version they want?

I'd say the majority of tests will not break as we update the base theme like that, and the opposite is that they have to be updated to the most recent stable version manually every other major release?

lauriii’s picture

You're right, it is an option to use the current stable release as a default value. I assumed that we couldn't go with that because it creates the possibility for some tests to break unexpectedly when upgrading between major releases. I would prefer an option where we force test authors to decide a theme and the time when they want to upgrade to the next release.

If markup and CSS are irrelevant to what is being tested and if only module markup and CSS are being tested, test cases could use stark for testing. This means that in theory, these tests could run with the same theme in any major version of Drupal.

I still think we should set a default to classy, possibly with a @trigger_warning() and not introduce a hard break for all existing tests?

I'm not sure I follow. If we set a default value to classy, what do we need the @trigger_error() for?

Berdir’s picture

> tests to break unexpectedly when upgrading between major releases.

As I wrote, I'd expect more things to break when we suddenly remove stable8 in Drupal10 or something like that. Our BC Policy does allow to break tests, as long as only the tests and not the functionality is broken.

> I'm not sure I follow. If we set a default value to classy, what do we need the @trigger_error() for?

for the same reason as every other deprecation message. We'd do that if we decide that tests *must* specify which theme they want to use but we don't want to break them yet in 8.x, we'd only fail them starting with 9.x. Wouldn't be trivial to detect that, as we would need to use reflection or something to explicitly check if any subclass does explicitly set one.

lauriii’s picture

for the same reason as every other deprecation message. We'd do that if we decide that tests *must* specify which theme they want to use but we don't want to break them yet in 8.x, we'd only fail them starting with 9.x. Wouldn't be trivial to detect that, as we would need to use reflection or something to explicitly check if any subclass does explicitly set one.

Sorry, I was confused because I thought that was already the plan even though it might not be properly explained here. So I agree, if we decided to make the theme property required, we shouldn't do a hard break in Drupal 8. We should instead deprecate the default value and optionality and require it in Drupal 9.

jibran’s picture

Issue tags: +DTT issue

This change may affect Drupal Testing Trait as well so tagging.

Wim Leers’s picture

Status: Needs work » Needs review

@lauriii and I pair-wrote this by talking about this issue in detail and playing devil’s advocate!

Install profiles

@lauriii in #58.2 and @dawehner in #62 argued that tests using install profiles other than testing should not have to specify a theme explicitly. Even though only a small number of tests in the overall ecosystem use specific install profiles (this is, of course, different for distribution maintainers such as Thunder), it improves DX and reduces disruption. So let’s do that. ✅

Hard break or not

Next, @Berdir was concerned that we’re going to do a hard break in 8.x (and hence cause huge disruption). That is not the case, we all agree that this type of hard break should be avoided. It’s already not the case in the current patch :) ✅

Explicit vs implicit

@Berdir points out in #65 that our BC policy allows tests to break between majors. At the moment, most tests use classy, because that is the default theme in the testing install profile. During the Drupal 9 cycle, classy is going to be deprecated to be removed from Drupal 10. Probably in Drupal 9.1 or 9.2.

If we can avoid new tests from being written against classy before it gets deprecated, then we can also avoid them needing to be rewritten during the Drupal 9 cycle.

This is why it’s very valuable to nudge Drupal developers towards explicitly specifying the theme they’re going to be testing against while using the testing install profile. They can still choose to stick to defaults during the Drupal 8 cycle (that’d be classy), but if they do explicitly specify the theme to be used, their tests will not need to change for a long time to come.

Future default?

Today, the default theme when writing tests is classy, which is going away in the future.
The patches so far in this issue are indicating that there will be no default. But “no default” is just one of the possible choices. There are 3 possible choices:

Theme Pros Cons When best fit?
None
  • Explicit: no surprises, full control
  • Higher chance the test authors choose the most appropriate theme for the test case
Boilerplate: every test must specify it N/A
stark No boilerplate Higher probability of breaking between minor releases
  • Contrib / custom tests that DO NOT depend on core markup
  • Core tests because they should be updated at the time when markup changes occur
stable9 No boilerplate Higher probability of breaking between major releases
  • Contrib / custom tests that DO depend on core markup
  • Contrib / custom tests that are not using markup for testing

(In Drupal 9, the default would be stable9, in Drupal 10, it would be stable10, and so on.)

Default: opt-in vs opt-out?

There might be a way to be both explicit and provide a default value. We could set a default value that is set only if the test case has explicitly opted into the default. One option would be provide TESTING_DEFAULT_THEME constant which would allow test cases to use the current default value. This would remove the need to manually update testing theme on major releases.

Drupal 8
  const TESTING_DEFAULT_THEME = ‘classy’;
  const CORE_TESTING_DEFAULT_THEME = ‘stark’;
Drupal 9
  const TESTING_DEFAULT_THEME = ‘stable9’;
  const CORE_TESTING_DEFAULT_THEME = ‘stark’;
Drupal 10
  const TESTING_DEFAULT_THEME = ‘stable10’;
  const CORE_TESTING_DEFAULT_THEME = ‘stark’;

This brings up a question: could we actually recommend anyone to use the TESTING_DEFAULT_THEME given that we would already know that it breaks in Drupal 9? We would be in a much better position if we made all existing tests explicitly set $theme to classy, and all new tests explicitly set $theme to stable (or stark if they’re not relying on markup at all or at least not on core markup). That way any new tests created between now and Drupal 9 wouldn’t have to be rewritten as part of the Drupal 9 upgrade process. This highlights how the default value is broken at least right now.

We could move the discussion about Drupal 9’s testing default theme to a separate issue so we don’t have to discuss it here.

TL;DR: it seems that it would be the best decision for the ecosystem to require test cases to explicitly pick the most suitable theme, at least until Drupal 9.0.0 (assuming we can come up with a better “default” strategy in that follow-up issue).

Wim Leers’s picture

The first green checkmark for #68: here's the updated patch for that :)

lauriii’s picture

Opened follow-up to decide the default theme for testing in Drupal 9: #3081131: Decide default theme for testing.

Status: Needs review » Needs work

The last submitted patch, 69: 2352949-69.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
8.26 KB

The ThemeTest failure took me hours to figure out 🙃.

Buckle up, I'm taking you down the rabbit hole! 🐰🥕

The root cause: after the patch is applied, only one Block config entity exists: the one that is created by the $this->drupalPlaceBlock('local_tasks_block'); call in ::setUp(), which places a local tasks block in stark. But in HEAD, 3 blocks exist: not only that same one in Stark, but also one in Bartik and one in Classy!

Why though?

Well, those two tests install the block module. And since #507488: Convert page elements (local tasks, actions) into blocks, the local_actions_block and local_tasks_block blocks must be explicitly placed by the user. This happens automatically when using the standard or minimal install profile (thanks to their default config). But not when using the testing profile. \Drupal\Tests\system\Functional\System\ThemeTest::setUp() for example was updated in that same issue to place the local_tasks_block block for the default theme! But why did this then work fine before, but stopped working now? Because the testing install profile no longer has a default theme, and hence block_theme_initialize() (called by block_themes_installed() upon installing a theme) no longer is able to copy blocks from the previous default theme, because there is not anymore a default theme.

  • In the case of \Drupal\Tests\system\Functional\System\ThemeTest::testSwitchDefaultTheme(), ThemeInstaller::install() returns early (before hook_themes_installed() is invoked, hence resulting in no blocks being duplicated. It returns early because stark is the default in core/modules/system/config/install/system.theme.yml and hence it already is installed.
  • In the case of \Drupal\Tests\block_content\Functional\BlockContentTypeTest::testsBlockContentAddTypes(), the same is happening, but that installs three themes (\Drupal::service('theme_installer')->install(['bartik', 'seven', 'stark']);), and the blocks are duplicated for bartik and seven, but not for stark, since that already is installed.

The solution then is quite simple: just explicitly place the block. 😅

Both of these are testing theme system/block system edge cases, this does not negatively impact many tests in the ecosystem (only a few test methods in all of Drupal core are affected!).

Status: Needs review » Needs work

The last submitted patch, 72: 2352949-72.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.9 KB

Apparently UpdatePathTestBase does not just use the DB dump, it does still install Drupal … to some extent. And because UpdatePathTestBase sets protected $installProfile = 'standard'; and simply ignores the inherited protected $profile = 'testing';, this results in the FunctionalTestSetupTrait-added code also executing for update path tests, which then results in the default theme being overridden 🤦‍♂️

I think we should probably get rid of \Drupal\FunctionalTests\Update\UpdatePathTestBase::$installProfile, but that's definitely out-of-scope here.

Wim Leers’s picture

FileSize
711 bytes

Oops, forgot this!

Status: Needs review » Needs work

The last submitted patch, 74: 2352949-74.patch, failed testing. View results

Wim Leers’s picture

Ah, just one remaining failing test! It fails on an assertion added in #2331991: Theme missing when installing in non-English language. Unsurprisingly, related to "the active theme".

The installer test uses the testing install profile, which is now using no theme (and hence falls back to stark), but the assertion added in #2331991: Theme missing when installing in non-English language is specifically asserting the presence of a classy CSS file. The installed theme is the correct one, it's the assertion that needs to be updated in this case.

So, we should be able to do

-    $this->assertRaw('classy/css/components/action-links.css');
+    $this->assertRaw('core/themes/stark/css/layout.css');

… but we can't, because of #2349711: Remove all visual from stark! (That means the stark/global-styling asset library should've been deprecated, but deprecating asset libraries was not yet possible at the time.)

So … we only have two possible choices:

  1. Remove the assertion, but risk regressing #2331991: Theme missing when installing in non-English language.
  2. We cannot choose to use a different theme, because this test is testing the installer. Therefore the only way to choose a different theme is to choose a different install profile.
Wim Leers’s picture

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -140,6 +140,8 @@ public static function getSkippedDeprecations() {
+      // @todo Remove in …
+      'Drupal\Tests\BrowserTestBase::$theme is required in drupal:9.0.0 when using the default "testing" install profile. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use',

This was the follow-up we still needed. Created #3082655: Specify the $defaultTheme property in all functional tests for that.

Wim Leers’s picture

Title: [meta] Remove Classy testing profile dependency » Remove Classy testing profile dependency
Assigned: Wim Leers » Unassigned
Issue summary: View changes
Issue tags: -Needs issue summary update

I think we're done here! 🥳

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Needs change record
Wim Leers’s picture

Change record created: https://www.drupal.org/node/3083055

I noticed a small problem that we'd run into in #3082655: Specify the $defaultTheme property in all functional tests.

Wim Leers’s picture

UGHHHHH #81 includes a different patch I'd applied for testing 🤦‍♂️ 😅

dww’s picture

Thanks for moving this forward, @Wim Leers!

However, it seems the scope of this issue changed. When I commented in #30, it looked like this was going to be the "remove classy from our test suite" issue (as the current title implies), which is why I was excited about it. Now, it's turned into something more like: "allow individual tests to specify what theme to test against" (to give us a path out of the testing-with-classy-hell). That's still progress, hurray, but now I don't know where the "kill classy in the test suite" issue is. #3082655: Specify the $defaultTheme property in all functional tests doesn't look likely to be that issue, either, since that could more accurately be titled "remove a bunch of deprecation warnings we're now suppressing by specifying classy all over the place." :/ Alas.

In #59 @lauriii writes:

We will recommend tests to use classy for all existing tests.

Please, no. ;) That's why we're in so much pain now every time we try to fix a markup-related bug. See the current summary for more on this point. I don't understand why @lauriii continues to believe it's just a question that the child issues haven't tried hard enough to find BC-friendly solutions. That's why they've been languishing for months/years. There's no way to fix most of those bugs without changing (broken) markup that core is producing. That's the BC problem. That's why we've been in deadlock for ages.

Seems we need to:

a) Re-title / re-scope this issue so the commit message is more accurate about what is happening. The summary still implies this issue will remove classy from the test suite, which it no longer does. It just starts to give us an easier path to make that change (incrementally).

b) Create a follow-up issue to actually do what the current title and summary describe, namely, purge classy from the test suite.

c) Reconsider why we want to recommend that existing tests keep using classy. That's creating problems.

Thoughts?

Thanks,
-Derek

p.s. #2349711: Remove all visual from stark was listed as related twice. Removing one of the copies.

Berdir’s picture

> There's no way to fix most of those bugs without changing (broken) markup that core is producing.
That's the BC problem. That's why we've been in deadlock for ages.

That's IMHO not about the tests. We are allowed to break tests. What we're not allowed to break is actual sites, and if sites rely on the markup/selectors of classy/stable, then that's what we'd break, if anything, the tests represent that.

Many tests require some kind of identifiers to assert that stuff is shown where it's supposed to and I suspect the minimalistic markup of stable will make that pretty hard, but we'll see. I'm not actually that involved in the things around that.

Also, I think what you're looking for is just below the comment you quoted in #61:

> It is likely that classy will be removed from core in future: #2352949: Deprecate using Classy as the default theme for the 'testing' profile. As a result, contrib and custom tests should migrate to stable or stark.

And the same comment also explains about different stable themes, which is then what would actually allow us to change markup, by doing it in a new "version" of stable.

dww’s picture

I agree that the BC problems are not only about the tests. I disagree that "the tests represent [sites that would break if we change some markup]". That's not really the case if you look at the details of most of the issues I put in the summary. There simply is no test coverage of the broken thing (or we would have known it was broken before we called it "stable"). The problem is that we're can't add tests to demonstrate the bugs and fix them, since the tests are (currently) always running against a theme that we have a policy that prevents us from changing. So if we "fix the bug in core" and update the test coverage to add a test that demonstrates the bug, the test will never pass so long as it's using classy, since we don't allow ourselves to ever fix classy (except for exceptions, fun).

Anyway, I really don't want to have to write all this up again. ;) I understand that we don't want to "break" existing sites that are relying on core's already-broken markup. So that means we never fix the underlying bugs? If that's what everyone wants me to believe, I'll stop trying to fix core bugs that have anything to do with markup. That just seems like a really bad approach. And please don't answer: "just find a BC-friendly way to fix the problem". That's simply not possible in most of these issues.

For example, in #2419131: [PP-1] #states attribute does not work on #type datetime we encounter the sad fact that the core/modules/system/templates/datetime-wrapper.html.twig twig template doesn't actually provide any wrapper element at all. If we want #states to work, we need a wrapper element. Now what? How, exactly, does adding a wrapper div to the entire datetime element "break" existing sites? I know I'm getting off topic from the issue and patch here, but I'm trying to advocate for the "let's find a way that lets us fix bugs" position, instead of the "our policy is fine since we never break existing sites" answer I seem to keep getting. ;)

Thanks/sorry,
-Derek

dww’s picture

p.s. I don't grok this:

It is likely that classy will be removed from core in future: #2352949: Deprecate using Classy as the default theme for the 'testing' profile. As a result, contrib and custom tests should migrate to stable or stark.

That's a circular reference to this issue. That's why I'm confused about the direction all this is headed in, and why I think we need better title, scope, plan, and probably some additional follow-ups.

Cheers,
-Derek

dww’s picture

p.p.s. The title of the change record is also currently misleading:

Tests no longer use Classy by default

Yes they will, at least given the current patch. This patch isn't changing any tests to not use classy. They all still default to using classy. The only change is that it would now be possible to specify something else if you know you want it...

Wim Leers’s picture

Title: Remove Classy testing profile dependency » Deprecate using Classy as the default theme for the 'testing' profile
Issue tags: -Needs title update, -Needs issue rescope, -Needs issue summary update

@dww Please take a closer look at the patch in #3082655: Specify the $defaultTheme property in all functional tests. It is actually switching most tests over from classy to stark. In that issue, we're only doing it for tests that require no changes in assertions, to keep the scope of that issue under control: to keep it easy to review and hence easy to land. Nothing prevents us from moving more tests over to stark in the future by changing some assertions.

RE: change record title: they indeed still do use Classy by default, to minimize disruption for contrib tests. But that default now does trigger a deprecation error. Improved CR title.

lauriii’s picture

The problem is that we're can't add tests to demonstrate the bugs and fix them, since the tests are (currently) always running against a theme that we have a policy that prevents us from changing.

Tests are not always running using Classy, it's the default theme. It's possible to change the testing theme manually to stark. This patch will make that easier since it will be handled by the base class. This is what I tried to recommend on #2419131-122: [PP-1] #states attribute does not work on #type datetime to unblock the issue.

How, exactly, does adding a wrapper div to the entire datetime element "break" existing sites?

Someone could have build JavaScript or CSS that relies on certain HTML structure. For example, something like #edit-field-date-range > input would break.

Please, no. ;) That's why we're in so much pain now every time we try to fix a markup-related bug. See the current summary for more on this point. I don't understand why @lauriii continues to believe it's just a question that the child issues haven't tried hard enough to find BC-friendly solutions.

What I tried to explain in some of the child issues was that we could consider making an exemption to make those markup changes to Classy since they had a low risk of causing disruption to pre-existing themes. Sorry if I didn't make that clear.

The reason for recommending Classy as a replacement is that it's the easiest way to get rid of the deprecation warning. I did also explicitly say that I don't recommend using Classy as a testing theme on new tests. All tests should be converted to use Stable or Stark but that's the next step on the process.

Wim Leers’s picture

Issue summary: View changes

Clarification for #88 (and issue summary updated):

  1. This issue only introduces the deprecation, and skips it. Why only this scope? To ensure this lands in time before 8.8.0. Only this portion of the patch is truly essential to land before 8.8's alpha deadline.
  2. #3082655: Specify the $defaultTheme property in all functional tests then unskips the deprecation error and makes all core tests compliant. This already results in most tests being converted from classy to stark. But it does so without changing a single assertion. Why only this scope? Because it keeps it easier to review and allows even a novice user to do this work.
  3. After that, #3083275: [meta] Update tests that rely on Classy to not rely on it anymore takes a critical look at the tests that the previous step added protected $theme = 'classy'; to. Every test that has that yet is not explicitly testing Classy markup should have its assertions updated, so that it can switch to stark (or perhaps stable).

Once those two follow-ups are completed, @dww's concerns will have been addressed.

dww’s picture

Status: Needs review » Reviewed & tested by the community

First, huge thanks to @Wim Leers for updating the summary, title, and providing all the links to the relevant follow-ups! Indeed, I'm totally satisfied now. In fairness, #3082655: Specify the $defaultTheme property in all functional tests didn't have a patch when I started drafting comment #83. ;) But yes, I'm much happier now.

Also, huge thanks to both @Wim Leers and @lauriii for responding so thoughtfully and calmly, since re-reading my comments, I'm sure they came off as annoying and unnecessarily heated. Huzzah for everyone keeping their cool, and apologies for my tone.

Finally, this is basically RTBC, with a few possible nits/questions:

  1. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
    @@ -359,6 +361,18 @@ protected function initConfig(ContainerInterface $container) {
    +        @trigger_error('Drupal\Tests\BrowserTestBase::$theme is required in drupal:9.0.0 when using the default "testing" install profile. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use', E_USER_DEPRECATED);
    

    Smallest of nits: Since this isn't ending with the link, but includes additional stuff afterwards, it should probably end in a period. ;)

  2. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
    @@ -403,6 +417,32 @@ protected function initKernel(Request $request) {
    +      // The exception message has all the details.
    ...
    +      // The exception message has all the details.
    

    Maybe this comment doesn't need to be repeated, and could happen once before the first catch?

  3. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
    @@ -197,6 +197,7 @@ public function testsBlockContentAddTypes() {
    +      $this->drupalPlaceBlock('local_actions_block');
    
    +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
    @@ -334,6 +334,7 @@ public function testSwitchDefaultTheme() {
    +    $this->drupalPlaceBlock('local_tasks_block');
    

    These seems out of scope. But lo, they're not, as the very helpful comment #72 makes clear. However, all that research and goodness is missing in these tests now. Wonder if we can summarize/simplify #72 into some code comments about this?

  4. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -74,6 +74,11 @@ abstract class UpdatePathTestBase extends BrowserTestBase {
       protected $installProfile = 'standard';
    ...
    +  protected $profile = 'standard';
    

    This was a minor head-scratching WTF when looking at the patch and the surrounding context. We actually have two separate protected properties, both about 'profile', both set to standard? Huh? ;) I'm sure it's off topic for this patch, but what gives? Hah, right, comment #74 to the rescue. ;) Carry on, then. But maybe #NeedsFollowup?

  5. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
    @@ -140,6 +140,8 @@ public static function getSkippedDeprecations() {
    +      'Drupal\Tests\BrowserTestBase::$theme is required in drupal:9.0.0 when using the default "testing" install profile. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use',
    

    If we change point #1, this line will also need the trailing period.

That's all I can find with a careful review. Bot's happy, and this is all minor cosmetic stuff, so I'm definitely not going to call this 'needs work'. In fact, I'll just call it RTBC, and if @Wim Leers (or someone else) wants to re-roll to address any of this, we can reconsider.

Totally on board with the plan and scope, now that the roadmap is clear. This is a hugely important fix, so let's get it in ASAP!

Thanks again,
-Derek

Wim Leers’s picture

The patch at #3082655-2: Specify the $defaultTheme property in all functional tests was posted about 6 hours before #83 was posted, but I can totally see how drafting that comment would've taken a long time. To be even more fair: #3083275: [meta] Update tests that rely on Classy to not rely on it anymore did not exist at all yet! So I completely understand your hesitation in #83 😊

Also, huge thanks to both @Wim Leers and @lauriii for responding so thoughtfully and calmly, since re-reading my comments, I'm sure they came off as annoying and unnecessarily heated. Huzzah for everyone keeping their cool, and apologies for my tone.

❤️ I appreciate that! They did come across like that a bit, but we all sometimes have a more emotional day, so really no need to apologize! 😊


  1. ✅ — I was just following the pattern in most other deprecation errors but I really don't care either way, so done!
  2. ✅👍
  3. 👎 — if you grep for $this->drupalPlaceBlock('local_tasks_block', you'll find dozens upon dozens of matches. They don't have comments. The reason we had to do it here in this issue instead of it having already happened in #507488: Convert page elements (local tasks, actions) into blocks like for all those other occurrences, is that the tests here are kinda edge-casey. The changes in this issue make these tests less edge-casey and therefore I don't think we should sprinkle additional comments here, unlike those dozens of other occurrences.
  4. 👍 Agreed! Created #3083588: Remove \Drupal\FunctionalTests\Update\UpdatePathTestBase::$installProfile, use the inherited $profile property instead.

Thanks for your very thoughtful review!

Wim Leers’s picture

FileSize
2.8 KB

😅

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
    @@ -359,6 +361,18 @@ protected function initConfig(ContainerInterface $container) {
    +    if ($this->profile === 'testing') {
    +      if (empty($this->theme)) {
    +        @trigger_error('Drupal\Tests\BrowserTestBase::$theme is required in drupal:9.0.0 when using the default "testing" install profile. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.', E_USER_DEPRECATED);
    +        $this->theme = 'classy';
    +      }
    +      $config->getEditable('system.theme')
    +        // @see core/modules/system/config/install/system.theme.yml
    +        ->clear('admin')
    +        ->set('default', $this->theme)
    +        ->save();
    +    }
    
    +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -114,6 +114,13 @@ abstract class BrowserTestBase extends TestCase {
    +  /**
    +   * The theme to install as the default for testing.
    +   *
    +   * @var string
    +   */
    +  protected $theme;
    

    One thing that seems a bit tricky is that the $theme property is present for all tests. If someone sets the profile to standard or whatever and sets the $theme property it is going to be ignored. I think this is quite unexpected behaviour.

  2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
    @@ -140,6 +140,8 @@ public static function getSkippedDeprecations() {
    +      // @todo Remove in https://www.drupal.org/project/drupal/issues/3082655
    +      'Drupal\Tests\BrowserTestBase::$theme is required in drupal:9.0.0 when using the default "testing" install profile. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.',
    

    This makes me quite uncomfortable. Also is the plan to require all tests in the future that use the testing profile to have to set a $theme property?

dww’s picture

Re: #92.3: sure, I'll buy that. I don't have my face deep in our test code often enough to feel strongly. Seemed like a lot of good research went into adding those lines, and future-me generally appreciates comments that capture such research. But if you think these are edge-casey tests that were getting some setup "for free" b/c of weirdness, which they now have to explicitly do (like most tests), than fine.

Re: #92.4: thanks for the follow-up. Wow, what a mess. ;)

- - -

Re: #94.1: Hrm. I'm not totally sure that's true. :) I *believe* the $theme property will take precedence over the $profile property. Quick attempts to test this locally aren't working, since my environment is a bit fubar right now. :( Maybe @Wim Leers can confirm/deny?

Re: #94.2: What about it makes you uncomfortable? Indeed, that's the plan. See #3082655: Specify the $defaultTheme property in all functional tests and #3083275: [meta] Update tests that rely on Classy to not rely on it anymore.

Jaesin’s picture

For #94.1, you can see the them only gets set automatically for the 'testing theme'.

For #94.2, This was also mentioned previously by @Berdir in #60 but kind of dismissed in #68. I agree with @alexpott as a matter of DX. I don't think there is a good enough reason to add the deprecation when it can easily fall back to the default theme. I think a change record that explains the default theme may change between majors versions is enough.

I think the other work here is still valuable though. I like the idea of being able declare the theme as a property in the test. This should work regardless of the installation profile IMHO.

lauriii’s picture

Issue summary: View changes
Wim Leers’s picture

@alexpott, @lauriii and I had a long video call two long video calls about this issue. Turns out @alexpott had more concerns than he raised in #94 but omitted those because the ones in #94 were the most concrete ones.

Outcomes:

  1. ::$theme should be renamed to ::$defaultTheme. Because A) that's what it is, B) ::$theme is too similar to ::$modules, which has very special behavior (automatic merging of all ancestral values)
  2. Consequently, ::installThemeFromClassProperty() needs to be renamed.
  3. Making ::installThemeFromClassProperty() catch exceptions is unproductive, it's better to let it throw exceptions and let it trigger explicit errors.
  4. Rather than making ::$defaultTheme work only for the testing install profile, we need to allow every test with any install profile use it. As a general principle, but also because it's an immensely valuable thing for install profile maintainers.

    For example: core's NodeEditFormTest may be subclasses, switched from ::$profile = 'testing' to ::$profile = 'thunder', but in order to keep the existing assertions working as-is, it would need to set ::$defaultTheme = 'classy' — i.e. to use another install profile, but not that install profile's theme.

  5. In the patches above, ::$defaultTheme was an instruction, not necessarily an introspective value: for profiles other than testing it would have been NULL. We $defaultTheme to automatically be set to the … default theme (system.theme config's value for the default key) of the install profile being used. This also allows us to remove all the if ($this->profile === 'testing') checks: every install profile that does not specify a default theme will require tests using that install profile to specify ::$defaultTheme.

So, implemented all that. Issue summary updated.

Wim Leers’s picture

alexpott  3 hours ago
I think we should add a new installDefaultTheme() function - that
• Installs the default theme
• triggers any necessary deprecations
• Sets the default theme.


alexpott  3 hours ago
So we have things in initConfig and in other places

alexpott  3 hours ago
And this method + the $defaultTheme propery can have all the necessary docs

alexpott’s picture

+++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
@@ -403,6 +406,40 @@ protected function initKernel(Request $request) {
+    // If the used install profile does not configure a default theme, require a
+    // default theme to be specified. For backwards compatibility, tests using
+    // the 'testing' install profile on Drupal 8 automatically get 'classy' set,
+    // but those tests will have to be updated before Drupal 9.0.0.
+    $default_install_path = drupal_get_path('profile', $this->profile) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
+    $profile_config_storage = new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION);
+    $profile_default_theme = $profile_config_storage->read('system.theme');
+    if (!$profile_default_theme) {
+      @trigger_error('Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.', E_USER_DEPRECATED);
+      if ($this->profile === 'testing') {
+        $this->defaultTheme = 'classy';
+      }
+      $container->get('theme_installer')->install([$this->defaultTheme], TRUE);
+      $container->get('config.factory')->getEditable('system.theme')
+        // @see core/modules/system/config/install/system.theme.yml
+        ->clear('admin')
+        ->set('default', $this->defaultTheme)
+        ->save();
+    }

This is not quite what I expected.

I thought this would be something like

    // Use the install profile to determine the default theme if configured.
    $default_install_path = drupal_get_path('profile', $this->profile) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
    $profile_config_storage = new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION);
    if (!isset($this->defaultTheme) && $profile_config_storage->exists('system.theme')) {
        $this->defaultTheme = $profile_config_storage->read('system.theme')['default'];
    }

    // Require a default theme to be specified at this point.
    if (!isset($this->defaultTheme)) {
      // For backwards compatibility, tests using the 'testing' install profile
      // on Drupal 8 automatically get 'classy' set, and other profiles use
      // stark.
      @trigger_error('Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.', E_USER_DEPRECATED);
      $this->defaultTheme = $this->profile === 'testing' ? 'classy' : 'stark';
    }

    // Ensure the default theme is installed.
    $container->get('theme_installer')->install([$this->defaultTheme], TRUE);

    $system_theme_config = $container->get('config.factory')->getEditable('system.theme');
    if ($system_theme_config->get('default') !== $this->defaultTheme) {
      $system_theme_config
        // @see core/modules/system/config/install/system.theme.yml
        ->clear('admin')
        ->set('default', $this->defaultTheme)
        ->save();
    }

So we default the value to the profile's default theme if supplied. If the test overrides this value we respect it regardless of what the install profile says. We make classy the default for the testing profile. I also suddenly realise that it is no certainty that contrib profiles ship with system.theme and so we should default to stark (as system module does) if the profile is not testing and does not have a default theme as that is the current behaviour.

Also I just noticed we're also changing the admin theme - I wonder why we're doing that if is it necessary.

The last submitted patch, 98: 2352949-97.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 99: 2352949-98.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
1.76 KB
22.84 KB

So we default the value to the profile's default theme if supplied. If the test overrides this value we respect it regardless of what the install profile says. We make classy the default for the testing profile. I also suddenly realise that it is no certainty that contrib profiles ship with system.theme and so we should default to stark (as system module does) if the profile is not testing and does not have a default theme as that is the current behaviour.

This took me a while to digest, especially this bit:

    $this->defaultTheme = $this->profile === 'testing' ? 'classy' : 'stark';
…
    $system_theme_config = $container->get('config.factory')->getEditable('system.theme');
    if ($system_theme_config->get('default') !== $this->defaultTheme) {

I had started writing a comment describing my understanding, but that ended up looking exactly like what you wrote 😀 So … yep, makes sense! Although to be honest, I don't see how/when a test could even change the system.theme config because that'd require something like

  protected function setUp() {
    $this->container->get('config.factory')->set(…)
    parent::setUp();
  }

… but … $this->container is not yet available at that time.

Still, I like your logic better: it's more robust. Used it, and only fixed a few comment nits.

Wim Leers’s picture

D'oh, #103 only contains #100's proposed changes.

This is what the interdiff-100-103.txt interdiff was changing, and so what the above patch should've been.

EDIT: lol and I apparently uploaded the wrong patch ending in -103. What can I say? I have too many patches that I'm juggling. 🤹‍♀️

Wim Leers’s picture

Also I just noticed we're also changing the admin theme - I wonder why we're doing that if is it necessary.

See #56: that interdiff introduced it, and fixed 4 failures: ConfigImportInstallProfileTest, ConfigInstallProfileOverrideTest, InstallProfileDependenciesBcTest and InstallProfileDependenciesTest.

But … it seems that thanks to the restructured approach, that just isn't necessary anymore.

alexpott’s picture

  1. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
    @@ -403,6 +406,47 @@ protected function initKernel(Request $request) {
    +    $default_install_path = drupal_get_path('profile', $this->profile) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
    
    +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -74,6 +74,11 @@ abstract class UpdatePathTestBase extends BrowserTestBase {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected $profile = 'standard';
    

    I think if instead of using $this->profile here we used the profile from the container then we wouldn't need to make the changes to UpdatePathTestBase. The install profile is available as a param on the container - $container->getParameter('install_profile'). Yes we should still refactor InstallerTestBase::$installProfile to be the same as InstallerTestBase::$profile on that class but at least we don't introduce the strange double variable in this issue.

  2. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTranslationTest.php
    @@ -19,6 +19,11 @@ class InstallerTranslationTest extends InstallerTestBase {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected $profile = 'standard';
    

    So this change highlights something quite interesting. InstallerTestBase tests that use the testing install profile are now using stark. So how come? It's because they are using the interactive installer. This means that defaults set in install_profile_info() are being merged in. Which means that the stark theme is being installed. However in order to maintain BC here we really shouldn't be changing this here. We need to add $this->installDefaultThemeFromClassProperty($this->container); to \Drupal\FunctionalTests\Installer\InstallerTestBase::setUp(). (at the end of the if ($this->isInstalled) { clause).

Status: Needs review » Needs work

The last submitted patch, 105: 2352949-105.patch, failed testing. View results

alexpott’s picture

Oh wow. So we've uncovered a bug here that's been hidden but the testing profile's system.theme.yml because it didn't have an admin theme value or key. This is an impossible situation for a site to get into. I feel (very unfortunately) that we need to sort out the admin theme mess before we can fix this issue because the bug makes it very hard to remove the testing profile config.

I'll open a separate issue (or find existing issues) to discuss.

alexpott’s picture

Wim Leers’s picture

Title: Deprecate using Classy as the default theme for the 'testing' profile » [PP-1] Deprecate using Classy as the default theme for the 'testing' profile
Status: Needs work » Postponed

Per #108, we're blocking this on fixing the system.theme's default key' value mess in #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense.

Wim Leers’s picture

  1. #106.1: TIL! I didn't know this 🤓🙏That makes sense. It reminds me of #98.5: ::$profile is an instruction, not an introspective value. So by just introspecting instead, we remove this requirement. I like it!
  2. #106.2: 🦅👁 → ✅
Wim Leers’s picture

Title: [PP-1] Deprecate using Classy as the default theme for the 'testing' profile » Deprecate using Classy as the default theme for the 'testing' profile
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 111: 2352949-111.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
@@ -403,6 +406,48 @@ protected function initKernel(Request $request) {
+    $profile = $container->getParameter('install_profile');
+    $default_install_path = drupal_get_path('profile', $profile) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
+    $profile_config_storage = new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION);
+    if (!isset($this->defaultTheme) && $profile_config_storage->exists('system.theme')) {
+      $this->defaultTheme = $profile_config_storage->read('system.theme')['default'];

We could refrain from checking the installation profile configuration when the ::defaultTheme property is set. I don't think this leads into any meaningful change so I'm still going to go ahead and RTBC this. ✌️

Jaesin’s picture

This just skips the profile check if the defaultTheme is already set.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 116: 2352949-116.patch, failed testing. View results

Jaesin’s picture

Accidentally reset the default theme.

Jaesin’s picture

Status: Needs work » Needs review
alexpott’s picture

--- /dev/null
+++ b/111-116-interdiff.txt

The interdiff has crept into the #118 patch - in fact lots of stuff seems to be in that patch.

Also I'm not convinced that the change in #116 and suggested by #115 is correct. Yes we could save on doing

+    $profile = $container->getParameter('install_profile');
+    $default_install_path = drupal_get_path('profile', $profile) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
+    $profile_config_storage = new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION);

But these things are exceptional quick to do. We only actually read the install profile config if $defaultTheme is not set and that is (compared to these methods) the more expensive thing to do.

Jaesin’s picture

Welp, Trying again.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
@@ -403,6 +406,47 @@ protected function initKernel(Request $request) {
+    if (!isset($this->defaultTheme)) {
+      // Use the install profile to determine the default theme if configured and
+      // not already specified.
+      $profile = $container->getParameter('install_profile');
+      $default_install_path = drupal_get_path('profile', $profile) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
+      $profile_config_storage = new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION);
+      if ($profile_config_storage->exists('system.theme')) {
+        $this->defaultTheme = $profile_config_storage->read('system.theme')['default'];
+      }
+      // Require a default theme to be specified at this point.
+      @trigger_error('Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.', E_USER_DEPRECATED);
+      // For backwards compatibility, tests using the 'testing' install profile
+      // on Drupal 8 automatically get 'classy' set, and other profiles use
+      // 'stark'.
+      $this->defaultTheme = $this->defaultTheme ?: ($profile === 'testing' ? 'classy' : 'stark');
+    }
+
+    // Ensure the default theme is installed.
+    $container->get('theme_installer')->install([$this->defaultTheme], TRUE);

So this is doesn't look right to me. Because we'll trigger deprecation errors for profiles other than testing which provide a system.theme config and we don't want to.

As I said in #120 I don't think this is the correct change.

Jaesin’s picture

Status: Needs review » Reviewed & tested by the community

OK, Let's just roll this back to #111 then.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Okay committed #111.

Committed 2bb5476 and pushed to 8.8.x. Thanks!

Credited @dww, @alexpott, @Berdir, @dawehner, @tstoeckler, @xjm and @andypost for code review and comments that helped arrived at the solution.

  • alexpott committed 2bb5476 on 8.8.x
    Issue #2352949 by Wim Leers, akalata, Jaesin, lauriii, dww, alexpott,...
Wim Leers’s picture

🥳

Wim Leers’s picture

xjm’s picture

I published the change record for this issue.

xjm’s picture

Wim Leers’s picture

Wim Leers’s picture

xjm credited pameeela.

xjm’s picture

Issue summary: View changes

Adding the release note and crediting @pameeela for the change.

Status: Fixed » Closed (fixed)

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

oknate’s picture

Here's a follow-up: #3095598: defaultTheme deprecation warning should link to change record

I think the deprecation warning should link to the change record, rather than the issue, so that the recommendations for which theme to use in a test are easier to find.