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:
- #2396145: Option #description_display for form element fieldset is not changing anything
- #2419131: [PP-1] #states attribute does not work on #type datetime
- #2443815: #description_display broken for details elements
- #2945727: Form radios/checkboxes elements should have js-form-wrapper class
- #2982187: datetime-wrapper.html.twig ignores #description_display parameter
- #2988461: Radios template does not apply form-radios class
- ...
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
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.- 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
tostark
. 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. - 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 tostark
(or perhapsstable
). - 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
default
key). Read more in the change record for deprecating the use of Classy as the default for the testing profile.
Comment | File | Size | Author |
---|---|---|---|
#111 | 2352949-111.patch | 9.41 KB | Wim Leers |
#111 | interdiff.txt | 4.17 KB | Wim Leers |
#105 | 2352949-105.patch | 10.32 KB | Wim Leers |
#105 | interdiff.txt | 841 bytes | Wim Leers |
#104 | 2352949-104.patch | 10.42 KB | Wim Leers |
Comments
Comment #1
MKorostoff CreditAttribution: MKorostoff commentedComment #2
tstoecklerHmm... 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.
Comment #3
dawehnerMh, 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.
Comment #4
tstoecklerIMHO 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.
Comment #5
xjmAgreed @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.
Comment #6
xjmComment #7
dawehnerThis 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.
Comment #8
akalata CreditAttribution: akalata commentedMy tests aren't running locally, so seeing waht the bot says. And yes, I expect it to fail!
Comment #9
akalata CreditAttribution: akalata commentedComment #11
akalata CreditAttribution: akalata commentedI'm going to try and get this started after lunch!
Comment #12
akalata CreditAttribution: akalata commentedPost in-progress test conversion.
Comment #14
akalata CreditAttribution: akalata commentedMore in-progress test conversion.
Comment #16
akalata CreditAttribution: akalata commentedLet's try that again.
Comment #18
akalata CreditAttribution: akalata commentedReroll
Comment #20
akalata CreditAttribution: akalata commentedAnd trying this again (I'm out of practice)
Comment #22
akalata CreditAttribution: akalata commentedComment #27
Mile23Want to use stark for testing modal dialogs? Nope. #2936535: Modal dialog errors in stark theme
Comment #30
dwwWas 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
Comment #31
lauriiiComment #32
lauriiiTLDR; 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 likedata-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.
Comment #33
lauriiiComment #34
lauriiiComment #35
lauriiiDrupal 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.Comment #36
lauriiiWe should update the issue summary if we agree that #35 is the way to go.
Comment #37
Wim LeersThank 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.
Comment #38
Wim LeersI 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:
\Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations()
.classy
from the list of dependencies intesting.info.yml
without it breaking tests 🤓Will continue this in the morning.
Comment #40
Wim LeersThat seems to have worked. This skips that deprecation.
Comment #41
Wim LeersWhile #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 thetesting
profile's dependencies.Comment #42
Wim Leers#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()
. 🤞Comment #44
Wim LeersThe 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 thanclassy
.This should make the tests using an install profile other than
testing
pass tests.Comment #45
Wim Leers#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.
Comment #46
Wim LeersThe 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.Comment #47
andypostLast interdiff the
save()
should probably usesave(TRUE)
Comment #48
Wim LeersMake
\Drupal\Tests\minimal\Functional\MinimalTest
pass again — I missed theminimal
profile in #44.I also attempted to fix
\Drupal\simpletest\Tests\SimpleTestTest
in this reroll — but since that one can't be run usingphpunit
I'm letting DrupalCI verify that for me.Comment #49
Wim Leers#47 Interesting remark! But all the other config creation in
\Drupal\Core\Test\FunctionalTestSetupTrait::initConfig()
doesn't do that either, why should this one?Comment #50
Berdir> #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?
Comment #51
Wim Leers#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 :)
Comment #52
Wim Leers#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.)Comment #54
Wim Leers#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.
Comment #56
Wim LeersThe missing ingredient: taking the default from
core/modules/system/config/install/system.theme.yml
into account!Comment #58
lauriiiThis 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:
Comment #59
lauriiiDiscussed 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 ofstable
, 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 useclassy
for all existing tests. However, for new tests, this could be set tostable
orstark
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. Thetesting
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.Comment #60
BerdirWhy 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?
Comment #61
lauriiiIt 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 tostable
orstark
.In future there will be multiple versions of Stable (
stable
,stable9
andstable10
and so on). To make upgrading easier, we will support at least two versions simultaneously. Because of this, it's not possible forBrowserTestBase
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.Comment #62
dawehnerFor me it comes down to explicitness. Can someone figure out what the setup of a test is by looking at the test class.
*
Comment #63
Berdir> 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?
Comment #64
lauriiiYou'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'm not sure I follow. If we set a default value to
classy
, what do we need the @trigger_error() for?Comment #65
Berdir> 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.
Comment #66
lauriiiSorry, 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.
Comment #67
jibranThis change may affect Drupal Testing Trait as well so tagging.
Comment #68
Wim Leers@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 thetesting
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 beclassy
), 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:
stark
stable9
(In Drupal 9, the default would be
stable9
, in Drupal 10, it would bestable10
, 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.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
toclassy
, and all new tests explicitly set$theme
tostable
(orstark
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).
Comment #69
Wim LeersThe first green checkmark for #68: here's the updated patch for that :)
Comment #70
lauriiiOpened follow-up to decide the default theme for testing in Drupal 9: #3081131: Decide default theme for testing.
Comment #72
Wim LeersThe
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 instark
. 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, thelocal_actions_block
andlocal_tasks_block
blocks must be explicitly placed by the user. This happens automatically when using thestandard
orminimal
install profile (thanks to their default config). But not when using thetesting
profile.\Drupal\Tests\system\Functional\System\ThemeTest::setUp()
for example was updated in that same issue to place thelocal_tasks_block
block for the default theme! But why did this then work fine before, but stopped working now? Because thetesting
install profile no longer has a default theme, and henceblock_theme_initialize()
(called byblock_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.\Drupal\Tests\system\Functional\System\ThemeTest::testSwitchDefaultTheme()
,ThemeInstaller::install()
returns early (beforehook_themes_installed()
is invoked, hence resulting in no blocks being duplicated. It returns early becausestark
is the default incore/modules/system/config/install/system.theme.yml
and hence it already is installed.\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 forbartik
andseven
, but not forstark
, 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!).
Comment #74
Wim LeersApparently
UpdatePathTestBase
does not just use the DB dump, it does still install Drupal … to some extent. And becauseUpdatePathTestBase
setsprotected $installProfile = 'standard';
and simply ignores the inheritedprotected $profile = 'testing';
, this results in theFunctionalTestSetupTrait
-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.Comment #75
Wim LeersOops, forgot this!
Comment #77
Wim LeersAh, 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 tostark
), but the assertion added in #2331991: Theme missing when installing in non-English language is specifically asserting the presence of aclassy
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
… 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:
Comment #78
Wim LeersThis was the follow-up we still needed. Created #3082655: Specify the $defaultTheme property in all functional tests for that.
Comment #79
Wim LeersI think we're done here! 🥳
Comment #80
Wim LeersComment #81
Wim LeersChange 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.
Comment #82
Wim LeersUGHHHHH #81 includes a different patch I'd applied for testing 🤦♂️ 😅
Comment #83
dwwThanks 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:
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.
Comment #84
Berdir> 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.
Comment #85
dwwI 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
Comment #86
dwwp.s. I don't grok this:
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
Comment #87
dwwp.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...
Comment #88
Wim Leers@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
tostark
. 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 tostark
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.
Comment #89
lauriiiTests 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.
Someone could have build JavaScript or CSS that relies on certain HTML structure. For example, something like
#edit-field-date-range > input
would break.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.
Comment #90
Wim LeersClarification for #88 (and issue summary updated):
classy
tostark
. 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.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 tostark
(or perhapsstable
).Once those two follow-ups are completed, @dww's concerns will have been addressed.
Comment #91
dwwFirst, 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:
Smallest of nits: Since this isn't ending with the link, but includes additional stuff afterwards, it should probably end in a period. ;)
Maybe this comment doesn't need to be repeated, and could happen once before the first
catch
?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?
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?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
Comment #92
Wim LeersThe 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 😊
❤️ 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! 😊
$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.Thanks for your very thoughtful review!
Comment #93
Wim Leers😅
Comment #94
alexpottOne 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.
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?
Comment #95
dwwRe: #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.
Comment #96
Jaesin CreditAttribution: Jaesin at Chapter Three commentedFor #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.
Comment #97
lauriiiComment #98
Wim Leers@alexpott, @lauriii and I had
a long video calltwo 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:
::$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)::installThemeFromClassProperty()
needs to be renamed.::installThemeFromClassProperty()
catch exceptions is unproductive, it's better to let it throw exceptions and let it trigger explicit errors.::$defaultTheme
work only for thetesting
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.::$defaultTheme
was an instruction, not necessarily an introspective value: for profiles other thantesting
it would have beenNULL
. We$defaultTheme
to automatically be set to the … default theme (system.theme
config's value for thedefault
key) of the install profile being used. This also allows us to remove all theif ($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.
Comment #99
Wim Leers✅
Comment #100
alexpottThis is not quite what I expected.
I thought this would be something like
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.
Comment #103
Wim LeersThis took me a while to digest, especially this bit:
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… 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.
Comment #104
Wim LeersD'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. 🤹♀️Comment #105
Wim LeersSee #56: that interdiff introduced it, and fixed 4 failures:
ConfigImportInstallProfileTest
,ConfigInstallProfileOverrideTest
,InstallProfileDependenciesBcTest
andInstallProfileDependenciesTest
.But … it seems that thanks to the restructured approach, that just isn't necessary anymore.
Comment #106
alexpottI 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.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 theif ($this->isInstalled) {
clause).Comment #108
alexpottOh 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.
Comment #109
alexpottYep there is an existing issue - of course there is - #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense
Comment #110
Wim LeersPer #108, we're blocking this on fixing the
system.theme
'sdefault
key' value mess in #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense.Comment #111
Wim Leers::$profile
is an instruction, not an introspective value. So by just introspecting instead, we remove this requirement. I like it!Comment #112
Wim Leers#2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense landed! Re-testing #111.
Comment #114
Wim LeersComment #115
lauriiiWe 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. ✌️Comment #116
Jaesin CreditAttribution: Jaesin at Chapter Three commentedThis just skips the profile check if the
defaultTheme
is already set.Comment #118
Jaesin CreditAttribution: Jaesin at Chapter Three commentedAccidentally reset the default theme.
Comment #119
Jaesin CreditAttribution: Jaesin at Chapter Three commentedComment #120
alexpottThe 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
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.
Comment #121
Jaesin CreditAttribution: Jaesin at Chapter Three commentedWelp, Trying again.
Comment #122
alexpottSo 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.
Comment #123
Jaesin CreditAttribution: Jaesin at Chapter Three commentedOK, Let's just roll this back to #111 then.
Comment #124
alexpottOkay 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.
Comment #126
Wim Leers🥳
Comment #127
Wim LeersNext steps:
Comment #128
xjmI published the change record for this issue.
Comment #129
xjmComment #130
Wim LeersComment #131
Wim LeersFollow-up created: #3087043: Follow-up for #2352949: themes that include required configuration that depends on modules not handled correctly.
Comment #133
xjmAdding the release note and crediting @pameeela for the change.
Comment #135
oknateHere'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.