Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The search settings at admin/config/search/settings need to be converted to use the new configuration system.
Tasks
- Create a default config file and add it to the search module.
- Convert the admin UI in search.admin.inc to read to/write from the appropriate config.
- Convert any code that currently accesses the variables used to use the config system.
- Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.
I am tagging this config novice but it is actually a bit more complex than the other novice issues. There are a lot more settings and its more far reaching, but if you're feeling brave go for it!
Comment | File | Size | Author |
---|---|---|---|
#72 | 1496510-72.search-cmi.patch | 25.98 KB | alexpott |
#72 | 65-72-interdiff.txt | 7.93 KB | alexpott |
#65 | interdiff-59-65.txt | 21.5 KB | alexpott |
#65 | 1496510-65.search_cmi.patch | 26.13 KB | alexpott |
#59 | 1496510-59.patch | 25.61 KB | swentel |
Comments
Comment #1
jvns CreditAttribution: jvns commentedComment #2
jvns CreditAttribution: jvns commentedHere's a first stab at this.
Comment #4
marcingy CreditAttribution: marcingy commentedThere is some missing white space
should be (This happens in number of places)
You can remove the extra white space that you have in your xml eg
should be
Also each sub tag should be idented 2 characters
eg
You can't have 2 keys with the same name
Instead we need something like
There are some layout issue in the update hook
Comment #5
jvns CreditAttribution: jvns commentedOkay, fixed the whitespace issues. Haven't done anything about the issues with the upgrade path yet, but let's see what this does.
Comment #6
jvns CreditAttribution: jvns commentedshould actually attach a patch.
Comment #8
jvns CreditAttribution: jvns commentedOops. This one should apply.
Comment #9
marcingy CreditAttribution: marcingy commentedThe latest patch seems to be missing the xml file but all the white space problems look like they are fixed and changes seem good. Leaving for needs to review for the bot but I am assuming the lack of the config file is going to use failures.
Comment #10
jvns CreditAttribution: jvns commentedUploading again to see if it gets picked up by the test bot this time
Comment #11
jvns CreditAttribution: jvns commentedWith the config file
Comment #13
jvns CreditAttribution: jvns commentedFixed up the search tests some.
Comment #15
jvns CreditAttribution: jvns commented#13: search-1496510-13.patch queued for re-testing.
Comment #17
marcingy CreditAttribution: marcingy commentedConverting to new helper update hook.
Comment #18
marcingy CreditAttribution: marcingy commentedComment #20
marcingy CreditAttribution: marcingy commentedHopefully fixes some issues
Comment #22
marcingy CreditAttribution: marcingy commentedComment #23
swentel CreditAttribution: swentel commentedWhitespace
You can reuse $active_modules variable again instead of calling config() again.
Reuse $active_modules instead of calling with $config again.
Whitespace
Whitespace issues
-29 days to next Drupal core point release.
Comment #24
marcingy CreditAttribution: marcingy commentedNew patch fixing comments above and fixing up some tests. Still having an issue with the config folder not being created as part of the upgrade test, which is the main blocker to last test passing as far as I can tell.
Giving to the bot to see where this leaves us.
Comment #26
swentel CreditAttribution: swentel commentedThis will make the tests pass.
It needed a change it config.inc to make sure the simpletest directory existed. The same fix is also used in #1496458: Convert maintenance mode settings to configuration system, so as this or the other is committed (with that fix or another way than the current one), this or the other patch needs to be updated.
Comment #27
marcingy CreditAttribution: marcingy commentedAh that makes sense but shouldn't we be using
instead of direct calls to file_exists and mkdir.
Comment #28
swentel CreditAttribution: swentel commentedAbsolutely, good catch!
Comment #30
swentel CreditAttribution: swentel commentedMeh, that's what you getting trying to manually update a patch :)
Comment #31
aspilicious CreditAttribution: aspilicious commentedLooks good. Tempted to mark this rtbc. But if maintenance gets committed first this needs a reroll.
Comment #32
marcingy CreditAttribution: marcingy commented#1517138: SimpleTest does not create configuration folder causing test failures has now been committed to get round the folder issue this likely needs a re-roll.
Comment #33
swentel CreditAttribution: swentel commentedNew patch
Comment #34
marcingy CreditAttribution: marcingy commentedReroll is just minus the test folder element so happy to RTBC.
Comment #35
catchThis makes me uncomfortable. What happens if default config for search changes during the Drupal 8 lifecycle? Seems like it should explicitly lay out the default configuration as it is now, install that, then it'll be guaranteed to be the same regardless of when any given 8.x site gets upgraded.
Comment #36
gddThe problem if we do that is then when things change, we have to update it in two places instead of just in the config file, and things are pretty likely to be changing at this point (we're already talking about, for instance, changing the file format.) Until such a time as we are supporting head-to-head upgrades, I don't really consider this a problem.
Comment #37
gddOne thing I only just realized about this patch is that it removes the 'search_' prefix from all the variable names. I am not sure how I feel about this. We have not been doing it on the other conversion patches, and I would like to keep it consistent. We can attack changing the variable names in a followup if we want.
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch rerolls the previous patch and includes the edits called for in #37. Also, I applied a few coding standards that have been pointed out to me in the past. Namely use the fluid syntax when possible on store the config object in a variable whenever that variable can be reused.
Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedFound my mistake, new patch.
Comment #42
cosmicdreams CreditAttribution: cosmicdreams commentedFixed the fails by discovering that one of the config gets was using the wrong key name. Also changed the way the incrementing was coded to be absolutely sure that the number of submits is incrementing.
Comment #43
alexpottI think an htaccess diff has crept into the patch.
Also I think we can make a minor code style improvement:
should be
Basically checking if $active_modules is empty is unnecessary... There are two instances of this. To prove the point, there is other code in this patch the makes the assumption the $config->get('search_active_modules') will return an array - such as:
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedThanks Alex, this patch fixes that.
Comment #46
alexpottNow that you do this:
You can't use $active_modules here....
I suggest you make the following change:
Comment #47
gddTwo things need to happen to this patch now:
- It need to be updated to YAML instead of XML
- The naming needs to be adjusted to match the conventions defined at http://drupal.org/node/1667896
Comment #48
aspilicious CreditAttribution: aspilicious commentedComment #49
aspilicious CreditAttribution: aspilicious commentedUnassign not enough time available
Comment #50
swentel CreditAttribution: swentel commentedTaking
Comment #51
swentel CreditAttribution: swentel commentedPatch attached. config file is now in YAML and converted the variable names, let's hope this turns green!
Talked this through first a bit with alexpott on IRC and at first we thought splitting these up in search.settings and search.indexing. However, that turned out to be a little awkwardly and overcomplicated, especially in the admin form.
Comment #52
aspilicious CreditAttribution: aspilicious commentedsearch_embed_form_submitted is kinda verbose. And don't we need a default value? (or is it not needed in this test)
Don't we need to pass $form_state (for config_settings_form) ?
-13 days to next Drupal core point release.
Comment #53
swentel CreditAttribution: swentel commentedNew patch with $form_state variable
The default value of search_embed_form_submitted is set in the test itself, no need to have a default value for that for core itself as it's not used.
Comment #54
cosmicdreams CreditAttribution: cosmicdreams commenteddo we no longer need to modify the search_update_8000 function? I still find the variables we're modifying elsewhere there.
I think I might need to catch up with the documentation on this.
Comment #55
Dries CreditAttribution: Dries commentedThis looks good to me.
I don't see these in the search.settings. Also, these appear to be test related search variables so do we want them there? Third, should we drop the 'search_' prefix?
Missing return before menu_router_rebuild().
Comment #56
swentel CreditAttribution: swentel commentedNew patch. Fixed the return. Dropped the 'search_' prefix as well.
I changed search.settings to search.testing as the embedded variable is indeed only used in testing.
However, really strictly speaking, I think 'embedded_form_submitted' a state, thus not belonging to the config system, so should we wait untill #1175054: Add a storage (API) for persistent non-configuration state gets in ?
Comment #57
sunAll config system conversions are API changes, so tagging as such.
Comment #58
aspilicious CreditAttribution: aspilicious commentedFrom another cmi issue
Comment #59
swentel CreditAttribution: swentel commentedNew patch without update_variables_to_config()
Comment #60
cosmicdreams CreditAttribution: cosmicdreams commentedIs that correct? Since the update_variables_to_config() is in an update hook it seems appropriate.
Comment #61
swentel CreditAttribution: swentel commentedSorry, patch is without the config_install_default_config() function :)
Comment #62
cosmicdreams CreditAttribution: cosmicdreams commentedThat's what I mean. I think that the patch needs the update_variables_to_config function. I've seen that function in other update hooks.
However, your patch went green. so........
Comment #63
sunThanks for working on this! :)
I wonder whether there is some room for improving the keys? It looks like a good chunk of them belongs to indexing, and another chunk to searching.
So how about this?
Would that make sense?
Please also note that all non-strings need to be enclosed in single-quotes. All strings are not quoted.
It looks like this should be an indexed array instead of an associative/hashmap (and actually has been that way before); i.e.,
&$form_state needs to be taken by reference.
For these two, we do not have to check the existing value and can just simply set the new value.
The idea of this API hook example code is to demonstrate a module that integrates with Search module, so the sample config object actually belongs to sample_search.module; i.e.:
The config object name needs to be in search_embedded_form's namespace; i.e.:
Comment #64
swentel CreditAttribution: swentel commentedI'm off for a few days - so if anyone else wants to re-roll with sun's remarks, feel free! If it's not done by wednesday, I'll take it back then!
Comment #65
alexpottMade changes proposed in #63 and did a couple of code tidy ups.
re: search_embedded_form.module and it's config value "submitted"... I think this really is state but in absence of the state system and the fact that this is a test module I've converted it anyway.
Comment #67
alexpott#65: 1496510-65.search_cmi.patch queued for re-testing.
Comment #68
aspilicious CreditAttribution: aspilicious commentedPersonally this looks to verbose
$limit_combinations = config('search.settings')->get('search.and_or_limit');
I understand the reason for grouping stuff in "index" but why group again in search. Everything not grouped is part of "search" by default. No need to create an extra layer.
Comment #69
alexpottLooking at this again I agree with @aspilicious - the yml file should look like this:
Comment #70
cosmicdreams CreditAttribution: cosmicdreams commentedDo I need to reroll this patch with the changes is #69?
Comment #71
sun@cosmicdreams: The answer to such questions is always "yes." :-D
Comment #72
alexpottRerolled patch with changes suggested by @aspilicious
Comment #73
sunAWESOME, friends! Thank you so much for getting this done! :)
Comment #74
webchickRock! So great to have another one of these off our plate! :D
Committed and pushed to 8.x. Thanks!