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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
3.59 KB

Suppose it's enough

socketwench’s picture

When I ran the test locally, there's an exception on line 444 of schema.inc, "Array to string conversion".

andypost’s picture

@socketwench try to use latest git checkout

socketwench’s picture

I just checked out git two hours ago. Is it newer than that? ^_^;;;;

EDIT: Ran git pull, re-applied patch. Running tests now.

socketwench’s picture

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

andypost’s picture

@socketwench could you run tests without patch? Probably core has some fragile in testing

socketwench’s picture

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

andypost’s picture

Suppose you could. I've tested some patches from other issues in 2 different servers and has no this trouble with schema

socketwench’s picture

Status: Needs review » Reviewed & tested by the community

It 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. ^_^

marcingy’s picture

Status: Reviewed & tested by the community » Needs work

The update hook does not the load the default configuration into active store.

catch’s picture

variable_del('tracker_index_nid');

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

gdd’s picture

On 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

swentel’s picture

FileSize
3.56 KB

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

cosmicdreams’s picture

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

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerTest.phpundefined
@@ -214,7 +214,7 @@ class TrackerTest extends WebTestBase {
+    config('tracker.settings')->set('index_nid', 3)->save();

according to catch tracker_index_nid is state not configuration. Why do we persist the value into configuration here?

+++ b/core/modules/tracker/tracker.installundefined
@@ -6,20 +6,12 @@
+    config('tracker.settings')->set('index_nid', $max_nid)->save();
     // To avoid timing out while attempting to do a complete indexing, we

according to catch we should persist this to config either.

sun’s picture

Issue tags: +API change

All config system conversions are API changes, so tagging as such.

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review

Go Go Testbot.

sun’s picture

I 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?

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies and works as expected and +1 for the switch from tracker_batch_size to cron_index_limit

alexpott’s picture

P.S Core committers don't forget to add the config directory and config file :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks. Glad to see more conversions happening.

@alexpott: thanks for the subtle reminder. ;-)

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