Convert taxonomy_override_selector and taxonomy_terms_per_page_admin variables to use configuration system

Small coding style nit... Can you move the inline comment from the end of the line you touched to a separate line above. Thanks.

These variables are both configuration as if these were configured on your site then you'd probably like to be able to deploy them from development to production. One way of spotting something that is state is something that is rebuilt on a cache clear and/or would not be shared by development and production versions of the same site.

Fixing tags

This needs to create a taxonomy.settings.yml file and provide the defaults in this file.

Need a taxonomy_update_N to update variable to config using the function (see system.install).

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -10,8 +10,9 @@
+  $config = config('taxonomy');
+  $config->delete('taxonomy_override_selector');
+  $config->delete('taxonomy_terms_per_page_admin');

This is not necessary.

Ah, I thought that wasn't necessary. Thanks for the review.

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -151,7 +151,7 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
+  $page_increment  = config('taxonomy.settings')->get('terms_per_page_admin');   // Number of terms per page.

This comment is unnecessary, it smells like a naming problem. Should it be terms_per_page_count?

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -9,9 +9,6 @@
-  // Remove variables.
-  variable_del('taxonomy_override_selector');
-  variable_del('taxonomy_terms_per_page_admin');

We should config()->clear() those variables.

This is the original comment and the name is almost as the original.
In the above comments is mentioned clearing is not necessary. But I'll include it if it's needed.

Updated in regards to above review.

Looks good to me and tests passed. RTBCing.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -9,9 +9,11 @@
-  // Remove variables.
-  variable_del('taxonomy_override_selector');
-  variable_del('taxonomy_terms_per_page_admin');
+  // Clear settings.
+  config('taxonomy.settings')
+    ->clear('override_selector')
+    ->clear('terms_per_page_count');
+

This is unnecessary. Config will be cleared automatically on uninstall

Talked with @alexpott on IRC and he explained me that configs owned by a module are automatically deleted. I have seen some other modules with clear() on them, but was because they were altering other modules configs. So the clears are unnecessary.

New patch without the config()->clear().

Oops, bad patch

Forgot config file :-S

Go bot.

Looks good thanks for the work!

Merge patch from #1831450: Convert taxonomy_maintain_index_table variables to use configuration system into #24.

LGTM if tests pass. I'll leave it as Needs review so alexpott can agree too.

Should have noticed this before... All config values are strings or arrays at the moment so the yml file values should have single quotes around them and true / false should be '1' / '0'. Sorry.

This might change in the future and there is an issue for that :)

