Problem/Motivation

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/3083055, which includes recommendations on which theme to use.

Proposed resolution

Some of the submodules' tests are broken right now. Fix this deprecation warning and coding standards can not bring back them to green.
So let's only fix those submodules which only have these deprecation warnings and coding standards issues.

As examples are small/tiny modules, I think it's ok to fix other tests/issues and this tiny issue at the same. Let the submodules untouched by the patch of this issue to fix its own deprecation warning later.

So this issue is targeting to fix this warning and coding standards of those corresponding submodules.

CommentFileSizeAuthor
#13 3103518-13.patch2.56 KBjungle
#8 3103518-8.patch3.93 KBjungle
#2 default-theme-3103518-2.patch16.05 KBjungle

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Assigned: jungle » Unassigned
Status: Active » Needs review
StatusFileSize
new default-theme-3103518-2.patch16.05 KB
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Parent issue: » #3072416: [Meta] Drupal 9 Deprecated Code Report
valthebald’s picture

valthebald
Location Sofia
commented 
+++ b/config_entity_example/src/Form/RobotFormBase.php
@@ -162,6 +164,7 @@ class RobotFormBase extends EntityForm {
+  // phpcs:disable

why disable phpcs?

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented 
  /**
   * {@inheritdoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {
    parent::validateForm($form, $form_state);

    // Add code here to validate your config entity's form elements.
    // Nothing to do here.
  }

Hi @valthebald. If do not disable it. phpcs would complain:

 168 | WARNING | Possible useless method overriding detected

Yes, it's a useless method overriding. But the comment inside should be there. Disable it or remove the comment.

  • marvil07 committed fe63c80 on 8.x-1.x 
    Issue #3103518: Do not rely on classy output during examples toolbar...
marvil07’s picture

marvil07 commented
Status: Needs review » Needs work

@jungle: thanks for the patch here.
@valthebald: Hi! Nice to meet another maintainer.

On the phpcs warning, I guess it is OK to have that warning given the idea is to point to where to add the code.

Currently the provided patch seems to not apply anymore, so moving to NW.

I started with the examples module code, which seems to not yet be covered on the patch, and added that commit directly.
I needed to change a bit the logic on how the links were generated, so it is clear we are testing for something we know we are adding.

I see the patch is adding one protected $defaultTheme = 'classy'; , but I guess we should instead try to change the behavior to not rely on classy output, but I see all the rest is adding stark theme which is great.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Related issues: +#3103586: [Meta] Bring back all tests GREEN
StatusFileSize
new 3103518-8.patch3.93 KB

Yes, protected $defaultTheme = 'classy' should not be used, I was in a hurry to get all test cases passed while working on #3103586: [Meta] Bring back all tests GREEN, changing it to stark breaking existed tests I remember, so just kept it as classy for quick.

@marvil07 thanks for reviewing.

Re-rolled patch from #2

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review
marvil07’s picture

marvil07 commented
Status: Needs review » Needs work

@jungle: Thanks for the new iteration here.

Indeed it now applies, but I think most of the changes here are no longer relevant.

Several of them are adding protected $defaultTheme = 'stark'; to different classes that already have them.
I am guessing that was not the case when this ticket was opened, as the first attached patch suggests.
For instance, just to point one example:

--- a/batch_example/tests/src/Functional/BatchExampleWebTest.php
+++ b/batch_example/tests/src/Functional/BatchExampleWebTest.php
@@ -18,6 +18,11 @@ class BatchExampleWebTest extends BrowserTestBase {
    */
   protected $defaultTheme = 'stark';
 
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
   /**
    * {@inheritdoc}
    */

See the already defined data member <? $defaultTheme ?> before the lines added.

I also see several of the target deprecation messages triggered on the last branch test run.
Hence, I am moving this ticket back to NW.
@jungle: I will be taking a look at related issues, so this may be fixed as part of other test fixes; feel free to mark this as postponed if you think it is better to wait for the rest to get in.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Ooops, sorry, @marvil07, yes, you are right! I should check it in details myself. I think it is ok to close this issue now, nothing to do to meet the scope as the title indicated, right?

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Assigned: Unassigned » jungle

2x: 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/3083055, which includes recommendations on which theme to use.

Sorry again, from the latest test run, https://www.drupal.org/pift-ci-job/1597488, yes, it still needs work and I will work on it.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review
StatusFileSize
new 3103518-13.patch2.56 KB

Status: Needs review » Needs work

The last submitted patch, 13: 3103518-13.patch, failed testing. View results

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review

Checked https://www.drupal.org/pift-ci-job/1597504, no more $defaultTheme relevant notices.

govind.maloo’s picture

govind.maloo commented
Status: Needs review » Reviewed & tested by the community

#13 Patch is clean and able to apply. No more error related to the default theme.

valthebald’s picture

valthebald
Location Sofia
commented

  • valthebald committed d4fbc5e on 8.x-1.x 
    Issue #3103518 by jungle, marvil07, govind.maloo: Deprecation notices:...
valthebald’s picture

valthebald
Location Sofia
commented
Assigned: jungle » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed to 8.x-1.x, thank you!

Status: Fixed » Closed (fixed)

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