In #1928082: Make usage of book.settings:allowed_types consistent (http://drupal.org/node/1928082#comment-7128904) @sun points out that the duplicate key/value in book.settings:allowed_types kind of sucks. That patch made the following change.

allowed_types:
-  - book
+  book: book

In trying to resolve this issue I realised we actually have a much more important issue. book_node_type_update() keeps book.settings in sync with node type changes. At the moment if you have two content types with machine names book and page as allowed types and then you change the book machine name to album the book_node_type_update() function will update book.settings:allowed_types to:

  page: page
  album: album

If you were then to go to admin/content/book/settings and press the save button without making any changes to the form book.settings:allowed_types will be updated to:

  album: album
  page: page

This means you will end up with unexpected config changes which is a bad thing(tm).

Files: 
CommentFileSizeAuthor
#15 10-15-interdiff.txt2.4 KBalexpott
#15 1933548.book-settings.15.patch9.94 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,124 pass(es).
[ View ]
#10 9-10-interdiff.txt640 bytesalexpott
#10 1933548.book-settings.10.patch9.84 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,579 pass(es).
[ View ]
#6 5-6-interdiff.txt24.4 KBalexpott
#6 1933548.book-settings.6.patch35.34 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,382 pass(es).
[ View ]
#9 5-9-interdiff.txt420 bytesalexpott
#9 1933548.book-settings.9.patch9.75 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,599 pass(es).
[ View ]
#5 1-5-interdiff.txt3.69 KBalexpott
#5 1933548.book-settings.5.patch9.75 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,338 pass(es).
[ View ]
#1 1933548.book-settings.1.patch7.19 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,283 pass(es).
[ View ]

Comments

alexpott’s picture

StatusFileSize
new7.19 KB
PASSED: [[SimpleTest]]: [MySQL] 52,283 pass(es).
[ View ]

The patch attached fixes this by changing allowed_types to a numerically keyed array eg.

  - book
  - page

I've added lots of tests to fully test the book_node_type_update() function.

alexpott’s picture

Status:Active» Needs review
sun’s picture

