backdrop-contrib / tocbot

Builds a table of contents (TOC) from content headings.
GNU General Public License v2.0
2 stars 2 forks source link

Some settings missing after installation #7

Closed olafgrabienski closed 4 years ago

olafgrabienski commented 5 years ago

After installation, some module and JS settings on the page admin/config/content/tocbot are populated with strings like dynamic value in file /tocbot/tocbot.module line 343. This affects the following settings:

Module settings:

JS settings:

olafgrabienski commented 5 years ago

In tocbot.install I see this related code which was added by Coder upgrade:

/**
 * Implements hook_update_N().
 */
function tocbot_update_1000() {
  $config = config('tocbot.settings');
  $config->set('tocbot_extrabodyclass', update_variable_get('tocbot_extrabodyclass', 'toc-is-active'));
  $config->set('tocbot_tocTitle', update_variable_get('tocbot_tocTitle', 'dynamic value in file /tocbot/tocbot.module line 343'));

(...)

Above, see the last line as an example for a 'dynamic value':

And later on:

/**
 * Implements hook_install().
 */
function tocbot_install() {
  // Dynamically generated variable data was detected.
  // /tocbot/tocbot.module line 343
  // /tocbot/tocbot.module line 346
  // /tocbot/tocbot.module line 347
  // /tocbot/tocbot.module line 348
  // /tocbot/tocbot.module line 349
}
olafgrabienski commented 5 years ago

I'm not sure why the setting is supposed to use a dynamic value. Consequently I'm not sure if it's okay to just change the value to a static one. Example:

Before: $config->set('tocbot_tocTitle', update_variable_get('tocbot_tocTitle', 'dynamic value in file /tocbot/tocbot.module line 343'));

Fix: $config->set('tocbot_tocTitle', update_variable_get('tocbot_tocTitle', 'Table of contents'));

docwilmot commented 5 years ago

@olafgrabienski Coder searches the D7 files for instances of variable_get(A, B) and does a few things with that:

  1. creates a config file with value A: B
  2. creates a hook_install() instance to initialize the config A (not always necessary if you already have the config file of course)
  3. creates a hook_update_N() instance to delete the variable A
  4. then re-writes the string variable_get(A, B) to config_get(config_name, A)

This is easy when A and B are simple variables or strings, but if they are function calls or concatenated strings/variables then step 1 above is impossible: you cant put function calls in a JSON file. For example variable_get('node_type_' . $node_type, module_get_node_type()).

Keep in mind as well that Coder checks all instances of A before writing, because values of B (the default value) may be different in different calls of variable_get(A, B). And if B is dynamic once, Coder writes that "default value" thing.

So the notification is just to let the dev know that Coder found a variable_get(A, B) that it couldnt parse, and that some manual work will be necessary.

And therefore its likely that somewhere you have a variable_get('tocbot_tocTitle', SOMETHING UNPARSEABLE). Probably on line 343 (of the D7 file that is)

olafgrabienski commented 5 years ago

Thanks @docwilmot for the explanation, trying to understand it!

This is line 343 of the D7 file:

$settings['tocTitle'] = check_plain(variable_get('tocbot_tocTitle', 'Page Outline:'));

Hm, actually I'm not sure why 'Page Outline:' isn't parseable.

Apart of that, do you think I can just fill in defaults in tocbot.install manually? For example:

$config->set('tocbot_tocTitle', update_variable_get('tocbot_tocTitle', 'Page Outline:'));

For reference, these are the other lines in question of the D7 file:

olafgrabienski commented 4 years ago

@docwilmot Today I've had another look at the settings in the install file which couldn't be parsed by Coder upgrade and asked a friend what he was thinking about it. He was suggesting that special characters in the respective strings might lead to problems. And indeed, all non parseable strings contain special characters like a colon, a dot, a hash tag, or several commas:

olafgrabienski commented 4 years ago

For the record: In the PR I've added the missing setting values in config/tocbot.settings.json and in tocbot.install. To test the PR, I've uninstalled Tocbot, deleted the active configuration file and reinstalled the module including the updated files.