Increase site slogan maxlength to 255 to allow longer slogans

+++ b/core/modules/system/src/Tests/System/PageTitleTest.php
@@ -142,4 +142,12 @@ public function testRoutingTitle() {
 
+  /**
+   * Tests that the site slogan has the correct maxlength.
+   */
+  public function testSloganSize() {
+    $this->drupalGet('admin/config/system/site-information');
+    $this->assertPattern('/<input .* id="edit-site-slogan" .* maxlength="255" .* \/>/', 'Site slogan field has correct maxlength in site settings form.');
+  }
+

Wouldn't be the better test whether you can actually enter 255 characters and save it?

Attached proposed patch in #4 + changes for #7.

Cheers.

Thank you @waspper!

+++ b/core/modules/system/src/Form/SiteInformationForm.php
--- a/core/modules/system/src/Tests/System/PageTitleTest.php
+++ b/core/modules/system/src/Tests/System/PageTitleTest.php

+++ b/core/modules/system/src/Tests/System/PageTitleTest.php
+++ b/core/modules/system/src/Tests/System/PageTitleTest.php
@@ -142,4 +142,16 @@ public function testRoutingTitle() {

Are we sure we want to add this to PageTitleTest?

This test isn't about page titles. The rest of the test looks unrelated to our issue here.

testTitleXSS in that same class is about the same title. I don't see a better place to do this in.
We could create a followup for this issue to create a new test for that method + the new test added here.

It would be easy to create a new test class, wouldn't it?

It would be easy to create a new test class, wouldn't it?

I agree, that's easy, but I don't think we want to move testTitleXSS to that new test at the same time in this issue as well, that sounds like scope creep. I think that's better suited to a new (novice) followup.

@larowlan: do you have an opinion about #17?

The patch no longer applies since the affected test class was moved in #2862508: Convert system functional tests to phpunit

Slogan Text increased to 255 Characters

Thanks for helping out. We actually need the test changes so I’m setting this to “needs work”.

Slogan Text increased to 255 Characters and added test

Slogan Text increased to 255 Characters and added test

Fixed Coding Standards Issues

Fixed Coding Standards Issues

This patch needs a review

@Neslee Canil Pinto, thanks for the patch.

I reviewed the patch and found a few things. If you are like me the out of scope changes creep in when I accidentally reformat the code for the entire file and not the block I have changed.

1. +++ b/core/modules/system/tests/src/Functional/System/PageTitleTest.php
   @@ -34,7 +34,13 @@ protected function setUp() {
   -    $this->contentUser = $this->drupalCreateUser(['create page content', 'access content', 'administer themes', 'administer site configuration', 'link to any page']);
   +    $this->contentUser = $this->drupalCreateUser([
   +      'create page content',
   +      'access content',
   +      'administer themes',
   +      'administer site configuration',
   +      'link to any page',
   +    ]);
   
   @@ -61,7 +67,7 @@ public function testTitleTags() {
   -  public function testTitleXSS() {
   +  public function testTitleXss() {
   
   @@ -106,7 +112,7 @@ public function testRoutingTitle() {
   -    // Test forms
   +    // Test forms.
   

   I've been pinged lately on out of scope changes so since there are not related to issue let's remove them. And we get a smaller easier to read patch.

2. +++ b/core/modules/system/tests/src/Functional/System/PageTitleTest.php
   @@ -155,4 +161,17 @@ public function testRoutingTitle() {
   +      'site_slogan'  => $this->randomMachineName(255),
   

   Looks like an extra space before '=>'.

And the testbot reports a coding standards error.

Hi @quietone
Updated patch as you suggested please review.

1. I added test coverage of \Drupal\system\Form\SiteInformationForm.

2. I added a negative test.

+    $placeholders = [
+      '%maxlength' => $slogan_maxlength,
+      '%out_of_size' => $slogan_maxlength + 1,
+    ];
+    $this->assertSession()->responseContains(t('Slogan cannot be longer than %maxlength characters but is currently %out_of_size characters long.', $placeholders));

3. +1 for Increase length of slogan to 255. It looks enough. If someone needs more, they can still alter the form.

128 characters
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer non justo quis ligula varius cursus quis quis neque. Nunc orci.

255 characters
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin sed orci commodo, congue ipsum eu, finibus dolor. Sed molestie leo nec luctus mattis. Nulla id ex ac nunc sollicitudin vehicula nec eget odio. Vivamus lacus neque, varius ut magna vel viverra.

+++ b/core/modules/system/tests/src/Functional/System/SiteInformationFormTest.php
@@ -0,0 +1,49 @@
+    $this->drupalPostForm('admin/config/system/site-information', $edit, t('Save configuration'));
...
+    $this->drupalPostForm('admin/config/system/site-information', $edit, t('Save configuration'));
...
+    $this->assertSession()->responseContains(t('Slogan cannot be longer than %maxlength characters but is currently %out_of_size characters long.', $placeholders));

We don't need to call t() in tests, we have ongoing issues elsewhere to remove these calls.

Hi @longwave
Made changes as you suggested please review.

@Deepak Goyal thanks for working on it!

The second output will be
Slogan cannot be longer than <em class="placeholder">255</em> characters but is currently <em class="placeholder">256</em> characters long.
We need to add html tags and we need the sprintf() function that formats the string.

$expected_error = sprintf('Slogan cannot be longer than <em class="placeholder">%d</em> characters but is currently <em class="placeholder">%d</em> characters long.', $slogan_maxlength, $slogan_maxlength + 1);
$this->assertSession()->responseContains($expected_error);

Hi @Krzysztof Domański
Made changes as you suggested please review.

We no longer need the following code.

+    $placeholders = [
+      '%maxlength' => $slogan_maxlength,
+      '%out_of_size' => $slogan_maxlength + 1,
+    ];

Hi @Krzysztof Domański
Removed above code please review.

+++ b/core/modules/system/tests/src/Functional/System/SiteInformationFormTest.php
@@ -0,0 +1,46 @@
+    $expected_error = sprintf('Slogan cannot be longer than <em class="placeholder">%d</em> characters but is currently <em class="placeholder">%d</em> characters long.', $slogan_maxlength, $slogan_maxlength + 1);
+    $this->assertSession()->responseContains($expected_error);

Instead of checking the explicit HTML output here should we use pageTextContains()?

Unrelated random test failure. Back to RTBC.

+++ b/core/modules/system/tests/src/Functional/System/SiteInformationFormTest.php
@@ -0,0 +1,46 @@
+
+  /**
+   * Tests that the site slogan has the correct maxlength.
+   */
+  public function testSloganSize() {
+    $slogan_maxlength = 255;
+
+    $edit = [
+      'site_slogan' => $this->randomMachineName($slogan_maxlength),
+    ];
+    $this->drupalPostForm('admin/config/system/site-information', $edit, 'Save configuration');
+    $this->assertSession()->pageTextContains('The configuration options have been saved.');
+
+    $edit = [
+      'site_slogan' => $this->randomMachineName($slogan_maxlength + 1),
+    ];
+    $this->drupalPostForm('admin/config/system/site-information', $edit, 'Save configuration');
+    $expected_error = sprintf('Slogan cannot be longer than %d characters but is currently %d characters long.', $slogan_maxlength, $slogan_maxlength + 1);
+    $this->assertSession()->pageTextContains($expected_error);
+  }
+

This is a case where I don't think we should add test coverage - our configuration forms are generic and have generic test coverage.

This is a case where I don't think we should add test coverage - our configuration forms are generic and have generic test coverage.

+1 for this approach. A similar situation in the past #2277281-50: Sometimes you want more than 128 chars of allowed file extensions.

Well, that certainly makes the patch simpler :)

Before Applying the patch I was able to site slogan with max length = 128 characters
After applying patch slogan max length = 255 character
This looks good
Moving to RTBC

Committed 2115909 and pushed to 9.1.x. Thanks!

Seems like the patch does not account for the translation of the slogan, when I want to translate the slogan in a different language it's still limited to 128 chars for the other language.

Added #3176880: Increase site slogan maxlength for translations.