Duplicated comment :(

Duplicated comment :(

Duplicated comment :(

Should have noticed this before... All config values are strings or arrays at the moment so the yml file values should have single quotes around them and true / false should be '1' / '0'. Sorry.

And why are tests green? This scares me.

This is because our yml reader supports typed variables and the yml file and these variables are never written. In PHP '0' will be treated as false unless you use the === operator.

Makes sense, but should that have tests?

I'll update the patch ASAP.

This is because our yml reader supports typed variables and the yml file and these variables are never written. In PHP '0' will be treated as false unless you use the === operator.

I cannot reproduce this. Datatypes seem to be written and read ok by Drupal\Core\Config\FileStorage. See attached (manual) test.

Makes sense, but should that have tests?

Should probably have tests for taxonomy_update_8004. Miro posted an update on http://drupal.org/node/1667896 covering update tests.

Znerol if you'd written the above test using the config objects and it's set method then the results would be different.

Ok, but it occures to me that loading typed data works okay with the config object whilst serialization puts everything into strings. I'm I wrong? Could you point me to the relevant issues, probably could contribute something there...

Znerol - you are correct loading will work.. but writing though the config object will convert everything to strings.

There is an issue to change this. But the issue is with the Form API since this nearly always returns strings. For now we should just make the values in taxonomy.settings.yml strings and be done. If we convert to using typed data this can be changed later.

Ok, I'm leaving this for @penyaskito, I don't want to interfere with what he's working on.

Taking that again, still have some sprint-time.

Patch attached. Currently the mapping for taxonomy related variables to config keys is:

  update_variables_to_config('taxonomy.settings', array(
    'taxonomy_override_selector' => 'override_selector',
    'taxonomy_terms_per_page_admin' => 'terms_per_page_count',
    'taxonomy_maintain_index_table' => 'maintain_index_table',
  ));

The new terms_per_page_count is IMO a bit misleading.

#43: 1799218-42-taxonomy-variables-to-cmi.patch queued for re-testing.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -64,10 +64,10 @@ public function form(array $form, array &$form_state, EntityInterface $term) {
     // taxonomy_get_tree and taxonomy_term_load_parents may contain large numbers of
-    // items so we check for taxonomy_override_selector before loading the
+    // items so we check for taxonomy.settings:override_selector before loading the
     // full vocabulary. Contrib modules can then intercept before

Do we have a standard for referencing configuration in comments? So far, I've always (= maybe two times? ;)) used taxonomy.settings.override_selector. That's how you set it using settings.php, that makes IMHO more sense but doesn't necessarly tell you what the config filename/setting name is.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1388,7 +1388,7 @@ function taxonomy_rdf_mapping() {
- * you should set the variable 'taxonomy_maintain_index_table' to FALSE
+ * you should set the config setting 'maintain_index_table' to false
  * to avoid unnecessary writes in SQL.

This one is actually yet another differrent way that doesn't mention the config filename at all....

Thanks for pointing those out. Before rerolling I'd like to have feedback on another question: In D7 we had taxonomy_terms_per_page_admin indicating, that this limit is effective only on the admin-page. Now this variable is mapped to terms_per_page_count which is somewhat misleading.

have re-rolled with with a return to terms_per_page_admin

silly mistake.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -64,10 +64,10 @@ public function form(array $form, array &$form_state, EntityInterface $term) {
-    // items so we check for taxonomy_override_selector before loading the
+    // items so we check for taxonomy.settings.override_selector before loading the

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1390,7 +1390,7 @@ function taxonomy_rdf_mapping() {
+ * you should set the config setting 'taxonomy.settings.maintain_index_table' to '0'

Should we quote configuration variables in comment or not? I don't know if there is a convention for this, but looks inconsistent.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -349,3 +346,14 @@ function taxonomy_update_8003(&$sandbox) {
\ No newline at end of file

Newline char missing in the configuration files.

re-rolled without the new lines. Completely agree that there should be a convention for how we quote configuration variables, I've changed the second example so that it's easier to use, but it would be great to have more thoughts on this.

Unrelated test failure due to a database deadlock...

#53: 1799218-taxonomy_variables_to_config-drupal8-53.patch queued for re-testing.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -64,10 +64,10 @@ public function form(array $form, array &$form_state, EntityInterface $term) {
+    // items so we check for taxonomy.settings.override_selector before loading the

Change taxonomy.settings.override_selector to taxonomy.settings:override_selector

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1390,7 +1390,7 @@ function taxonomy_rdf_mapping() {
+ * you should set the config('taxonomy.settings)->set('maintain_index_table') to '0'

This line is longer than 80 chars and I would write
you should set taxonomy.settings:maintain_index_table to '0'

Whilst this not a written standard it has been followed in several place eg user.install:537, SystemUpgradePathTest.php:68 and a few other places.

fixed the comments.

Two tiny coding style issues, then this is IMHO ready.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermFormController.phpundefined
@@ -64,10 +64,10 @@ public function form(array $form, array &$form_state, EntityInterface $term) {
     // taxonomy_get_tree and taxonomy_term_load_parents may contain large numbers of
-    // items so we check for taxonomy_override_selector before loading the
+    // items so we check for taxonomy.settings:override_selector before loading the
     // full vocabulary. Contrib modules can then intercept before

Now the comment is too long :)

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -151,7 +151,7 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
   $page            = isset($_GET['page']) ? $_GET['page'] : 0;
-  $page_increment  = variable_get('taxonomy_terms_per_page_admin', 100);  // Number of terms per page.
+  $page_increment  = config('taxonomy.settings')->get('terms_per_page_admin');  // Number of terms per page.

Let's move the comment to a separate line while we're changing this line anyway.

Hey have changed the first comments to be the correct length. But the trailing comment comes in a a block of a few,

$page_increment  = config('taxonomy.settings')->get('terms_per_page_admin');  // Number of terms per page.
  $page_entries    = 0;   // Elements shown on this page.
  $before_entries  = 0;   // Elements at the root level before this page.
  $after_entries   = 0;   // Elements at the root level after this page.
  $root_entries    = 0;   // Elements at the root level on this page.

so I've opened up a new issue to deal with coding standards in that file. #1862390: Fix trailing comments from taxonomy.admin.inc..

The usual practice is to simply fix up lines that you are touched anyway in a patch but a follow-up is fine with me :) This looks good to go.

#60: 1799218-taxonomy_variables_to_config-drupal8-60.patch queued for re-testing.

I guess that was a random failure, still RTBC unless it fails again.

Committed/pushed to 8.x, thanks!