Status:Needs review» Needs work
+++ b/core/modules/book/book.admin.inc
@@ -92,9 +92,14 @@ function book_admin_settings_validate($form, &$form_state) {
function book_admin_settings_submit($form, &$form_state) {
+  $allowed_types = array_filter($form_state['values']['book_allowed_types']);
...
     // Remove unchecked types.
-    ->set('allowed_types', array_filter($form_state['values']['book_allowed_types']))
+    ->set('allowed_types', $allowed_types)

Stale comment, should be moved up above the new line where unchecked types are removed now.

+++ b/core/modules/book/book.admin.inc
@@ -92,9 +92,14 @@ function book_admin_settings_validate($form, &$form_state) {
+  // 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);

+++ b/core/modules/book/book.module
@@ -1247,12 +1246,13 @@ function book_node_type_update($type) {
+      // Ensure that the allowed type array is sorted by node type machine name.
+      sort($allowed_types);

The corresponding added test explains this some more, but we should actually clarify why this is done in the functional code, too. Actually, a proper explanation in the functional code is much more important.

In general:

Instead of explaining what the technical code lines are doing à la:

"sort and save values so they appear in the 'correct' order."

Always document the actual whats and whys:

"Ensure that the existing order of enabled book types remains to be identical, (because... | in order to ...)"

Alas, what I'm currently missing is the "why?" — why are we doing this? :)

alexpott’s picture

The why is because: when you change node type's machine name, book.settings.yml will change. Maybe you import this configuration into another site. Whatever... you do some action that means are an expected fixed point in terms of the state of your configuration eg. check it into git.

Then someone goes to admin/content/book/settings changes nothings and presses "Save configuration" and hey presto you configuration has changed because the order of the allowed_types array has changed.

Which is wrong.

Every time we save the book.settings:allowed_types config key we need to ensure that the ordering is consistent that means we need to sort it. We have two places in the code where we save it at the moment and they are inconsistent when they should be.

At the moment the admin form saves in the order that's returned by node_type_get_names() which is in the order of the human readable value.

In book_node_type_update() we only have the machine names in this array and sorting here by human readable value only leads to more complexity.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new9.75 KB
PASSED: [[SimpleTest]]: [MySQL] 52,338 pass(es).
[ View ]
new3.69 KB

Here's a patch that addresses - @sun thanks for the review!

Realised that I'd forgotten about the upgrade path too...

alexpott’s picture

StatusFileSize
new35.34 KB
PASSED: [[SimpleTest]]: [MySQL] 52,382 pass(es).
[ View ]
new24.4 KB

Minor code style fix

EDIT: ignore rebase issue... intended patch is in #9

YesCT’s picture

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.phpundefined
@@ -9,31 +9,29 @@
- * A configuration context object provides a data array that can be used as
- * parameters to get customized configuration objects.
+ * A configuration context object provides a data array that can be used:
+ *   - as a parameter to get customized configuration objects.
+ *   - as a store of config data used to override values.

+++ b/core/modules/user/lib/Drupal/user/UserConfigContext.phpundefined
@@ -26,6 +26,14 @@ class UserConfigContext extends ConfigContext {
+  /**
+   * Implements \Drupal\Core\Config\Context\ContextInterface::setUuid().
+   */
+  public function setUuid() {
+    // Use the user's uuid to identify the config context.
+    $this->uuid = $this->get(self::USER_KEY)->uuid();
+  }
+

is this unrelated changes? it's reminding me of #1929136: Fix override-free context, move global config overrides back to an event listener

+++ b/core/lib/Drupal/Core/Config/Context/GlobalConfigContext.phpundefined
@@ -0,0 +1,29 @@
+  }
+}

I think a newline at the end of the class is usual.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverride.phpundefined
@@ -36,7 +36,7 @@ public function setUp() {
-  /**
+  /*
    * Tests basic locale override.

@@ -44,22 +44,19 @@ function testConfigLocaleOverride() {
-  /**
+  /*
    * Tests locale override based on user's preferred language.

@@ -126,92 +118,30 @@ function testConfigLocaleUserOverride() {
-  /**
+  /*

unintended change I think.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.phpundefined
@@ -101,15 +116,24 @@ function testConfOverride() {
+    // Write file to staging

missing period to end the sentence.

This was just a quick review. I didn't read it really carefully.

Note, this will need a re-roll when the follow-up for #1763640: Introduce config context to make original config and different overrides accessible gets in.

alexpott’s picture

:( patch mess... rerolling...

alexpott’s picture

StatusFileSize
new9.75 KB
PASSED: [[SimpleTest]]: [MySQL] 52,599 pass(es).
[ View ]
new420 bytes

Hmmn forgot to rebase... doh!

alexpott’s picture

StatusFileSize
new9.84 KB
PASSED: [[SimpleTest]]: [MySQL] 52,579 pass(es).
[ View ]
new640 bytes

On talking with @sun an improvement in code comments...

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks! :)

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Sorry, this is not applying for me...

$ git apply --index 1933548.book-settings.10.patch
error: patch failed: core/modules/book/lib/Drupal/book/Tests/BookTest.php:405
error: core/modules/book/lib/Drupal/book/Tests/BookTest.php: patch does not apply
YesCT’s picture

Issue tags:+Needs reroll

adding tag for reroll (also a help document on rerolls: http://drupal.org/patch/reroll)

webchick’s picture

Thanks! I was trying to remember that tag.

alexpott’s picture

StatusFileSize
new9.94 KB
PASSED: [[SimpleTest]]: [MySQL] 53,124 pass(es).
[ View ]
new2.4 KB

Rerolled due to changes introduced by #1925196: Convert book's system_config_form() to SystemConfigFormBase

When this is green we should be back to rtbc.

alexpott’s picture

Status:Needs work» Needs review
sun’s picture

Status:Needs review» Reviewed & tested by the community

In the hope it comes back green.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Tidying up the english...