This is a child of #1775842: [meta] Convert all variables to state and/or config systems

The following calls to variable_get are made in API documentation. These should be removed or replaced with the Drupal 8 equivalent.

core/modules/node/node.api.php
375 $restricted = variable_get('example_restricted_roles', array());

core/modules/system/system.api.php
943 '#default_value' => variable_get('upload_' . $form['type']['#value'], 1),
1932 if (strpos(file_uri_target($uri), variable_get('user_picture_path', 'pictures') . '/picture-') === 0) {
2593 * variable_get() if you are collecting any data that you need to store and
2655 $myprofile_needs_batch_processing = variable_get('myprofile_needs_batch_processing', FALSE);

All calls to variable_set should also be replaced.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

Status: Active » Needs review
FileSize
6.53 KB

Here are my changes. This doesn't fix the user_picture_path variable, as that looks a bit more complicated (I'll probably open a new issue for it).

ianthomas_uk’s picture

+++ b/core/modules/system/system.api.php
@@ -3235,12 +3238,10 @@ function hook_filetransfer_info() {
-  if (variable_get('paranoia', FALSE)) {	
- // Remove the FTP option entirely.
- unset($filetransfer_info['ftp']);
- // Make sure the SSH option is listed first.
- $filetransfer_info['ssh']['weight'] = -10;
- }

I removed this variable completely as it is not referred to anywhere else and the if statement didn't seem to serve a useful purpose (with this sort of code, if you didn't want it to run you'd probably just delete the code from your custom module or uninstall the contrib module).

ianthomas_uk’s picture

Status: Needs review » Needs work

Ternary operators should be removed

alexpott’s picture

  1. +++ b/core/modules/system/system.api.php
    @@ -1787,7 +1787,9 @@ function hook_module_preinstall($module) {
    -    variable_set('lousy_module_conflicting_variable', FALSE);
    +    \Drupal::config('example.settings')
    +      ->set('lousy_module_conflicting_variable', FALSE)
    +      ->save();
    

    Lets change this to state() rather then config because the whole namespacing system in CMI is designed to avoid this :)

  2. +++ b/core/modules/system/system.api.php
    @@ -2589,11 +2591,11 @@ function hook_uninstall() {
    + * an installation page multiple times, so you should use \Drupal::state() or
    + * \Drupal::config() if you are collecting any data that you need to store and
    

    This is very tricky. Storing something in CMI means that you want to be able to deploy the configuration. Typically that is not information that is collected during installation. For example, the profile used to be stored the variable system but is now written to settings.php which is actually very sensible considering what modules are available to enable depend on this. I think at least it is worth mentioning that config() should only be used to store values that are changeable by the user after installation. If this not the case then it's not config.

  3. +++ b/core/modules/system/system.api.php
    @@ -2696,13 +2698,14 @@ function hook_install_tasks(&$install_state) {
    +    // call \Drupal::state()->delete('myprofile_needs_batch_processing') and
    +    // clean up the variable that was used above. If you want the user to pass
    

    You are no longer cleaning up a variable :)

ianthomas_uk’s picture

RE #4.2
I mentioned config to try and avoid people thinking they couldn't use config, so putting things in state, only to put them into config later on. I'll try and clarify that.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
8.71 KB

Here's a new patch with quite a few changes.

- No ternary operators used to provide config defaults
- State keys now start 'mymodule.' or 'myprofile.'
- State defaults provided via the second parameter of get()
- lousy_module variable renamed and moved to state
- hook_modules_uninstalled rewritten to mirror hook_modules_installed
- hook_install/hook_uninstall replaced with example from image.module, as this seemed to be a good demonstration of setting and unsetting something that is not managed by Drupal itself
- Installation guidance rewritten, removing reference to config.

TODO
- I'm not sure how to avoid the dynamic config on line 943 of system.api.php

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.api.php
@@ -940,7 +940,7 @@ function hook_form_alter(&$form, &$form_state, $form_id) {
-      '#default_value' => variable_get('upload_' . $form['type']['#value'], 1),
+      '#default_value' => \Drupal::config('mymodule.settings')->get('upload.' . $form['type']['#value']),

I'm trying to remember why / if a dynamic config key name like this is an issue. Perhaps a better way of doing this like this...

  if (isset($form['type']) && $form['type']['#value'] . '_node_settings' == $form_id) {
    $upload_enabled_types = \Drupal::config('mymodule.settings')->get('upload_enabled_types');
    $form['workflow']['upload_' . $form['type']['#value']] = array(
      '#type' => 'radios',
      '#title' => t('Attachments'),
      '#default_value' => in_array($form['type']['#value'], $upload_enabled_types),
      '#options' => array(t('Disabled'), t('Enabled')),
    );
    // Add a custom submit handler to save the array of types back to the config file.
    $form['actions']['submit']['#submit'][] = 'mymodule_upload_enabled_types_submit';
  }
+++ b/core/modules/system/system.api.php
@@ -2308,17 +2306,10 @@ function hook_query_TAG_alter(Drupal\Core\Database\Query\AlterableInterface $que
 function hook_install() {
-  // Populate the default {node_access} record.
-  db_insert('node_access')
-    ->fields(array(
-      'nid' => 0,
-      'gid' => 0,
-      'realm' => 'all',
-      'grant_view' => 1,
-      'grant_update' => 0,
-      'grant_delete' => 0,
-    ))
-    ->execute();
+  // Create the styles directory and ensure it's writable.
+  $directory = file_default_scheme() . '://styles';
+  $mode = isset($GLOBALS['install_state']['mode']) ? $GLOBALS['install_state']['mode'] : NULL;
+  file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS, $mode);
 }

@@ -2545,7 +2536,8 @@ function hook_update_last_removed() {
 function hook_uninstall() {
-  variable_del('upload_file_types');
+  // Remove the styles directory and generated images.
+  file_unmanaged_delete_recursive(file_default_scheme() . '://styles');
 }

Nice! Although this appears out of scope actually this is making the documentation consistent.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
13.17 KB

Here's a new patch.

I was unable to make an interdiff (technical difficulties). sorry. Things I changed:
- Used Alex's suggestion (more or less) from #7.
- Found a few more places where variable_* was being used in API comments, and removed them too.

jhodgdon’s picture

As a note:

After this patch... I grepped and the only place with variable_* in a core/modules/*.api.php file is in menu.api.php; this is covered by a separate issue:
#2108679: API documentation: Convert my_module_menus examples in menu.api.php to CMI

I also grepped through the whole codebase for variable_* in a line containing a * (i.e., in a doc block) and this patch got all the varable_*() calls.

Status: Needs review » Needs work

The last submitted patch, 2109065-update-api-docs-8.patch, failed testing.

jhodgdon’s picture

#10 looks like a bot glitch. The tests passed. ?!?

mtift’s picture

Yes, the tests passed. Someone re-ran the tests on the backed.

cosmicdreams’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a6efca1 and pushed to 8.x. Thanks!

alexpott’s picture

Issue summary: View changes

Updated issue summary.

Status: Fixed » Closed (fixed)

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