Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Tracker uses 2 variables so it's easy task to convert
Tracker tests affects variable_set('comment_preview_page', 0);
this should be converted within comments settings issue
Comment | File | Size | Author |
---|---|---|---|
#18 | drupal8.config-tracker-settings.18.patch | 1.77 KB | sun |
#16 | 1591412_16_tracker_cmi.patch | 1.75 KB | cosmicdreams |
#13 | 1591412-13.patch | 3.56 KB | swentel |
#1 | 1591412-tracker-settings-1.patch | 3.59 KB | andypost |
Comments
Comment #1
andypostSuppose it's enough
Comment #2
socketwench CreditAttribution: socketwench commentedWhen I ran the test locally, there's an exception on line 444 of schema.inc, "Array to string conversion".
Comment #3
andypost@socketwench try to use latest git checkout
Comment #4
socketwench CreditAttribution: socketwench commentedI just checked out git two hours ago. Is it newer than that? ^_^;;;;
EDIT: Ran git pull, re-applied patch. Running tests now.
Comment #5
socketwench CreditAttribution: socketwench commentedSo yeah, after pulling, applying, and rerunning tests, still the same exceptions.
If we're serializing the fields at #444 of schema.inc, shouldn't we just run $fields[$field] through implode?
EDIT: Wow, nevermind. Not only does this have nothing to do with Tracker, but the above suggestion breaks Drupal spectacularly.
Comment #6
andypost@socketwench could you run tests without patch? Probably core has some fragile in testing
Comment #7
socketwench CreditAttribution: socketwench commentedSure. Cleaned my local copy and pulled the latest Git tonight. Reran the Tracker tests.
Same result as Sunday. Zero fails, two exceptions in schema.inc.
Core noob question: Should we create a new issue for the schema.inc exceptions?
Comment #8
andypostSuppose you could. I've tested some patches from other issues in 2 different servers and has no this trouble with schema
Comment #9
socketwench CreditAttribution: socketwench commentedIt could just be my server. If memory serves I've had it set to report anything that might be considered an error. The patch certainly applies and works.
I'm still new at this, but since the only errors I'm running into are outside of the tracker module, I hope I'm not being presumptive in marking this reviewed and tested. ^_^
Comment #10
marcingy CreditAttribution: marcingy commentedThe update hook does not the load the default configuration into active store.
Comment #11
catchtracker_index_nid is state, not configuration. It will need to be converted to #1175054: Add a storage (API) for persistent non-configuration state when that's available, but definitely not to config.
Comment #12
gddOn top of the above
- This file needs to be converted to YAML per #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML
- The names need to match the new naming conventions laid out at http://drupal.org/node/1667896
Comment #13
swentel CreditAttribution: swentel commentedNew patch with yaml file, we still need to wait for the state patch to go in though.
@heyrocker: is 'batch_size' fine enough or not ? I've read the naming conventions a bit, but I'm not fully clear on it yet :)
Comment #14
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a new patch that adds some extra documentation. Some comments were still referring to old configuration values.
Also, here's some feedback that is trying to make sense of catch's direction from comment #11.
according to catch tracker_index_nid is state not configuration. Why do we persist the value into configuration here?
according to catch we should persist this to config either.
Comment #15
sunAll config system conversions are API changes, so tagging as such.
Comment #16
cosmicdreams CreditAttribution: cosmicdreams commentedI guess I never really uploaded the patch previously. No matter here's a better one.
This one reverts attempts to put tracker's index_nid into the config file. index_nid is state not config. Later, we will handle all the things that are state.
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedGo Go Testbot.
Comment #18
sunI had to read up what tracker_cron() is actually doing to make sense of that setting.
In light of what the setting is actually for, I think that
cron_index_limit
would be a better name?Comment #19
alexpottPatch applies and works as expected and +1 for the switch from
tracker_batch_size
tocron_index_limit
Comment #20
alexpottP.S Core committers don't forget to add the config directory and config file :)
Comment #21
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks. Glad to see more conversions happening.
@alexpott: thanks for the subtle reminder. ;-)