Posted by YesCT on February 26, 2013 at 4:07pm
8 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | book.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Configuration schema, Configuration system, D8MI, language-config |
Issue Summary
Problem/Motivation
The format in the book.settings.yml file initially is:
allowed_types:
- book
block:
navigation:
mode: all pages
child_type: bookAfter 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: bookThe 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
- clean out a possible previous site: sudo rm -r sites; git checkout sites;
- install site
- open in an editor: core/modules/book/config/book.settings.yml
- enable book module under Extend
- save the book settings form at admin/content/book/settings
- 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
Comments
#1
look for/near:
config('book.settings') ->set('allowed_types', $form_state['values']['book_allowed_types'])#2
here is a little start, just for the quotes.
#3
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!
#4
Fixed and added patch. Attached screenshot after this change on schema inspector.
#5
+++ 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?
#6
#7
Adding "for example"
#8
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.
#9
#10
Patch makes the book.settings:allowed_types consistent... as a result it includes:
book_node_type_update()book_type_is_allowed()book_node_type_update()Drupal\system\Tests\Upgrade\SystemUpgradePathTestto 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.txtwhich 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.
#11
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 :)
#12
Looks great, and even comes with upgrade tests! Lovely. :)
Committed and pushed to 8.x. Thanks!
#13
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()toin_array().#14
I wonder if there is already an issue to tackle that. If not, we should open one to address the root cause.
#15
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.
#16
Automatically closed -- issue fixed for 2 weeks with no activity.