Maintenance mode description shows wrong permission name

Uploading the patch.

Given a try to apply the same patch just to see whether it get pass or not.

Thanks!!

Given a try to apply the same patch just to see whether it get pass or not.

Thanks!!

Patch corrected as per comment #12.

The description should be enclosed in $this->t() for proper translation when used in Classes. I guess this is the reason for failed tests.

Din't add an interdiff since it's just a line of code.

Thank you for providing the patch. This is indeed the right permission label

.

+++ b/core/modules/system/src/Form/SiteMaintenanceModeForm.php
@@ -65,7 +65,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#description' => t('Visitors will only see the maintenance mode message. Only users with the "Access site in maintenance mode" <a href=":permissions-url">permission</a> will be able to access the site. Authorized users can log in directly via the <a href=":user-login">user login</a> page.', array(':permissions-url' => $this->url('user.admin_permissions'), ':user-login' => $this->url('user.login'))),
+      '#description' => $this->t('Visitors will only see the maintenance mode message. Only users with the "Use the site in maintenance mode" <a href=":permissions-url">permission</a> will be able to access the site. Authorized users can log in directly via the <a href=":user-login">user login</a> page.', array(':permissions-url' => $this->url('user.admin_permissions'), ':user-login' => $this->url('user.login'))),

So this is a bit more complex than this. Unfortunately. We shoudl get the translated permission name from the permissions handler so a translator does not have to keep this in sync with whatever ever they've translated the 'access site in maintenance mode' (yep that's the machine name) permission title to be.

@alexpott asked me to look in here. IMHO we do use the pattern of looking up specific translations as well as inlining known strings like this in help text, so IMHO either works.

I'm working on it during the Drupal North code sprint.

Here is my patch. I did what alexpott suggested on #21:

We shoudl get the translated permission name from the permissions handler so a translator does not have to keep this in sync with whatever ever they've translated the 'access site in maintenance mode' (yep that's the machine name) permission title to be.

Please review it and accept it if it is OK.

Thanks @alauzon for your first patch!
I added the DrupalNorth tag and put it in need review to trigger the tests.
Also, it is good practice to add an interdiff when you update an existing patch, that is a file showing only what changed since the previous patch. Here's a link for the doc: https://www.drupal.org/documentation/git/interdiff

Please finf the updated patch.

Done the modifications suggested by Alex #21,
Updated the latest patch of Alauzon #24. by updating the permission label with
$permission_label = $permissions['Use the site in maintenance mode']['title'];.

I believe this is actually the right patch to move this issue forward - it is essentially the patch from #24 so I haven't provided an interdiff.

There was a typo in there:

-      '#default_value' => $this->state->get('system.maintenance_m'),
+      '#default_value' => $this->state->get('system.maintenance_mode'),

In think the tests should pass now.

Applied the patch to 8.2.x and tested. It works!

Does this need any testing?

I have also applied the patch to 8.2.x and tested, its looks good to me and I don't think we need to add a test for this, so I'm changing the status.

Sorry, I changed the version by error.

Well we broke it so adding a test might seem like good idea. This was broken in #620446: Rewrite permission titles and descriptions so these types of bugs are hard to find. We just need an assert that the permission label appears on the page. Considering we're now making the text come from the permission system I think this is worth testing.

Also string changes can only happen in 8.3.0. This is so translations have a chance to keep up.

The test to assert that the permission label appears on the page.

@felribeiro, thanks for adding the test.

At first I thought the message in the assertion, "Found the permission message." was too brief. But after running the tests I see that it makes sense in context. And just one question, since $this->url is deprecated, does this need to be changed now or later?

It's a bit out of scope I think, but since we're changing this line, why not.

I guess that answers my question. Thank you. Looks good to me, so RTBC.

@quietone, @maxocub you are both right - it is out-of-scope. But considering the line is changing seems ok to me.

Committed 73078ed and pushed to 8.3.x. Thanks!