Problem/Motivation

The format in the book.settings.yml file initially is:

allowed_types:
  - book
block:
  navigation:
    mode: all pages
child_type: book

After saving the form, looking at the file in sites/default/files/hash... it is:

allowed_types:
  book: book
  article: '0'
  page: '0'
block:
  navigation:
    mode: 'all pages'
child_type: book

The format of saving the "sequence" is different.
Before it uses - content_type
and after: content_type_allowed: content_type_allowed
or: content_type_not_allowed: 0

Also... the mode string is missing quotes initially.

Proposed resolution

Add quotes around the string that has a space in book.settings.yml

Fix the book config set to only save allowed types, and to match the format of the sequence with the -

Remaining tasks

discuss proposed resolution
implement

User interface changes

No UI changes.

API changes

No API changes.

Steps to reproduce

  1. clean out a possible previous site: sudo rm -r sites; git checkout sites;
  2. install site
  3. open in an editor: core/modules/book/config/book.settings.yml
  4. enable book module under Extend
  5. save the book settings form at admin/content/book/settings
  6. open in an editor the yml file saved under the config hash, for example: sites/default/files/config_NrobAyuHYCIoR5CIio_mXfe6zna5HJ1qO3tsEp1eJ5g/active/book.settings.yml

Original report by @YesCT

Follow up for #1912302-11: Create configuration schemas for book module

Files: 
CommentFileSizeAuthor
#10 8-10-interdiff.txt6.04 KBalexpott
#10 1928082-format-config-book-10.patch6.96 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es). View
#10 book_bench.php_.txt1.29 KBalexpott
#8 1928082-format-config-book-8.patch1.06 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,239 pass(es). View
#7 1928082-format-config-book-6.patch6.73 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1928082-format-config-book-6.patch. Unable to apply patch. See the log in the details link for more information. View
#7 1928082-diff-5-6.txt666 bytesvijaycs85
#6 1928082-diff-4-5.txt666 bytesvijaycs85
#6 1928082-format-config-book-5.patch762 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es). View
#4 1928082-format-config-book-1.patch761 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,370 pass(es). View
#4 after-tree.png14.76 KBvijaycs85
#4 after-form.png19.39 KBvijaycs85
#4 after-settings.png20.7 KBvijaycs85
#4 after-settings.yml_.png3.19 KBvijaycs85
#2 drupal-book_config_format-1928082-2.patch355 bytesYesCT
PASSED: [[SimpleTest]]: [MySQL] 52,356 pass(es). View

Comments

YesCT’s picture

look for/near: config('book.settings') ->set('allowed_types', $form_state['values']['book_allowed_types'])

YesCT’s picture

Status: Active » Needs review
FileSize
355 bytes
PASSED: [[SimpleTest]]: [MySQL] 52,356 pass(es). View

here is a little start, just for the quotes.

alexpott’s picture

Status: Needs review » Active

Looks like the default config should look like this:

allowed_types:
  book: book
block:
  navigation:
    mode: 'all pages'
child_type: book

And use array_filter() here...

config('book.settings')->set('allowed_types', array_filter($form_state['values']['book_allowed_types']))

Nice catch!

vijaycs85’s picture

Status: Active » Needs review
FileSize
3.19 KB
20.7 KB
19.39 KB
14.76 KB
761 bytes
PASSED: [[SimpleTest]]: [MySQL] 52,370 pass(es). View

Fixed and added patch. Attached screenshot after this change on schema inspector.

after-tree.png

after-form.png

after-settings.png

after-settings.yml_.png

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/book.admin.incundefined
@@ -92,8 +92,10 @@ function book_admin_settings_validate($form, &$form_state) {
+  // Save allowed types as indexed values (sample, 0 => book).

can we make this (for example, ...) instead of sample?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
762 bytes
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es). View
666 bytes
vijaycs85’s picture

FileSize
666 bytes
6.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1928082-format-config-book-6.patch. Unable to apply patch. See the log in the details link for more information. View

Adding "for example"

vijaycs85’s picture

FileSize
1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 52,239 pass(es). View

Updating book.settings.yml to have book: book format for allowed_types. We got to do this because:

1. book_node_type_update() using key to get the type.
2. book_type_is_allowed() checking in_array() to get the type.

To avoid any calculation (array_key() or flip etc), keeping this simple "type: type" format.

vijaycs85’s picture

Title: make consistant format of config set with configuration schema sequence format for book module settings » Make usage of book.settings:allowed_types consistent
alexpott’s picture

Category: task » bug
FileSize
1.29 KB
6.96 KB
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es). View
6.04 KB

Patch makes the book.settings:allowed_types consistent... as a result it includes:

  • Fixed update of d7 to d8 (this actually makes this a bug!)
  • Saner code in book_node_type_update()
  • More performant code in book_type_is_allowed()
  • A new test to ensure we don't break functionality in book_node_type_update()
  • Adds to Drupal\system\Tests\Upgrade\SystemUpgradePathTest to ensure variable book_allowed_types is migrated to the expected format.

Performance testing done with book_bench.php.txt (attached) and drush using the command:

drush scr book_bench.php.txt

which outputs:

Calling book_type_is_allowed_old with 1 allowed types takes 2.7218658924103 seconds.
Calling book_type_is_allowed_new with 1 allowed types takes 2.6360340118408 seconds.
Calling book_type_is_allowed_old with 10 allowed types takes 3.0037131309509 seconds.
Calling book_type_is_allowed_new with 10 allowed types takes 2.7561480998993 seconds.
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Great changes! I think it is of paramount importance to CMI that the config structure will not change when there are new content types added to the site (but the config itself does not change). The fixed approach to storing the types makes this possible. Now if the config diff shows a change it is due to real changes regarding this configuration. The extra performance improvements are also nice :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great, and even comes with upgrade tests! Lovely. :)

Committed and pushed to 8.x. Thanks!

sun’s picture

Status: Fixed » Active

Sorry for chiming in late. I wasn't aware of this issue.

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

 allowed_types:
-  - book
+  book: book

Hm. I'm not happy with this repetition of values. Configuration is supposed to be in a simple, declarative format.

In all config conversions thus far, we've used simple lists/sequences to denote a configured set of enabled option values.

We want to save the selected options with array_values(array_filter($form_state['values']['yada']))

AFAIK, that simple list format is suitable as #options value when building the settings form.

What probably needs to be changed are other API-level accesses, most likely from isset() to in_array().

YesCT’s picture

I wonder if there is already an issue to tackle that. If not, we should open one to address the root cause.

alexpott’s picture

Status: Active » Fixed

Opened #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly to address #13 as in trying to solve #13 I discover another closely related issue.

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