Duplicate labels for Styles dropdown causes Styles to not render: add validation to prevent this

CKEditor explicitly doesn't allow that:

A unique name must be assigned to your style definition […]

Source: http://docs.cksource.com/CKEditor_3.x/Developers_Guide/Styles

So, that means our validation is insufficient. Nice catch!

Here's a patch that adds the necessary validation. Now the only thing that's still necessary, is test coverage.

Testing this issu

Testing with Orizontal @DrupalNorth

We applied the patch: cke_styles_unique_name_validation-2749475-4.patch and we reproduce the issue. We did received a validation error: Each style must have a unique label. We did change the style label to use unique label. Now we can apply the style to the list. The validation is working. We did verify that the html style are working when inspecting the generated html. Nothing much to do with core gates.

Thanks for testing it, @Orizontal and @bohemier!

Looks like the behavior is as expected, next action would be adding an automated test for ensuring it.

I think this is a Normal bug according to https://www.drupal.org/node/45111, not a Minor.

Thanks for testing & confirming!

This would be a great issue for a new contributor to work on at DrupalDevDays Milan!

Updated the issue summary for clarity.

So what still needs to be done is adding test coverage, at \Drupal\ckeditor\Tests\CKEditorStylesComboAdminTest. You can look at \Drupal\ckeditor\Tests\CKEditorAdminTest for examples — you should be able to reuse significant parts :)

(It needs to be a separate test because this is not testing the overall CKEditor configuration UI, but only one particular plugin's settings form. This then sets the right example for contrib modules.)

Since drupaldevdays is over and this is a bug report for 8.1.x, I thought this should get moved forward. Here are the test-only and the full patch. Hopefully it's on the right track based on #13

Agreed! Thanks :)

1. +++ b/core/modules/ckeditor/src/Tests/CKEditorStylesComboAdminTest.php
   @@ -0,0 +1,151 @@
   +      'format' => 'filtered_html',
   ...
   +      'format' => 'filtered_html',
   

   Let's use $this->randomMachineName().

2. +++ b/core/modules/ckeditor/src/Tests/CKEditorStylesComboAdminTest.php
   @@ -0,0 +1,151 @@
   +      'name' => 'Filtered HTML',
   

   $this->randomString()

3. +++ b/core/modules/ckeditor/src/Tests/CKEditorStylesComboAdminTest.php
   @@ -0,0 +1,151 @@
   +  /**
   +   * Tests StylesCombo settings for an existing text format.
   +   */
   +  function testExistingFormat() {
   ...
   +  /**
   +   * Tests StylesCombo settings for a new text format.
   +   */
   +  function testNewFormat() {
   

   Do we really need both of these? I think just one will do.

4. +++ b/core/modules/ckeditor/src/Tests/CKEditorStylesComboAdminTest.php
   @@ -0,0 +1,151 @@
   +    // Ensure an Editor object exists, with the proper settings.
   

   s/object/config entity/

Thank you for the review Wim, here is the updated patch.

Do we really need both of these? I think just one will do.

+1

Thanks! Found another few nits, but those don't need to block commit.

1. +++ b/core/modules/ckeditor/src/Tests/CKEditorStylesComboAdminTest.php
   @@ -0,0 +1,99 @@
   +  protected function setUp() {
   

   Missing {@inheritdoc}.

2. +++ b/core/modules/ckeditor/src/Tests/CKEditorStylesComboAdminTest.php
   @@ -0,0 +1,99 @@
   +    // Create text format.
   ...
   +      'weight' => 0,
   ...
   +    // Create admin user.
   

   These can be removed IMO.

Unrelated test failure back to RTBC

And here are the nits trying to make the life easier to our beloved committers!

Random fails in upgrade path tests. Retesting. Back to RTBC.

Committed and pushed f2d19d2d2bee3229fd0730af4fb95ace4d5de3ad to 8.2.x and a6b370b to 8.1.x. Thanks!