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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chriscalip’s picture

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

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

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

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

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

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

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

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

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

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.

andypost’s picture

+++ b/core/modules/book/book.admin.inc
@@ -45,14 +47,14 @@ function book_admin_settings() {
   $form['array_filter'] = array('#type' => 'value', '#value' => TRUE);

@@ -66,6 +68,18 @@ function book_admin_settings_validate($form, &$form_state) {
+    ->set('allowed_types', $form_state['values']['book_allowed_types'])
+    ->set('child_type', $form_state['values']['book_child_type'])

This was wrong conversion, followup is #315176: Clean-up remains of $form['array_filter'] hack with array_filter in book module