Follow-up from #1653026: [META] Use properly typed values in module configuration.

Problem/Motivation

All integers, Booleans, and even octal numbers in config object files are converted to strings.

Proposed resolution

#1653026: [META] Use properly typed values in module configuration has fixed core, so no need to convert all data types to string anymore.

Remaining tasks

search.routing.yml ('TRUE' to TRUE)
entity.view_mode.node.search_index.yml
entity.view_mode.node.search_result.yml
search.settings.yml
search_embedded_form.settings.yml
search_extra_type.settings.yml

User interface changes

NO

API changes

NO

Parent: #1653026: [META] Use properly typed values in module configuration

CommentFileSizeAuthor
#13 21005983-search-13.patch1.47 KBvijaycs85
#13 21005983-diff-11-13.txt1002 bytesvijaycs85
#14 2105983-13-search.yml_.patch1010 bytesa_thakur
#11 21005983-11-search-yml.patch1.45 KBjhodgdon
#4 21005983-2-search-yml.patch1.37 KBmr.baileys

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Postponed

There is a note at the top of the meta-issue that says:
WARNING: DO NOT work on below sub-tasks until #2106459: Core config has everything as string
@chx got a better option to cover most of the sub tasks as a single issue

So I think this issue needs to be postponed for now.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Postponed » Needs review
Issue tags: +Novice

Since #2106459: Core config has everything as string has landed, I assume this issue is no longer postponed?

Attached is a patch that removes the force-casting to string form search module's configuration files. Note that this excludes search.routing.yml (I assume this is not a configuration file, and removing the quotes there causes a fatal error: A fatal error occurred: Routing requirement for "_search_access" must be a string.)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Make sure all YML files in Search module has no type-casting to string. » Make sure all YML files in Search module have no type-casting to string.
Status: Needs review » Needs work

The patch is missing?

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs work » Needs review
StatusFileSize
new 21005983-2-search-yml.patch1.37 KB

Oops, sorry. Here it is.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

This doesn't look problematic, and I verified it covers all the changes we need in core/modules/search/config.

However, there are a couple of test classes in core/modules/search/test that have config, and are not covered.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

Thanks for the review, jhodgdon!

However, there are a couple of test classes in core/modules/search/test that have config, and are not covered.

The only config file under core/modules/search/tests that contained non-strings cast to string is core/modules/search/tests/modules/search_embedded_form/config/search_embedded_form.settings.yml I think, and it is included in the patch in #4. Are there other config files under core/modules/search I missed?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: -Novice, -YAML

That looks like it. I just saw that one and added the comment without looking to see if there were more. Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review

#4: 21005983-2-search-yml.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Novice, +YAML

I just applied the patch above and confirmed there are no remaining files under core/modules/search that need to be fixed (at least, I think that's it).

I am not sure what that test failure was - random glitch? This patch should be fine.

Adding tags back that I deleted somehow in an earlier comment.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21005983-2-search-yml.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 21005983-11-search-yml.patch1.45 KB

This is a straight reroll. The above patch applied fine with fuzz.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/search/config/entity.view_mode.node.search_index.yml
@@ -1,6 +1,6 @@
+status: 0
+cache: 1

+++ b/core/modules/search/config/entity.view_mode.node.search_result.yml
@@ -1,6 +1,6 @@
+status: 0
+cache: 1

These are booleans so we should use the values true and false here. See entity.view_mode.comment.full.yml

vijaycs85’s picture

vijaycs85
Location London, UK
commented
StatusFileSize
new 21005983-diff-11-13.txt1002 bytes
new 21005983-search-13.patch1.47 KB

updating as per #12. However committing #2096373: Provide config schema for View modes and running @chx script (in #2106459-7: Core config has everything as string) solves this problem globally on all view modes.

a_thakur’s picture

a_thakur commented
Status: Needs work » Needs review
StatusFileSize
new 2105983-13-search.yml_.patch1010 bytes

Updated patch as per comment #12

vijaycs85’s picture

vijaycs85
Location London, UK
commented

Sorry for the cross post @a_thakur, but your patch missing

+++ b/core/modules/search/tests/modules/search_embedded_form/config/search_embedded_form.settings.yml
@@ -1,2 +1,2 @@
-submitted: '12'
+submitted: 12
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Agreed. The patch in ****#13**** -- not #14 -- is good to go. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed c8b0a63 and pushed to 8.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

updating files

Status: Fixed » Closed (fixed)

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

a_thakur’s picture

a_thakur commented
Issue summary: View changes