Files: 
CommentFileSizeAuthor
#60 1799218-taxonomy_variables_to_config-drupal8-60.patch9.11 KBACF
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es).
[ View ]
#58 1799218-taxonomy_variables_to_config-drupal8-58.patch8.88 KBACF
FAILED: [[SimpleTest]]: [MySQL] 49,217 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#53 1799218-taxonomy_variables_to_config-drupal8-53.patch8.85 KBACF
PASSED: [[SimpleTest]]: [MySQL] 49,274 pass(es).
[ View ]
#51 1799218-taxonomy_variables_to_config-drupal8-51.patch8.9 KBACF
PASSED: [[SimpleTest]]: [MySQL] 49,297 pass(es).
[ View ]
#49 1799218-taxonomy_variables_to_config-drupal8-49.patch8.9 KBACF
FAILED: [[SimpleTest]]: [MySQL] 49,105 pass(es), 30 fail(s), and 20 exception(s).
[ View ]
#43 1799218-42-taxonomy-variables-to-cmi.patch8.64 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 47,992 pass(es).
[ View ]
#36 file-storage-test.php_.txt616 bytesznerol
#36 file-storage-test.out_.txt515 bytesznerol
#27 taxonomy_admin-1799218-27.patch7.15 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 47,581 pass(es).
[ View ]
#24 taxonomy_admin-1799218-23.patch3.16 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 46,860 pass(es).
[ View ]
#23 taxonomy_admin-1799218-21.patch2.86 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 46,570 pass(es), 21 fail(s), and 37 exception(s).
[ View ]
#21 taxonomy_admin-1799218-17.patch3.16 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy_admin-1799218-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 taxonomy_admin-1799218-13.patch3.28 KBanenkov
PASSED: [[SimpleTest]]: [MySQL] 46,845 pass(es).
[ View ]
#14 taxonomy_admin-1799218-12.patch3.19 KBanenkov
PASSED: [[SimpleTest]]: [MySQL] 46,434 pass(es).
[ View ]
#13 taxonomy_admin-1799218-12.patch3.19 KBanenkov
FAILED: [[SimpleTest]]: [MySQL] 46,431 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 taxonomy_admin-1799218-11.patch3.06 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] 46,039 pass(es), 9 fail(s), and 23 exception(s).
[ View ]
#11 interdiff.txt2.73 KBAlbert Volkman
#9 taxonomy_admin-1799218-9.patch2.36 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]
#9 interdiff.txt705 bytesAlbert Volkman
#7 taxonomy_admin-1799218-7.patch2.33 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] 42,821 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#7 interdiff.txt2.34 KBAlbert Volkman
#1 taxonomy_admin-1799218-1.patch2.31 KBLinL
FAILED: [[SimpleTest]]: [MySQL] 41,898 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

LinL’s picture

Status:Active» Needs review
StatusFileSize
new2.31 KB
FAILED: [[SimpleTest]]: [MySQL] 41,898 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here's the patch

Lars Toomre’s picture

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

alexpott’s picture

Status:Needs review» Needs work
Issue tags:-State system+Configuration system

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.

LinL’s picture

Title:Convert taxonomy_override_selector and taxonomy_terms_per_page_admin variables to use state system» Convert taxonomy_override_selector and taxonomy_terms_per_page_admin variables to use configuration system
Assigned:LinL» Unassigned
Issue tags:-Configuration system+State system

@alexpott OK, thanks, that makes sense.

Changing title and unassigning myself until I've figured it out.

alexpott’s picture

Fixing tags

LinL’s picture

And switching tags back...

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new2.34 KB
new2.33 KB
FAILED: [[SimpleTest]]: [MySQL] 42,821 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, taxonomy_admin-1799218-7.patch, failed testing.

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new705 bytes
new2.36 KB
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]
alexpott’s picture

Status:Needs review» Needs work

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.

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new2.73 KB
new3.06 KB
FAILED: [[SimpleTest]]: [MySQL] 46,039 pass(es), 9 fail(s), and 23 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, taxonomy_admin-1799218-11.patch, failed testing.

