Update standard profile so Olivero is the default theme

Let's get rolling. Updated the standard profile dependency as well as the block configs. I have no idea if this is the right placement of the right blocks though. I did change footer_fifth and sidebar_first to footer_bottom and sidebar respectively.

I did not find any other profile that depends on bartik.

Now this exposes some tests that depend on Bartik :D It has an interesting database fail that is hopefully random, and then a bunch of fails about cache tags (which depend on what data the theme pulls for the page, so sounds related) and the lack of contact link results in multiple fails.

IMHO first the block placements need to be verified before we get into fail fixing and I don't think I am the right person for that.

So Olivero actually has its own blocks fine, we can just remove the Bartik ones and not interfere with what Olivero adds anyway. Whether we should move the Olivero ones to Standard is a discussion to itself.

Pretty much the same fails. So. those need to be resolved :) I ran out of time for today for this though. Feel free to pick it up!

I tested #5 manually, and you get this. :)

We seem to be missing the "default" bit somehow. (Note that both Seven and Olivero have the "Set as default" link active)

Oh, duh. Let's make it the default too :D As you can tell I did not actually try these yet.

Lookin' great now! :D

The fails look logical. Like

The string "No front page content has been created yet." was not found anywhere in the HTML response of the current page.

as well as fails around comment posting and editing form elements and breadcrumb formatting, etc. specifically for the default theme. These are different with Olivero, so the tests need to be updated either to their Olivero format or to the Stark output (and use the Stark theme).

1. Done: Updated entity counts in two migration tests. It makes total sense that they would be different since we are now migrating into Olivero.
2. Done: Updated expected welcome text. Can I say again how much more welcoming it is? :)
3. Todo: Poked around in the breadcrumb test, but it does not seem like its trivial. The links exist as expected in //nav[@class="breadcrumb"]/ol/li/a. Added a todo.
4. Discuss: Footer menu block is not shown in Olivero currently. That had the contact link that is being tested for. Added a todo.
5. Random: LayoutBuilderDisableInteractionsTest looks like a random frontend fail. See #3208791: [random test failure] Random fail in LayoutBuilderDisableInteractionsTest.
6. Discuss: The comment Save link is "Post comment" in Olivero. This does not map exactly well to editing comments, since you are not posting the comment, just saving it. Updated the test for now.
7. No idea: the page cache tag test looks suspicious. Does Olivero try to access data in a template or preprocess before Drupal is even properly installed?

Also unrelatedly found a test that was verifying Bartik does not show up as a backend theme in Standard. That will of course not happen now :D Made it test Olivero to be more realistic.

Let's see what do we get with this.

Patch doesn't apply anymore, added a re-roll tag

Reroll patch #15

This patch should clear the PageCacheTagsIntegrationTest::testPageCacheTags error

1. +++ /dev/null
   @@ -1,24 +0,0 @@
   -status: true
   -dependencies:
   -  config:
   -    - system.menu.account
   

   does Olivero provide its own default blocks? I'd argue that it should not and that we should update/change all (that make sense) block configs here instead of removing them.

   It's the same with modules. Opinionated config creation should be part of the install profile, so that other install profiles can easily create their own blocks.

