Convert book settings to the new configuration system

The book settings at :

admin/content/book/settings
admin/structure/block/manage/book/navigation/configure aka hook_block_configure

Tasks

Create a default config file and add it to the module.
Convert the admin UI in core/modules/book/book.admin.inc core/modules/book/book.module to read to/write from the appropriate config.
Convert any book code that currently accesses the variables used to use the config system.
Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.
This would be a good task for someone wanting to get up to speed on how the new config system works. That would be me, chriscalip.

Files: 
CommentFileSizeAuthor
#25 interdiff.txt370 bytesno_commit_credit
#24 book-1703164-24.patch7.34 KBno_commit_credit
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View
#22 1703164-22.patch7.34 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 39,844 pass(es). View
#22 interdiff.txt470 byteskbasarab
#20 drupal8.config-book-settings.20.patch7.33 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 39,807 pass(es). View
#17 drupal8.config-book-settings.17.patch7.91 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,809 pass(es). View
#15 drupal8.config-book-settings.15.patch7.15 KBchriscalip
FAILED: [[SimpleTest]]: [MySQL] 39,750 pass(es), 3 fail(s), and 0 exception(s). View
#15 Screenshot-book-settings.png142.91 KBchriscalip
#12 drupal8.config-book-settings.12.patch7.15 KBchriscalip
PASSED: [[SimpleTest]]: [MySQL] 39,754 pass(es). View
#9 drupal8.config-book-settings.9.patch7.15 KBchriscalip
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View
#7 drupal8.config-book-settings.7.patch7.18 KBchriscalip
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View
#4 drupal8.config-book-settings.4.patch6.86 KBchriscalip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#1 drupal8.config-book-settings.1.patch6.89 KBchriscalip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Comments

chriscalip’s picture

FileSize
6.89 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

first try.

chriscalip’s picture

Status: Active » Needs review
cosmicdreams’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/book.installundefined
@@ -93,3 +86,16 @@ function book_schema() {
\ No newline at end of file

Need to add a newline here.

Great work!

chriscalip’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

a.) per #3 drupal coding standards compliance code change, adding new line.
b.) variable name changes; updated mapping name to correct one from 'all_pages' to intended 'block_mode'

Status: Needs review » Needs work

The last submitted patch, drupal8.config-book-settings.4.patch, failed testing.

cosmicdreams’s picture

Chris, two things

1. you need a config directory with your yml file in the patch
2. you should look at the book tests and see if there are things you need to change in there for handling these variables. A find and replace on the variable_get and variable_sets in there.

chriscalip’s picture

Status: Needs work » Needs review
FileSize
7.18 KB
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View

Chris,
Yeah my git diff patch didnt take into account the added new file.
adding now..

cosmicdreams’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/config/book.settings.ymlundefined
@@ -0,0 +1,4 @@
\ No newline at end of file

No newline

chriscalip’s picture

Status: Needs work » Needs review
FileSize
7.15 KB
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es). View

adding line.

chriscalip’s picture

Assigned: chriscalip » Unassigned
aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/config/book.settings.ymlundefined
@@ -0,0 +1,6 @@
+

one newline to much

Besides of that this looks good

-29 days to next Drupal core point release.

chriscalip’s picture

FileSize
7.15 KB
PASSED: [[SimpleTest]]: [MySQL] 39,754 pass(es). View

removed a line too far.

chriscalip’s picture

Status: Needs work » Needs review
aspilicious’s picture

+++ b/core/modules/book/config/book.settings.ymlundefined
@@ -0,0 +1,5 @@
+

Looks like there still is a line to much... damnit, srry for that. It just must end with a newline so if you remove all the trailing lines and press enter after "all pages" it should be fine

-29 days to next Drupal core point release.

chriscalip’s picture

FileSize
142.91 KB
7.15 KB
FAILED: [[SimpleTest]]: [MySQL] 39,750 pass(es), 3 fail(s), and 0 exception(s). View

no worries, i'm just not used to yml on my ide.. spacing dyslexia

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/book.admin.inc
@@ -32,12 +32,13 @@ function book_admin_overview() {
+function book_admin_settings($form, $form_state) {

&$form_state always needs to be taken by reference. :)

+++ b/core/modules/book/book.module
@@ -284,7 +284,7 @@ function book_block_view($delta = '') {
+  if (config('book.settings')->get('block_mode') == 'all pages') {

I think I'd expect to find this in the key block.navigation.mode

aspilicious’s picture

Status: Needs work » Needs review
FileSize
7.91 KB
PASSED: [[SimpleTest]]: [MySQL] 39,809 pass(es). View

Lets see...

alexpott’s picture

Status: Needs review » Needs work

Really, really minor nitpick...

+++ b/core/modules/book/book.admin.incundefined
@@ -66,6 +68,18 @@ function book_admin_settings_validate($form, &$form_state) {
 /**
+ * Form submission handler for book_admin_settings().
+ *
+ * @see book_admin_settings()
+ */
+function book_admin_settings_submit($form, &$form_state) {
+  config('book.settings')
+    ->set('allowed_types', $form_state['values']['book_allowed_types'])
+    ->set('child_type', $form_state['values']['book_child_type'])
+    ->save();

@see book_admin_settings() is superflous here

aspilicious’s picture

actually it has to point to the validate function: http://drupal.org/node/1354#forms

kbasarab’s picture

Status: Needs work » Needs review
FileSize
7.33 KB
PASSED: [[SimpleTest]]: [MySQL] 39,807 pass(es). View

Here's the change from @see book_admin_settings() to @see book_admin_settings_vaidate()

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/book.installundefined
@@ -93,3 +86,16 @@ function book_schema() {
+function book_update_8000() {
+  update_variables_to_config('book.settings', array(
+    'book_allowed_types' => 'allowed_types',
+    'book_child_type' => 'child_type',
+    'book_block_mode' => 'block_mode',
+  ));

Need to update 'block_mode' to the new config key 'block.navigation.mode'

kbasarab’s picture

Status: Needs work » Needs review
FileSize
470 bytes
7.34 KB
PASSED: [[SimpleTest]]: [MySQL] 39,844 pass(es). View

Updated.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to go! Thanks for all the work!

no_commit_credit’s picture

FileSize
7.34 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View

Corrected update hook docblock to use the imperative as per our standard:
http://drupal.org/node/1354#hookimpl

no_commit_credit’s picture

FileSize
370 bytes

Keep forgetting to attach interdiffs. :)

And, the name of this account is no_commit_credit. Which means, I did not actually contribute to this patch, so please don't mention the account in the commit message.

Dries’s picture

Committed to 8.x.

sun’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

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

Anonymous’s picture

Issue summary: View changes

added another related link.