Branding block should include config dependency on system.site config

This was daily target for the Bug Smash Initiative.

Local testing has a hiccup so no idea if this works

Looks good to me!

Should we also do the only other missing block plugin config dependency I could find in this issue, rather than creating a new issue?

That only other one is: UserLoginBlock, which depends on user.settings.

The last submitted patch, 20: 2843433-20-TEST-ONLY.patch, failed testing. View results →

Only moving to NW as the full patch in #20 caused some failures it appears.

Trying to fix the tests.

Hopefully test fix.

Added return.

1. --- a/core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php
   +++ b/core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php
   

   The test should assert that the config dependency is set correctly. You could check \Drupal\Tests\system\Kernel\Block\SystemMenuBlockTest::testSystemMenuBlockConfigDependencies for an example.

2. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
   @@ -181,4 +184,11 @@ public function getCacheTags() {
   +    return $this->addDependency('config', 'system.site');
   

   At least according to \Drupal\Component\Plugin\DependentPluginInterface::calculateDependencies this method is supposed to return an array 🤔 \Drupal\Core\Entity\DependencyTrait::addDependency returns this.

Addressed Lauri's feedback from #29 🐛 Branding block should include config dependency on system.site config Needs work

This is ready for another review. Thanks.

Appears all threads have been addressed.

Few more comments on the MR! Getting close! 👍 😊

I have added the update path test, I have to comment out the core/modules/system/src/Plugin/Block/SystemBrandingBlock.php -- the update path test (BlockConfigUpdateTest.php) will fail as there will be 'system.site' config already existing before running the update. Is there any other way to test the update path, otherwise we will have to get rid of this test at the end.

I think the commit is missing the test 😅

Moving to NW for the test failures.

I have explained about the test failures in #35 🐛 Branding block should include config dependency on system.site config Needs work .

#35 + #38: Why doesn't that just mean that the test is inadequate at simulating the actual problem? 🤔

Also, nothing in the issue summary is saying that system.site does not exist at some point. It's saying that updates to system.site are not immediately reflected on the site, which points to at least one missing cache tag. This was discussed at the very start of the issue.

The missing config dependency is just a related bug.

This means the test coverage is incomplete, as is the issue summary.

I agree that the test is inadequate at simulating the actual problem. Is there a way to fix it so that we don't have to comment out newly added calculateDependencies and still pass core/modules/block/tests/src/Functional/BlockConfigUpdateTest.php

Creating a different test only patch for Update path test and remove it from the original test.

This is a test only patch to test

I don't think this is working. Maybe you could start by writing an upgrade path test so that we can validate that the post update we come up with works?

Keeping it assigned to myself because, we still need to test the caching thing by creating two users and checking to see if the name gets updated. It worked perfectly fine when I tried changing the name using Drush command and one user, next thing is to be done is to check through UI and drush with two different users.

I tried on my local, with two different users and changing the name through UI. If I change the name using one user and check it with another user, it is reflecting consistently. So, I cannot reproduce the main problem discussed in IS, other than that the proposed solution is in the MR. Update path test is posted as a test only patch in #42 🐛 Branding block should include config dependency on system.site config Needs work .

Close, but a few nits and an update path that is slightly too eager 😅

Ready for another review.

All threads appear to be resolved.

The issue summary includes both patches and a merge request. There should be only one canonical patch or merge request listed.

Please close any non-canonical merge request(s) and hide non-canonical patches. If you don't have permission to close merge requests, please hide any non-canonical patches and then document which merge request(s) should be closed in an issue comment and the IS. This will allow a committer to close them for you. Thanks!

Hiding patch files, as the MR contains the latest solution.

Removing tests tag because they exist.

Have left a comment on the 2 open threads, can't resolve them but they seem good.

May be super simple but believe this will need a CR actually. Existing contrib themes may ship with a default branding block config that won't include this dependency.

Found several bugs 😅 — see my review on the merge request.

Existing contrib themes may ship with a default branding block config that won't include this dependency.

Good point! That is a reason to implement this in a Block presave hook in system.module. That way, those cases would get updated automatically too.

Tests are only passing because there are no update path tests yet. That would've automatically surfaced pretty much all of the problems I've just pointed out manually.

I recommend starting with that 😊