2. +++ b/core/tests/Drupal/FunctionalTests/Installer/StandardInstallerTest.php
   @@ -18,8 +18,8 @@ class StandardInstallerTest extends ConfigAfterInstallerTestBase {
      public function testInstaller() {
   -    // Verify that the Standard install profile's default frontpage appears.
   -    $this->assertRaw('No front page content has been created yet.');
   +    // Verify that Olivero's default frontpage appears.
   +    $this->assertRaw('Congratulations and welcome to the Drupal community!');
        // Ensure that the contact link enabled in standard_install() works as
   

   This is the same thing. If you want to use olivero in your install profile you likely don't want to have this text or something else.

   I see the file even has a @todo to make it part of the default frontpage view, so we could just move it into the default frontpage view... which is part of node.module. So much for that theory :) But doing that in the existing view at least means that any site that already customizes that view and overrides it keeps their existing override.

   IMHO doing that is either in scope of this issue or a blocker?

Opened a MR with patch #20

I agree with #21, it's usually painful to deal with unwanted config that is installed by a module/theme. By moving it into the profile, this is easier to manage.

In the MR above I've moved Olivero blocks into the `optional` config install. I also updated a few tests but ran out of time. It seems there is still quite a lot of work to do in tests here.

The fail from Drupal\Tests\views\Kernel\Plugin\DisplayPageTest::testEmptyRow is interesting. The test looks at views markup for a grid view with 5 columns. It looks for columns using an xpath expression (//div[contains(@class, "views-col")]) using the .views-col class. Olivero does not use that class for grid items (see the views-view-grid.html.twig template). Furthuremore, Olivero ignores a handful of the views grid settings:

We can fix this test by just adding views-col alongside views-view-grid__item-inner. Or we could fix the test by changing the xpath expression used to olivero. I've pushed a commit doing the later. Perhaps if more discussion is needed on this point, it could be moved to a follow up.

I think the blocks have been moved to standard profile and the tests are probably using minimal, then that's expected and consistent with other core themes. So if you expect them to be there then I think we would need to set them up. Or have a test module that does that for example.

Exactly. We could copy & paste all those blocks also into the module and then it should work as before if you enable that module on minimal. We might want to put them in install/optional though, because if you for some reason enable that test module on standard, which defines the same blocks, then it would fail if they are mandatory config in config/install as it already exists.

A couple more green to me.

In \Drupal\FunctionalTests\Installer\StandardInstallerTest::testInstaller() , there was this code:

    // Ensure that the contact link enabled in standard_install() works as
    // expected.
    $this->clickLink('Contact');
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->addressEquals('contact');

which didn't make much sense to me, since standard_install() isn't really doing what the comment says it's doing?

/**
 * Implements hook_install().
 *
 * Perform actions to set up the site for this profile.
 *
 * @see system_install()
 */
function standard_install() {
  // Assign user 1 the "administrator" role.
  $user = User::load(1);
  $user->roles[] = 'administrator';
  $user->save();

  // Populate the default shortcut set.
  $shortcut = Shortcut::create([
    'shortcut_set' => 'default',
    'title' => t('Add content'),
    'weight' => -20,
    'link' => ['uri' => 'internal:/node/add'],
  ]);
  $shortcut->save();

  $shortcut = Shortcut::create([
    'shortcut_set' => 'default',
    'title' => t('All content'),
    'weight' => -19,
    'link' => ['uri' => 'internal:/admin/content'],
  ]);
  $shortcut->save();
}

So regardless of who was placing the "contact" link in Bartik/Standard, IMHO we don't need to test that if Olivero doesn't have the same link on the frontpage. I changed the test code to be:

    // Ensure that the contact form works.
    $this->drupalGet('/contact');
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->addressEquals('contact');
    $this->assertSession()->pageTextContains('Website feedback');

But I'm leaning towards just ripping these lines off, if this test isn't about testing that. Let me know if I'm missing something here.

Sorry, @Gábor Hojtsy had already mentioned it in #15.4:

4. Discuss: Footer menu block is not shown in Olivero currently. That had the contact link that is being tested for. Added a todo.

IMHO, the answer to this question:

/* @todo Footer menu block with contact link is not exposed in Olivero. Should it?

is no. We can assert the footer is there by checking the "Powered by Drupal" string, doesn't it achieve the same goal?

Also, regarding #15.6:

Discuss: The comment Save link is "Post comment" in Olivero. This does not map exactly well to editing comments, since you are not posting the comment, just saving it. Updated the test for now.

I'm attempting in the last commit an approach where we can minimize the disruption to how CommentTestBase::postComment() works, by adding an extra optional parameter to receive the submit button label. My reasoning is that this allows us to touch less code in this patch, which is always good.

There seems to be extra mismatches in CommentPreviewTest::testCommentEditPreviewSave() , notably around date formats, but I've run out of time for today.

Just clarifying my previous comment... I'm not saying we shouldn't be having the discussion "Save" vs "Post comment" in Olivero comment edit forms, but I believe it would be better if we could fix the tests here with what Olivero has now, and then open a follow-up to discuss if those forms should have one or the other label for the submit button.

The test failure for Cache.Drupal\Tests\page_cache\Functional\PageCacheTagsIntegrationTest is an odd one.

I already cleared up most of it in #20

There is one issue that when I was first testing I thought was an artifact of something on my local. The test creates two nodes. A block is also created that is set to only show up on the second nodes page.

The the cache tags of the first node are inspected. The cache tags seem to indicate that it is looking at the front page (cache tags like url.path.is_front appear). I haven't been able to trace down why this is happening yet.

> The cache tags seem to indicate that it is looking at the front page (cache tags like url.path.is_front appear)

That's a cache context, not a path. Can't check the test right now, but could be block visibility conditions to show or not show on ?

🎉 Green!
We can finally start discussing / reviewing this.

Patch is not applying showing error for ref sharing screenshot ...

Re #41 @vikashsoni, the issue switched from using patches to a issue fork in #23, so the patch is no longer relevant. (If you are applying patches in an issue, it's only the most recent patch you need to apply - your screenshot shows an attempt to apply patch #19 when theres a newer one in #20, they are not cumulative. Just add the most recent)

But in this case, this issue switched from patch to an issue fork several weeks ago, and the automated tests show it "applies" fine. If you want to apply the merge request as a patch, take the merge request URL and add ".patch". For example, the "patch" for this issue would be https://git.drupalcode.org/project/drupal/-/merge_requests/938.patch

To learn more about issue forks and merge requests see https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... - it is much easier than patches in my opinion.

Thanks everyone for reviewing! I haven't had time to look at the feedback, if others have time feel free to do so.
For the time being, I'm pointing the MR to 10.0.x. A rebase / reroll is probably a good next step, setting it to NW for that and to address the review feedback above.

I spent an hour trying to resolve the conflicts in the rebase then got into some circular conflicts. So I decided to push a new branch here. Will see if I can get back to applying the patch and resolving it that way on top of current 10.x tomorrow. If someone has time in the meantime feel free to take it :)

I went ahead and merged the 10.0.x branch into the feature branch that was more manageable than doing an actual rebase. Hope that's alright in terms of history etc. Of course this can be reverted, if that approach was not correct. But the resulting diff looks fairly good to me, although I haven't been involved in this issue very much.

No worries, @mherchel, glad I could help. Will try to find some time to address some of the open discussions on the merge request - #3257407: Use "content" region in BlockCreationTrait::placeBlock() instead of "sidebar_first" is a first stab at that. Would really love to see this land!

There are a lot of test changes in here which are swapping Bartik for Olivero. I think we should split those out to their own issue since they aren't directly coupled to making Olivero the default theme in Standard. Then it'll be a smaller MR to review here. Makes sense to do #3257407: Use "content" region in BlockCreationTrait::placeBlock() instead of "sidebar_first" in its own issue too, following that one now.

If there are a tests of the standard profile itself, then obviously fine to update those here as necessary.

Landed #3257407: Use "content" region in BlockCreationTrait::placeBlock() instead of "sidebar_first" (thanks @tstoeckler and @catch!) which changes what is exactly required of the tests a bit. But now that should be moved to #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default either way.

Opened some more issues to further unblock this:

1. #3259925: Use Olivero instead of Bartik as example theme in documentation: this does not actually block anything and didn't come up here yet, but grepping the codebase for Bartik I found a bunch of references in docs so thought we might as well update that at some point and was easy enough to quickly come up with a patch for, so why not.
2. #3259928: Change various tests that test with "all themes" to also include Olivero: Also not really blocking this, as this could and should be done independently of this issue but some of the changes already came up in the MR here, so I didn't want to remove them here without having a proper replacement issue. Related to #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default but distinct as far as I can tell.
3. #3259931: Change default comment save button label to "Post comment" / "Reply to comment" and remove respective Olivero override: This one may be an actual blocker unless Olivero - for the time being - reverts its override of the comment save button (or if @Berdir is overruled, I guess, and it's fine for the default theme to override core functionality). That one is next on my list to make some progress with.

Other than that will see if I can find some time to help out with #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default, now that I have the other test-related stuff out of the way.

Opened one more issue for a little thing that I found when grepping the codebase that also doesn't block this but also doesn't fit into #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default: #3262674: Use Claro instead of Bartik as fallback maintenance theme

@vikashsoni, Posting screenshots of your codebase or command-line interface does not advance the issue, since the automated testing infrastructure tells us whether the change set still applies correctly.

So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!

See the issue credit guidelines for more information.

I'm also hiding the patch files from the IS for clarity.

With #3259931: Change default comment save button label to "Post comment" / "Reply to comment" and remove respective Olivero override my preference in terms of getting things done, would be to remove the override (and required test changes) from Olivero/this issue, and handle that issue in its own right, then we don't have complicated dependencies between the two. However this might need yet-another-issue to make that change in Olivero.

I have put a rough patch on #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default.

Opened #3276615: Remove olivero_form_comment_form_alter() comment button label override with a patch.

I went through #3260592: Change Bartik-specific tests to Olivero (or appropriate alternative) in preparation for making Olivero the default and basically all the changes that originally should have been there are no-longer relevant except potentially one one-liner, because they already got updated in #3259928: Change various tests that test with "all themes" to also include Olivero. So... I've marked that as duplicate.

Opened #3276618: Olivero sets inconsistent classes for active trail between menu and book for one other hunk that seems wrong rather than an issue with switching from Bartik to Olivero.

I think what we probably need here is a new patch with #3276615: Remove olivero_form_comment_form_alter() comment button label override included and most of the test changes removed to see what's actually necessary now.

Opened another issue but one got committed in the meantime, so still PP-2.

Tagging as needs re-roll, would be good to see what the MR looks like now various issues have landed, and the two that are split-off are relatively small ones.

Had a look at rebasing this, but there are dozens of conflicts, so at least for now converting back to a patch to get an idea what's here. This was a quick re-roll and doesn't include the two blocking issues, so will definitely fail but question of how much.

OK several new migration tests for to-be-removed modules, looking at those first.

Fixing bartik => olivero related test failures in \Drupal\Tests\page_cache\Functional\PageCacheTagsIntegrationTest

I'm a bit unsure if this should be done in a sub-issue or in here, but since there's no way of testing this without the base-switch-theme-in-standard-profile patch in here, I'll add it in here for now.

Also opening a few follow-ups for some "interesting" but not directly related stuff I've stumbled over whilst working on that test.

Opened #3276839: Remove leftover dumpHeaders property and, much more important IMHO #3276842: Asserting arrays are equal fails for the wrong reason which baffles me.

#3276620: Change NoJavaScriptAnonymousTest to use stark theme landed. So #3276618: Olivero sets inconsistent classes for active trail between menu and book remains.

Rerolled. It was not applying due to Claro becoming the default admin theme.

The patch applies fine, but when I try to install from scratch I get the following error:

..
[notice] Performed install task: install_profile_themes
In EntityStorageBase.php line 553:

  'block' entity with ID 'olivero_secondary_local_tasks' already exists.

I rerolled the patch wrong... This should be much better.

#3276618: Olivero sets inconsistent classes for active trail between menu and book landed too. No issues remaining at this time that this would be postponed.

The manual test of the patch worked!

Of the four fails, this one is easy, because it loads the bartik block. Will wait for @eojthebrave to post his fixes to comments though :D

1) Drupal\Tests\system\Functional\Menu\BreadcrumbTest::testAssertBreadcrumbTrait
Error: Call to a member function delete() on null

/var/www/html/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php:425
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

This patch should fix the failure @Gábor Hojtsy mentions in #74, and the CommentPreviewTest failures. 🤞

The CommentPreviewTest test failures are caused by the fact that they are checking for specific markup that is changed by the Olivero theme.

• Olivero outputs dates for comments using an interval format like "14 days ago". But classy/bartik use the value set in template_preprocess_comment() which uses the system default date format. The fix for this is to update the test so that it checks for the date in the format that Olivero is using in olivero_preprocess_comment()
• The other failure is due to the fact that classy wraps the comments at the bottom of a node with '.comment-wrapper' and Olivero uses '.comments'

Notes to future me, or whoever works on this next. There is a BreadcrumbTest failure at line 293. The call there to $this->assertBreadcrumb($link_path, $trail, $term->getName(), $tree); is the only call to the method that uses $tree param in this test. That triggers a call to \Drupal\Tests\system\Functional\Menu\AssertMenuActiveTrailTrait::assertMenuActiveTrail - and it seems like the failure is occuring in there. Because the breadcrumb trail doesn't have any classes indicating it's part of the active trail. I think ...

Also this line "':menu' => 'block-bartik-tools'," just below that test. Line 302. Is likely going to cause issues too.

In case this is useful for helping others debug the remaining test failures.

Re #77, as far as I can tell, Olivero only sets menu__item--active-trail menu items, not breadcrumb trails. (Compare core/themes/olivero/templates/navigation/menu.html.twig, line 52 with core/themes/olivero/templates/navigation/breadcrumb.html.twig, line 18.) \Drupal\Tests\system\Functional\Menu\AssertMenuActiveTrailTrait::assertMenuActiveTrail expects this class from Olivero in line 39, if I'm reading that right.

Changing line 293 in BreadCrumbTest.php (core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php) to:

      $this->assertBreadcrumb($link_path, $trail, $term->getName());

(which removes the optional call to $tree)

And running the test (Drupal\Tests\system\Functional\Menu\BreadcrumbTest again, it still fails with this message:

There was 1 failure:

1) Drupal\Tests\system\Functional\Menu\BreadcrumbTest::testBreadCrumbs
Link to taxonomy/term/1 appears only once.
Failed asserting that actual size 0 matches expected size 1.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php:305
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

FAILURES!
Tests: 2, Assertions: 86, Failures: 1.

Don't have any more time to look at this this evening, but thought I would share what I discovered. (Basically verifying what @eojthebrave noted in #77.)

Added #3277592: Make MenuBreadCrumbTest work without standard profile to clean this test up some more

This fixes the breadcrumb test, not sure if the quickedit one is a random 🤞

not sure if the quickedit one is a random 🤞

After running the test again, and never seen this specific JS test failure before in the (too long) list of semi-random ones, I'm pretty sure it isn't a random fail, but actually related to this change.

The test fail normally indicates something is obscuring the element that it needs to click on

It sure is...a little... (Screenshot from a previous run)
Let's see if we can click the link in the Block instead of the whole Block which seems to have a liking of clicking in the 0.10% of the Block that's obstructed from view.
In other words: A more specific place to click would make clicking in the wrong place less unlikely

Right....

I _think_ there's something wrong with the swapping of blocks for bartik/olivero. Because when I look at the screenshots for the failing test above, the page (/node/1) seems _very_ wrong, with no page title and the powered_by block on top instead of below.

Since I'm definitely _not_ the man to do Front End stuff, I'll happily leave that in more capable hands.

Tests passed for Drupal 9.4.x for #81

Manual installation works for me, both for 10.0.x and 9.4.x. So moving to RTBC 🎉

The ckeditor failures appear to be new, but not related to this issue - HEAD is failing, and I reopened #3276974: [drupalMedia] Media View Modes don't work if alignment not enabled.

Here's an updated version of the patch from #81 for D9. This hardens the SettingsTrayTest that was randomly failing. It seems like without the blocks on the page it's a little faster and the test sometimes fails because the element hasn't been removed yet. So this will wait a little to give it time for the element to be removed.

And here's a version of the patch for Drupal 10. This includes the same fix for the settings tray tests from #93. And it moves all the Olivero block configuration from the standard profile into the olivero theme. There are a bunch of Nightwatch tests for Olivero that will only work if these blocks are present when the tests run.

Saving credits

Committed 0bd4c9f and pushed to 9.4.x. Thanks!

Committed live from the DrupalCon Portland 2022 Trivia Night!

🥳 🥳 🥳

Tagging for the release notes too since this is a change for new site installation.