Issue #1933548 by alexpott: Fixed Book allowed_types() settings repetitive and in under certain conditions can change unexpectedly.

core/modules/book/book.install

@@ -99,18 +99,12 @@ function book_update_8000() {
   ));
   $allowed_types = update_variable_get('book_allowed_types', FALSE);
   if ($allowed_types) {
-    // In Drupal 7 the variable book_allowed_types was stored like this:
-    // array(
-    //   0 => 'book',
-    //   1 => 'article',
-    // )
-    // In Drupal 8 book.settings:allowed_types should be stored like this:
-    // array(
-    //   'book' => 'book',
-    //   'article' => 'article',
-    // )
+    // Ensure consistent ordering of allowed_types.
+    // @see book_admin_settings_submit()
+    sort($allowed_types);
+
     config('book.settings')
-      ->set('allowed_types', drupal_map_assoc($allowed_types))
+      ->set('allowed_types', $allowed_types)
       ->save();
   }
 

core/modules/book/book.module

@@ -1230,27 +1230,28 @@ function template_preprocess_book_node_export_html(&$variables) {
  *   A Boolean TRUE if the node type can be included in books; otherwise, FALSE.
  */
 function book_type_is_allowed($type) {
-  $allowed_types = config('book.settings')->get('allowed_types');
-  return isset($allowed_types[$type]);
+  return in_array($type, config('book.settings')->get('allowed_types'));
 }
 
 /**
  * Implements hook_node_type_update().
  *
- * Updates the Book module's persistent variables if the machine-readable name
- * of a node type is changed.
+ * Updates book.settings configuration object if the machine-readable name of a
+ * node type is changed.
  */
 function book_node_type_update($type) {
   if (!empty($type->old_type) && $type->old_type != $type->type) {
     $config = config('book.settings');
     // Update the list of node types that are allowed to be added to books.
     $allowed_types = $config->get('allowed_types');
-
-    if (isset($allowed_types[$type->old_type])) {
+    $old_key = array_search($type->old_type, $allowed_types);
+    if ($old_key !== FALSE) {
       // Replace the old machine-readable name with the new machine-readable
       // name.
-      $allowed_types[$type->type] = $type->type;
-      unset($allowed_types[$type->old_type]);
+      $allowed_types[$old_key] = $type->type;
+      // Ensure that the allowed_types array is sorted consistently.
+      // @see book_admin_settings_submit()
+      sort($allowed_types);
       $config->set('allowed_types', $allowed_types);
     }
 

core/modules/book/config/book.settings.yml

@@ -1,5 +1,5 @@
 allowed_types:
-  book: book
+  - book
 block:
   navigation:
     mode: 'all pages'

core/modules/book/lib/Drupal/book/BookSettingsForm.php

@@ -63,9 +63,14 @@ class BookSettingsForm extends SystemConfigFormBase {
    * Implements \Drupal\Core\Form\FormInterface::submitForm().
    */
   public function submitForm(array &$form, array &$form_state) {
+    $allowed_types = array_filter($form_state['values']['book_allowed_types']);
+    // We need to save the allowed types in an array ordered by machine_name so
+    // that we can save them in the correct order if node type changes.
+    // @see book_node_type_update().
+    sort($allowed_types);
     $this->configFactory->get('book.settings')
-      // Remove unchecked types.
-      ->set('allowed_types', array_filter($form_state['values']['book_allowed_types']))
+    // Remove unchecked types.
+      ->set('allowed_types', $allowed_types)
       ->set('child_type', $form_state['values']['book_child_type'])
       ->save();
 

core/modules/book/lib/Drupal/book/Tests/BookTest.php

@@ -67,7 +67,7 @@ class BookTest extends WebTestBase {
     // Create users.
     $this->book_author = $this->drupalCreateUser(array('create new books', 'create book content', 'edit own book content', 'add content to books'));
     $this->web_user = $this->drupalCreateUser(array('access printer-friendly version', 'node test view'));
-    $this->admin_user = $this->drupalCreateUser(array('create new books', 'create book content', 'edit own book content', 'add content to books', 'administer blocks', 'administer permissions', 'administer book outlines', 'node test view', 'administer content types'));
+    $this->admin_user = $this->drupalCreateUser(array('create new books', 'create book content', 'edit own book content', 'add content to books', 'administer blocks', 'administer permissions', 'administer book outlines', 'node test view', 'administer content types', 'administer site configuration'));
   }
 
   /**
@@ -405,6 +405,81 @@ class BookTest extends WebTestBase {
     // the new machine and the old one has been removed.
     $this->assertTrue(book_type_is_allowed('bar'), 'Config book.settings:allowed_types contains the updated node type machine name "bar".');
     $this->assertFalse(book_type_is_allowed('book'), 'Config book.settings:allowed_types does not contain the old node type machine name "book".');
+
+    $edit = array(
+      'name' => 'Basic page',
+      'title_label' => 'Title for basic page',
+      'type' => 'page',
+    );
+    $this->drupalPost('admin/structure/types/add', $edit, t('Save content type'));
+
+    // Add page to the allowed node types.
+    $edit = array(
+      'book_allowed_types[page]' => 'page',
+      'book_allowed_types[bar]' => 'bar',
+    );
+
+    $this->drupalPost('admin/content/book/settings', $edit, t('Save configuration'));
+    $this->assertTrue(book_type_is_allowed('bar'), 'Config book.settings:allowed_types contains the bar node type.');
+    $this->assertTrue(book_type_is_allowed('page'), 'Config book.settings:allowed_types contains the page node type.');
+
+    // Test the order of the book.settings::allowed_types configuration is as
+    // expected. The point of this test is to prove that after changing a node
+    // type going to admin/content/book/settings and pressing save without
+    // changing anything should not alter the book.settings configuration. The
+    // order will be:
+    // @code
+    // array(
+    //   'bar',
+    //   'page',
+    // );
+    // @endcode
+    $current_config = config('book.settings')->init()->get();
+    $this->drupalPost('admin/content/book/settings', array(), t('Save configuration'));
+    $this->assertIdentical($current_config, config('book.settings')->init()->get());
+
+    // Change the name, machine name and description.
+    $edit = array(
+      'name' => 'Zebra book',
+      'type' => 'zebra',
+    );
+    $this->drupalPost('admin/structure/types/manage/bar', $edit, t('Save content type'));
+    $this->assertTrue(book_type_is_allowed('zebra'), 'Config book.settings:allowed_types contains the zebra node type.');
+    $this->assertTrue(book_type_is_allowed('page'), 'Config book.settings:allowed_types contains the page node type.');
+
+    // Test the order of the book.settings::allowed_types configuration is as
+    // expected. The order should be:
+    // @code
+    // array(
+    //   'page',
+    //   'zebra',
+    // );
+    // @endcode
+    $current_config = config('book.settings')->init()->get();
+    $this->drupalPost('admin/content/book/settings', array(), t('Save configuration'));
+    $this->assertIdentical($current_config, config('book.settings')->init()->get());
+
+    $edit = array(
+      'name' => 'Animal book',
+      'type' => 'zebra',
+    );
+    $this->drupalPost('admin/structure/types/manage/zebra', $edit, t('Save content type'));
+
+    // Test the order of the book.settings::allowed_types configuration is as
+    // expected. The order should be:
+    // @code
+    // array(
+    //   'page',
+    //   'zebra',
+    // );
+    // @endcode
+    $current_config = config('book.settings')->init()->get();
+    $this->drupalPost('admin/content/book/settings', array(), t('Save configuration'));
+    $this->assertIdentical($current_config, config('book.settings')->init()->get());
+
+    // Ensure that after all the node type changes book.settings:child_type has
+    // the expected value.
+    $this->assertEqual(config('book.settings')->get('child_type'), 'zebra');
   }
 
   /**

core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php

@@ -147,9 +147,9 @@ class SystemUpgradePathTest extends UpgradePathTestBase {
 
     $expected_config['book.settings'] = array(
       'allowed_types' => array(
-        'book' => 'book',
+        'book',
         // Content type does not have to exist.
-        'test' => 'test',
+        'test',
       ),
       'block' => array(
         'navigation' => array(

core/modules/system/tests/upgrade/drupal-7.system.database.php

@@ -150,7 +150,7 @@ db_update('variable')
   ->condition('name', 'filter_fallback_format')
   ->execute();
 db_update('variable')
-  ->fields(array('value' => 'a:2:{i:0;s:4:"book";i:1;s:4:"test";}'))
+  ->fields(array('value' => 'a:2:{i:0;s:4:"test";i:1;s:4:"book";}'))
   ->condition('name', 'book_allowed_types')
   ->execute();