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

CommentFileSizeAuthor
#10 8-10-interdiff.txt6.04 KBalexpott
#10 1928082-format-config-book-10.patch6.96 KBalexpott
#10 book_bench.php_.txt1.29 KBalexpott
#8 1928082-format-config-book-8.patch1.06 KBvijaycs85
#7 1928082-format-config-book-6.patch6.73 KBvijaycs85
#7 1928082-diff-5-6.txt666 bytesvijaycs85
#6 1928082-diff-4-5.txt666 bytesvijaycs85
#6 1928082-format-config-book-5.patch762 bytesvijaycs85
#4 1928082-format-config-book-1.patch761 bytesvijaycs85
#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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

YesCT CreditAttribution: YesCT commented

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

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Active » Needs review
FileSize
drupal-book_config_format-1928082-2.patch355 bytes

here is a little start, just for the quotes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Active » Needs review
FileSize
after-settings.yml_.png3.19 KB
after-settings.png20.7 KB
after-form.png19.39 KB
after-tree.png14.76 KB
1928082-format-config-book-1.patch761 bytes

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

YesCT CreditAttribution: YesCT commented
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

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
1928082-format-config-book-5.patch762 bytes
1928082-diff-4-5.txt666 bytes
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
1928082-diff-5-6.txt666 bytes
1928082-format-config-book-6.patch6.73 KB

Adding "for example"

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
1928082-format-config-book-8.patch1.06 KB

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

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Category: task » bug
FileSize
book_bench.php_.txt1.29 KB
1928082-format-config-book-10.patch6.96 KB
8-10-interdiff.txt6.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

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
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

YesCT CreditAttribution: YesCT commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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.