Incorrect access denied messages in branding block

The patch is not applying to 8.4.x-dev. Looks like it's using the long array syntax and drupal now uses the short syntax.

Re-rolled.

Awesome! I'm attaching screenshots of the new branding elements section of the branding block for reverence:

With permissions to administer theme and configuration:

Without permissions to administer theme and configuration:

1. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
   @@ -75,40 +75,6 @@ public function blockForm($form, FormStateInterface $form_state) {
   -      $site_name_description = $this->t('Defined on the Site Information page. You do not have the appropriate permissions to change the site logo.');
   
   @@ -117,22 +83,41 @@ public function blockForm($form, FormStateInterface $form_state) {
   +      $description = $this->t('Defined on the <a href=":site-settings">Basic site settings</a> page.', [
   

   This is a string change!

2. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
   @@ -117,22 +83,41 @@ public function blockForm($form, FormStateInterface $form_state) {
   +    $block_theme_settings_url = Url::fromRoute('system.theme_settings_theme');
   +    $block_theme_settings_url->setRouteParameter('theme', $theme);
   

   These can be merged: Url::fromRoute() takes route parameters too.

1. Changed "Basic site settings" to "Site Information".
2. Added route parameter to Url::fromRoute().

The last patch applied cleanly on 9.2 these are the changes for a user without permissions.
Before patch

After patch

Moving this back to rtbc, the change here is still a good thing to do and the technical remark by @Wim Leers in #8 has been resolved.

The latest patch here, in #9 does not apply to 9.5.x.

Un-assigning because dpi has not working on this for 5 years.

Rerolled patch #9 for the 9.5.x branch, please review it.

patch #23 was applied successfully in 9.5.x branch. Adding screenshots for reference

Patch looks good just needs tests.

Take that back admin users blocks are now throwing fatal errors. Users with right role get warning. SO patch needs work.

Unassigning given the delay and also since we don't normally assign Drupal core issues to ourselves.

Issue already resolve in version 10. Adding screenshot for reference.

This issue is still reproducible on 10.1 for Olivero / Bartik themes Site branding blocks.
Please see attached screenshot.

Verified patch#27 on 10.1.x-dev. Patch do not apply, may be due test failure. This needs work.

error: while searching for:
    $this->assertEmpty($site_name_element, 'The branding block site name was disabled.');
    $this->assertSession()->elementTextNotContains('xpath', $site_slogan_xpath, 'Community carpentry');
    $this->assertSession()->responseHeaderContains('X-Drupal-Cache-Tags', 'config:system.site');
  }
}
error: patch failed: core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php:119
error: core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php: patch does not apply

This issue also exists in 10.0.2-dev for the default themes Olivero and Claro.
Adding steps to reproduce in the main description.

Rerolled #27
error: patch failed: core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php:119
error: core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php: patch does not apply

tested and verified patch #38 on claro and olivero themes, patch applied successfully and fixed the 'Incorrect access denied messages in branding block', attaching screenshots for reference. need RTBC+1

patch #38 is working fine. also mentioned in #39 so moving status to RTBC.

+++ b/core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php
@@ -115,6 +115,17 @@ public function testSystemBrandingSettings() {
+    $this->assertSession()->responseNotContains('Defined on the Appearance or Theme Settings page. You do not have the appropriate permissions to change the site logo.');
+    $this->assertSession()->responseNotContains('Defined on the Site Information page. You do not have the appropriate permissions to change the site logo.');
+    $this->assertSession()->responseNotContains('Defined on the Site Information page. You do not have the appropriate permissions to change the site logo.');

We never emit 'You do not have the appropriate permissions' anymore, so these assertions would still pass even if the patch didn't work.

Let's change them to assert the new string isn't emitted.

Let's also add some positive asserts for the user that does have the required permissions

added updated patch and addressed #41

Applied the latest patch and converted to a merge request. Fixed the test coverage and found a bug with the active theme and fixed that too.

I have reviewed and tested the patch against Drupal 11.x-dev, and it works as expected. The patch successfully removes the incorrect access denied messages that appeared in the Site Branding block when users lacked permission to edit certain elements. Specifically, users without the appropriate permissions no longer see misleading references such as "You do not have the appropriate permissions to change the site logo" when they only lack access to the site name or slogan.

This is changing the User Interface, so adding tag. I have also updated credit.

A reminder to everyone making screenshots that they should be available from the Issue Summary. Doing so makes it much easier for reviewers and committers.

I manually tested this but there is a bug, the "Theme Settings" link does not always take me to the right place.

This is probably out of scope and requires a new issue, but for users with permission, should we just allow them to configure these settings directly from the block?

Found this issue while working with Canvas, I followed the link to the Appearance settings to change the logo, and I thought there was a bug when it didn't update. Took me a few minutes to figure out that I changed the global logo rather than the one for my default theme.

I think the current state of the MR makes it more confusing by providing two different links (one of which is often to the admin theme rather than the front end theme).

Can we link only to the default front end theme page? What is the purpose of the global logo settings anyway?