Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
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.
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.
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 :)
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 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.
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.
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.
+++ 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.
+++ 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
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.
+++ 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.
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.
Comments
Comment #1
LinL CreditAttribution: LinL commentedHere's the patch
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedSmall coding style nit... Can you move the inline comment from the end of the line you touched to a separate line above. Thanks.
Comment #3
alexpottThese 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.
Comment #4
LinL CreditAttribution: LinL commented@alexpott OK, thanks, that makes sense.
Changing title and unassigning myself until I've figured it out.
Comment #5
alexpottFixing tags
Comment #6
LinL CreditAttribution: LinL commentedAnd switching tags back...
Comment #7
Albert Volkman CreditAttribution: Albert Volkman commentedComment #9
Albert Volkman CreditAttribution: Albert Volkman commentedComment #10
alexpottThis 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).
This is not necessary.
Comment #11
Albert Volkman CreditAttribution: Albert Volkman commentedAh, I thought that wasn't necessary. Thanks for the review.
Comment #13
anenkov CreditAttribution: anenkov commentedComment #14
anenkov CreditAttribution: anenkov commentedComment #15
penyaskitoThis comment is unnecessary, it smells like a naming problem. Should it be terms_per_page_count?
We should config()->clear() those variables.
Comment #16
anenkov CreditAttribution: anenkov commentedThis 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.
Comment #17
anenkov CreditAttribution: anenkov commentedUpdated in regards to above review.
Comment #18
penyaskitoLooks good to me and tests passed. RTBCing.
Comment #19
alexpottThis is unnecessary. Config will be cleared automatically on uninstall
Comment #20
penyaskitoTalked 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.
Comment #21
penyaskitoNew patch without the config()->clear().
Comment #23
penyaskitoOops, bad patch
Comment #24
penyaskitoForgot config file :-S
Comment #25
penyaskitoGo bot.
Comment #26
alexpottLooks good thanks for the work!
Comment #27
znerol CreditAttribution: znerol commentedMerge patch from #1831450: Convert taxonomy_maintain_index_table variables to use configuration system into #24.
Comment #28
penyaskitoLGTM if tests pass. I'll leave it as Needs review so alexpott can agree too.
Comment #29
alexpottShould 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 :)
Comment #30
alexpottDuplicated comment :(
Comment #31
alexpottDuplicated comment :(
Comment #32
alexpottDuplicated comment :(
Comment #33
penyaskitoAnd why are tests green? This scares me.
Comment #34
alexpottThis 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.
Comment #35
penyaskitoMakes sense, but should that have tests?
I'll update the patch ASAP.
Comment #36
znerol CreditAttribution: znerol commentedI cannot reproduce this. Datatypes seem to be written and read ok by
Drupal\Core\Config\FileStorage
. See attached (manual) test.Comment #37
znerol CreditAttribution: znerol commentedShould probably have tests for
taxonomy_update_8004
. Miro posted an update on http://drupal.org/node/1667896 covering update tests.Comment #38
alexpottZnerol if you'd written the above test using the config objects and it's set method then the results would be different.
Comment #39
znerol CreditAttribution: znerol commentedOk, 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...
Comment #40
alexpottZnerol - 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.
Comment #41
znerol CreditAttribution: znerol commentedOk, I'm leaving this for @penyaskito, I don't want to interfere with what he's working on.
Comment #42
znerol CreditAttribution: znerol commentedTaking that again, still have some sprint-time.
Comment #43
znerol CreditAttribution: znerol commentedPatch attached. Currently the mapping for taxonomy related variables to config keys is:
The new
terms_per_page_count
is IMO a bit misleading.Comment #44
znerol CreditAttribution: znerol commentedComment #45
znerol CreditAttribution: znerol commentedComment #46
Berdir#43: 1799218-42-taxonomy-variables-to-cmi.patch queued for re-testing.
Comment #47
BerdirDo 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.
This one is actually yet another differrent way that doesn't mention the config filename at all....
Comment #48
znerol CreditAttribution: znerol commentedThanks 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 toterms_per_page_count
which is somewhat misleading.Comment #49
ACF CreditAttribution: ACF commentedhave re-rolled with with a return to terms_per_page_admin
Comment #51
ACF CreditAttribution: ACF commentedsilly mistake.
Comment #52
penyaskitoShould we quote configuration variables in comment or not? I don't know if there is a convention for this, but looks inconsistent.
Newline char missing in the configuration files.
Comment #53
ACF CreditAttribution: ACF commentedre-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.
Comment #55
BerdirUnrelated test failure due to a database deadlock...
Comment #56
Berdir#53: 1799218-taxonomy_variables_to_config-drupal8-53.patch queued for re-testing.
Comment #57
alexpottChange
taxonomy.settings.override_selector
totaxonomy.settings:override_selector
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.
Comment #58
ACF CreditAttribution: ACF commentedfixed the comments.
Comment #59
BerdirTwo tiny coding style issues, then this is IMHO ready.
Now the comment is too long :)
Let's move the comment to a separate line while we're changing this line anyway.
Comment #60
ACF CreditAttribution: ACF commentedHey have changed the first comments to be the correct length. But the trailing comment comes in a a block of a few,
so I've opened up a new issue to deal with coding standards in that file. #1862390: Fix trailing comments from taxonomy.admin.inc..
Comment #61
BerdirThe 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.
Comment #63
ACF CreditAttribution: ACF commented#60: 1799218-taxonomy_variables_to_config-drupal8-60.patch queued for re-testing.
Comment #64
BerdirI guess that was a random failure, still RTBC unless it fails again.
Comment #65
BerdirComment #66
catchCommitted/pushed to 8.x, thanks!