anenkov’s picture

StatusFileSize
new3.19 KB
FAILED: [[SimpleTest]]: [MySQL] 46,431 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
anenkov’s picture

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
PASSED: [[SimpleTest]]: [MySQL] 46,434 pass(es).
[ View ]
penyaskito’s picture

Status:Needs review» Needs work
+++ 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.

anenkov’s picture

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.

anenkov’s picture

Status:Needs work» Needs review
StatusFileSize
new3.28 KB
PASSED: [[SimpleTest]]: [MySQL] 46,845 pass(es).
[ View ]

Updated in regards to above review.

penyaskito’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me and tests passed. RTBCing.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

penyaskito’s picture

Assigned:Unassigned» penyaskito

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.

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new3.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy_admin-1799218-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, taxonomy_admin-1799218-17.patch, failed testing.

penyaskito’s picture

StatusFileSize
new2.86 KB
FAILED: [[SimpleTest]]: [MySQL] 46,570 pass(es), 21 fail(s), and 37 exception(s).
[ View ]

Oops, bad patch

penyaskito’s picture

StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 46,860 pass(es).
[ View ]

Forgot config file :-S

penyaskito’s picture

Status:Needs work» Needs review

Go bot.

alexpott’s picture

Status:Needs review» Reviewed & tested by the community

Looks good thanks for the work!

znerol’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new7.15 KB
PASSED: [[SimpleTest]]: [MySQL] 47,581 pass(es).
[ View ]
penyaskito’s picture

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

alexpott’s picture

Status:Needs review» Needs work

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 :)

alexpott’s picture

Duplicated comment :(

alexpott’s picture

Duplicated comment :(

alexpott’s picture

Duplicated comment :(

penyaskito’s picture

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.

alexpott’s picture

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.

penyaskito’s picture

Makes sense, but should that have tests?

I'll update the patch ASAP.

znerol’s picture

StatusFileSize
new515 bytes
new616 bytes

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.

znerol’s picture

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.

alexpott’s picture

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

znerol’s picture

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...

alexpott’s picture

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.

znerol’s picture

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

znerol’s picture

Assigned:penyaskito» znerol

Taking that again, still have some sprint-time.

znerol’s picture

StatusFileSize
new8.64 KB
PASSED: [[SimpleTest]]: [MySQL] 47,992 pass(es).
[ View ]

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

<?php
  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.

znerol’s picture

Status:Needs work» Needs review
znerol’s picture

Assigned:znerol» Unassigned
Berdir’s picture

Berdir’s picture

+++ 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....

znerol’s picture

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.

ACF’s picture

StatusFileSize
new8.9 KB
FAILED: [[SimpleTest]]: [MySQL] 49,105 pass(es), 30 fail(s), and 20 exception(s).
[ View ]

have re-rolled with with a return to terms_per_page_admin

Status:Needs review» Needs work

The last submitted patch, 1799218-taxonomy_variables_to_config-drupal8-49.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new8.9 KB
PASSED: [[SimpleTest]]: [MySQL] 49,297 pass(es).
[ View ]

silly mistake.

penyaskito’s picture

Status:Needs review» Needs work
+++ 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.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new8.85 KB
PASSED: [[SimpleTest]]: [MySQL] 49,274 pass(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1799218-taxonomy_variables_to_config-drupal8-53.patch, failed testing.

Berdir’s picture

Unrelated test failure due to a database deadlock...

Berdir’s picture

Status:Needs work» Needs review
alexpott’s picture

Status:Needs review» Needs work
+++ 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.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new8.88 KB
FAILED: [[SimpleTest]]: [MySQL] 49,217 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

fixed the comments.

Berdir’s picture

Status:Needs review» Needs work

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.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new9.11 KB
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es).
[ View ]

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..

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Configuration system

The last submitted patch, 1799218-taxonomy_variables_to_config-drupal8-60.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system
Berdir’s picture

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

Berdir’s picture

Status:Needs review» Reviewed & tested by the community
catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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