OK, everyone, please help me think. I tried to explain it to my duck, but it wasn't in the mood.
In the Index
class, we have a method resetCaches()
to reset all the caches pertaining to the index (in particular cached fields lists, properties, plugins, etc.). It has a parameter $include_stored
which, when set to FALSE
, will skip resetting the permanent, stored cache and just reset the cache properties on the index object itself.
However, here comes my question: if you do that, doesn't that just mean it that next time getFields()
is called (which uses both a static and stored cache) the stale data from the stored cache will be loaded into the static property and returned? Seems to me, this doesn't make any sense.
Please tell me if I'm missing something there.
Otherwise, to save performance, we should either use something like cache tags, to avoid clearing all caches when just one part has changed, and maybe also have some property during index save that disables the stored cache for a while (to avoid dozens of cache sets and deletes with no gain whatsoever). Or, in general, maybe try to be a lot more thorough in resetting caches exactly when necessary, neither more often nor rarer – just like Core did, which brought great benefits, too.
In any case, we shouldn't allow clearing one cache, but leaving the other with the old, stale data.
Estimated Value and Story Points
Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.
Value: 3
Story Points: 5
Comment | File | Size | Author |
---|---|---|---|
#134 | 2638116-134--clean_up_index_caching.patch | 127.24 KB | drunken monkey |
Comments
Comment #2
borisson_We probably shouldn't need this method and having correct cache tags, keys and contexts should be sufficient. So in theory, we should be able to remove this method when we fix all other cacheability problems.
Comment #3
drunken monkeyNot really, no, since I'm pretty sure we'll still want static caching for most methods. Loading the fields anew from stored cache for each
getFields()
call would be insane. Same goes for all the plugins, which currently aren't even stored in permanent cache (which we probably don't want to change).Also, the normal cache system, with its cache tags etc., can only work on index save. The problem we're having here is that, while the index is being edited, the
getFields()
method should always retrieve the fields based on the current settings on the index object, but the stored cache should not be rebuilt (since the changes might not be saved). So while we definitely want to use the stored cache for the firstgetFields()
call of a page request, it mustn't be used once the index has been modified, until it has been saved and the cache thus been cleared.We probably need a broader concept for the whole thing, but it can wait until Beta (when all of the basics are in place so that we really know the scenarios under which this would have to work).
Comment #4
borisson_That makes sense.
Comment #5
drunken monkeyActually, I just ran into major trouble due to this over in #2640982: Fix "unsaved changes" code in the new Fields UI, so we might want to tackle this earlier.
On the other hand, that other issue is also only a UI change, so not really a beta blocker.
Comment #6
drunken monkeyNow that #2317771: Move processor and/or field settings to top-level properties on the index? is in, I think we can un-postpone this, and the way I have it currently in mind I think we need to make this a beta blocker after all, since it will change the internal API somewhat.
My current plan for this:
Index::[gs]etFieldSettings()
, and also don't change$this->fields
directly anywhere inside the index class.preSave()
, we write the array of fields objects back to thefields
property.cache
property after all – that would just send the wrong message. Instead, maybefieldObjects
? The name is a bit tricky, I admit.$this->fields
on the first call ofgetFields()
with the field objects and only make the translation back intoArray()
. But probably really not worth it just to avoid the naming problem.)addField()
will therefore only be used for adding a new field, not for re-saving the settings of an existing one. For changing an existing fields, just use the field's methods for changing its properties and you're done. (Except for changing the field ID or removing the field.)getFields()
will be that tricky anymore, so even having a stored cache might not really make sense anymore. It does add some additional complexity. However, in this new system, we'd never have to reset the array of field objects (since it's not really a "cache" anymore), so the stored cache would only be used when first loading the fields from the stored settings anyways.getFieldsByDatasource()
andgetFulltextFields()
can still use normal static caching, I think. Resetting their caches will only be necessary in the index's own(add|rename|remove)Field()
methods, I think. In theory, changing a field's type could invalidate the static cache ofgetFulltextFields()
, but I'd say that's too much of an edge case to take it into account. (I don't think that method is ever used when changing the index's fields.)resetCaches()
should be dropped in favor of something more fine-grained, at least for this use case. (Or for all, if we do the following, too.)Also,
Index::$properties
isn't even used anymore.Comment #7
borisson_I have opinions:
$this->fields
anywhere in the code.We probably have to keep the code to add / remove a field to the fields array in
AddField
/removeField
though.addField
method when trying to save a field that already exists?getFieldsByDatasource()
andgetFulltextFields()
.We should probably keep
resetCaches
but addresetFieldCaches
,resetProcessorCaches
, ...I think we should reset the cache of
getFulltextFields
on changing a field type as well, it's probably not complex to include that check (if $old_field->type != $field->type) { $this->cache[getFulltextFields] = NULL; }) and on the edge-case that this changes, it will save a ton of WTF'sI agree, let's make things more consistent; that's a good goal.
We should be making changes for:
- fields
- tracker
- datasource
- processors
I think I'd prefer to have 2 properties for all of those: things + thingInstances
- fields + fieldInstances
- tracker + trackerInstance
- server + serverInstance
- datasources + datasourceInstances
- processors + processorInstances
I also created #2642284: Remove the index's "properties" property for the other remark you made, that's a very novice issue.
Comment #8
borisson_As an addition to my previous comment:
The following is an example of how I think we might be able to change things, this is close to how fields work now and I think we should be doing this for all of the mentioned plugins.
Did I understand it correctly?
Comment #9
borisson_So, after the mess we just made in #2642534: processors shouldn't be unset in Index::__sleep, processorPlugins should be, swentel noted that having things + thingInstances is not super clear.
I think we could probably do $thingSettings + $thingInstances as variables and getThings + getThingInstances as methods?
Comment #10
drunken monkeyYeah, sure, but that also shouldn't touch the
$this->fields
array, just$this->fieldInstances
.Makes sense, yes. That will also get rid of that insanely complex and still error-prone
$would_overwrite
check.Depends on what that would do. It shouldn't touch any of the
$this->*Instances
properties, since those aren't caches anymore.Then the question is, when should the caches even be reset? And the answer is, I think, that only specific caches should ever be reset – the fields caches in
addField()
andremoveField()
, for example, or just thegetFulltextFields()
cache when a field's type changes to/from fulltext (see below).The complexity here is that that check would need to be in
Field::setType()
and then invalidate a cache on the index object, and a)Field
does currently not contain any such complex logic, it's more a data wrapper, and b) all the other index method cache logic is, unless I'm mistaken, currently encapsulated in the index, which makes a lot of sense.I feel that adding such a check and cache invalidation logic would cause unnecessary complexity and possible confusion, while I'd perceive the scenario in which it would be needed as extremely unlikely.
However, I might be mistaken, and of course you're right in saying that if it does occur it might cause some pretty large WTFs. I just can't really picture a scenario where it would.
Yes, more or less.
In my opinion, though, we shouldn't load all the instances in the constructor, that can in all cases remain in the associated getter method, I'd say. We just need to make sure to load that array properly in the
add*
/remove*
methods (andrename*
for fields). But otherwise we might waste considerable performance on some page requests.The reason we currently have it like this is that the settings properties of the index directly correspond to the fields of the exported index config, and having
field_settings
,processor_settings
, etc., there would be rather ugly in my opinion. (As would then having$this->field_settings
and$this->fieldInstances
.)I guess we could override
toArray()
orget()
to remove this direct mapping, but that is bound to cause more confusion than it solves, I'd say.But yes, I do admit that it's currently confusing. That's also why I suggested overwriting
$this->fields
with the objects internally (but pretty surely another case of creating more confusion than resolving). But maybe if all the four or five sets of properties work the same it will become clearer and simpler over time.Comment #11
borisson_Sure, that makes sense.
I don't think it'd be ugly to have them as thing_settings, it is more verbose but I like variables to be as explicit and verbose as needed. I agree that thing_settings + thingInstances is weird.
This would a very search_api specific solution for a common problem, and I don't think we should do that.
This would a very search_api specific solution for a common problem, and I don't think we should do that.
++
Comment #12
drunken monkeyOK, yes, the first part is disputable, and probably not too bad.
Having
thing_settings
andthingInstances
is definitely aweful, but apparently can't be helped. So just go for it, if you want! The most important thing here is probably just consistency.As said, maybe even make the tracker settings consistent? We could even have
$index->tracker_settings[$tracker_id] = $tracker_config
, even though there will always just be one member in that array – would simplify the code and also make it more consistent. But would probably need good comments to not make it too confusing.Comment #13
borisson_This still breaks in a couple of wonderful ways and I can't really figure out why yet. This patch is just intermediate work and probably shouldn't be reviewed yet. I'll post an update later.
There are also some things that aren't really related in this patch. I'll figure out if we should split part of that off into another issue after tests here are green.
Comment #16
borisson_I had a little scare earlier that an index with an integer as id was broken. I added a test to verify this. This also fixes at least one test (RenderedItem). Let's see how the bot feels.
Comment #19
borisson_fixes CustomDataTypesTest.
Comment #22
drunken monkeyWow, great work so far, thanks!
Quite a huge patch already …
A quick optical review (just Dreditor, not even in the IDE yet):
This additional nesting is pretty pointless for datasources and the tracker, I'd say. The
plugin_id
proprety is really just needed for config schema reasons for processors, since they have both their settings and their weights nested. The other plugins can just have the ID directly mapped to the settings.Or did you choose this on purpose, to be even more consistent?
Why FALSE and not NULL?
This will throw an exception if an illegal/unknown tracker plugin is set, which would make the method pointless.
Maybe use the default_tracker setting here? (And just !$settings.)
You can just use reset() for that. Also, use key() afterwards, then you don't need the plugin_id property.
Can the tracker settings ever be NULL? If so, this needs to be taken into account in getTrackerId(), too (or, better, changed). Otherwise, this should go.
As mentioned, I'd say we shouldn't even provide access to those raw settings – just to the tracker plugin itself, with the settings in its configuration.
Also, especially in the case of the tracker, I wouldn't return the whole array, even if the method stays, but just the settings for the tracker that is actually set.
While we're at it: I'm pretty sure the class_exists() isn't needed here? I think createInstance() would take care of that. We should just catch exceptions thrown from it (and then log, like currently in else here).
Why?
When it's a method call, I'd just use ! instead of empty().
Why add this, I don't think that's new? I think since it's just our own, internal code, if you really want to document this an assert() would make more sense.
(Same for the others below.)
Why that addition? And why not just use $index->getTrackerId()?
Does an index without datasources ever exist? It really shouldn't. And why add this to the test backend, of all places?
Also, it seems you're still writing to the field, datasource, etc. settings properties directly, not only when saving the index. Is that part still coming, or did you decide against it, or did I just not explain it very well?
Or should I just not have reviewed this at all yet? I wasn't sure whether that only applied to the patch in #13 or was still valid for #16.
Anyways, I hope some of my comments still help.
Comment #23
borisson_reset()
now, using plugin for consistencyswitched to getTrackerId, good suggestion.
I will do more work about writing into the fields later on; I'd like to get the tests green before trying that.
Comment #26
borisson_Comment #29
drunken monkeyOK, then let's go with consistency for the plugins and keep
plugin_id
.Then better just change the documentation, yes, but maybe really do add an
assert()
.Since there are otherwise some weird problems with caching, implementing this might actually help with the tests.
On the other hand, since it seems you didn't yet remove and
resetCaches()
calls, that shouldn't be much of a problem. So, yes, proceed as you like.Comment #30
borisson_Changed those calls to asserts, also removed [g|s]et[Processor|Field|Tracker]Settings. Did some more work on
DependencyRemovalTest
, but that still fails. Loads of schema errors but I'd like to see what else fails.Comment #33
borisson_Comment #36
borisson_Fixes at least he RendereditemTest, should also fix the LanguageIntegrationTest.
This patch + the one in #33 also add a new test in the rendereditemTest that was really useful while getting the other bug fixes out. Not sure if we should keep this in.
Comment #39
drunken monkeySure, why not? It's a Kernel test anyways, so cheap enough, I'd say. Can't do any harm, in any case.
I guess I'll just wait until the tests pass before reviewing again. Or would you like me to take a look right away? Just say so/ping me.
Comment #40
Nick_vhGave it a try to reduce some of the errors in the dependency test. Most of them were in the test themselves. What I ended up with is the following:
This means that when the dependency is set for removal it deletes the whole tracker settings array and it does not revert to the default tracker. When the dependency is not set for removal it somehow ends up with the default tracker. This leads me to believe that the new tracker we actually want to install to test our dependencies on is not really installed or configured correctly.
Comment #41
Nick_vhComment #42
Nick_vhWrong extension...
Comment #45
borisson_@Nick_vh and I discussed this, we figure it's a smarter idea to try starting over. attached are 2 patches, 1 with the new tests added over the previous patches + 1 with changes to the field-storage. Testsuite is green.
Comment #48
borisson_Second patch in #45 was the wrong patch.
Comment #49
borisson_Go testbot, go.
Comment #50
borisson_Changed to tracker_settings + moved to getTrackerInstance over getTracker.
Comment #51
borisson_DependencyRemovalTest::testDatasourceDependency, both @Nick_vh and I don't really understand these lines:
That test currently passes but I don't really understand it. We should probably explain more in code. There are several other things that fail regarding item counts. I have no idea why those fail either, just uploading this interdiff for now and diving back in.
Comment #54
drunken monkey$remove_dependency
determines whether the test plugin in question (in your case, the datasource) should remove the dependency from its configuration.If
$remove_dependency == TRUE
, inPlugin::onDependencyRemoval()
it clears its configuration (and thus its dependency, in those test plugins) and returnsTRUE
, which the index will take as "all OK, dependency removed" and leave the plugin where it is, only with updated configuration.If
$remove_dependency == FALSE
,Plugin::onDependencyRemoval()
will do nothing and just returnFALSE
, the index says "oh, that plugin still has that removed depenency, so I should better remove the plugin" (yes, indexes can talk) and the plugin gets removed.Therefore,
if ($remove_depenendcy)
we check that the datasource plugin is still there (was not removed from the index inIndex::onDependencyRemoval()
but that it's configuration was correctly updated (to an empty array, in the case of our test plugins).else
we check that the datasource really was removed, i.e., doesn't appear in$index->datasources
anymore. (Though I see now that there is a small typo/mistake/copy-paste error in there, because the two lines inelse
of course test the exact same thing in different ways.)Does that answer your question? Otherwise, what exactly is unclear?
Anyways, you are of course welcome to update the comment to something that makes sense to you. I'm usually not very good at seeing what is obvious and what needs explanation, and how to best explain (or name) things.
Comment #55
borisson_I added that as a comment in the code. This makes more sense but I can do this is another issue or revert the change if you want.
Comment #56
borisson_Attached patch adds extra failures in the
ContentAccessTest
.Comment #57
borisson_Run all the tests, testbot.
Comment #60
borisson_Add extra asserts.
Comment #63
borisson_Improved the description in the schema.yml file. Still fails
Comment #64
drunken monkeyThat was really hidden in a mean way.
Comment #67
drunken monkeyThe three remaining fails seem to be a different problem, which will hopefully be easier to solve. Can I leave that to you?
Otherwise, just ping me.
Comment #68
borisson_Fixes remaining tests. They were new assertions, added in #56. The fails were a case a bad counting on my side, they are green now.
Thank you so much for your awesome help @drunken monkey.
Comment #69
borisson_Comment #72
borisson_Rerolled.
Comment #73
borisson_Comment #74
borisson_Some overlooked conversions.
Comment #75
borisson_removed get/setFieldSettings.
Comment #76
borisson_Still fails in a couple of places, getting this up as intermediate work. - removed get/set Processor Settings.
Also adds a couple of extra checks in the
ProcessorIntegrationTest
.Comment #79
borisson_Fixes one of the fails; this is of a new assertion added in #76, so oops. 2 other fails still expected.
What is weird is that the failure occurs in
checkProcessorChanges
andconfigureFilter
, when triggered troughtestFramework
but not when triggered troughtestIntegerIndex
.Looking into that.
Comment #82
borisson_Fixes the remaining 2 failures. I think this is ready for an initial round of reviews. We should improve the caching as well but this is a big step in the right direction imho.
Comment #83
borisson_Go testbot, go!
Comment #86
drunken monkeyWow, awesome job! That patch is insane … (Although it might be even worse to review, with all those refactoring changes the IDE does automatically …)
And still a lot to do, I'm afraid. I really feel bad that you took this issue …
If you want, we can also swap? I think I've solved most of the hard problems in the Views fields issue (haven't yet posted a new patch, though), so maybe that would be fairer.
Anyways, my patch review:
That line was completely out-dated even before the patch. Let's fix it.
Also, I actually want to get rid of that
$only_enabled
parameter, it's almost never used and just complicates things (a lot).Incidentally: doesn't it complicate this patch, too? Without it, you could work directly with the properties, but with it you have one intermediate step again, and dealing with datasources and processors becomes both more complicated and inconsistent (compared to the tracker and fields).
Would initializing this with an empty array cause any errors? I'd say this is a pretty unclean way of setting a default for the tracker (and it also doesn't (and can't) use the config setting). Even though it's of course just the translation of the previous one, I get that.
array_flip(array_keys())
? Seems like a no-op (in this case).The weights should be set to the defaults, I reckon, passing the settings shouldn't be necessary and I'd just add it to the processorInstances property – if we directly manipulate the processors only in a few methods on the index, we can just keep the array up-to-date.
As said earlier: not a "cache", but really the canonical representation. Then we wouldn't even change the
processor_settings
property there, but just inpreSave()
write back the configuration from the plugins. (Would need a storage and retrieval mechanism for processors on the processor plugin, though – that could be tricky, or would at least be another larger framwork change.)Also, if we do reset the
processorInstances
property, I'm pretty sure it should be set toNULL
. And the check inloadProcessors()
changed toisset()
. Doesn't make sense this way, you could never cache an empty processors array. (Although that won't happen in practice anyways, of course, due to the locked processors.)As above: don't work with
field_settings
, work withfieldInstances
. Take care of the settings only on save. That should save a lot of trouble with the cache (which is the whole point of this issue).And with the addition of a
getSettings()
method toIndexPluginInterface
(maybe even on a new common interface withFieldInterface
) you could simplify the code for that a lot – just reset the*_settings
arrays, go through*Instances
and savegetSettings()
back. (Would also require the above change for weights on the processor plugin, though. So maybe really do that?)The whole code in this method now seems quite confusing, with "settings" as names for arrays of processor objects, and then even the additional
$processors
array. I cleaned that up a bit.However, this part:
is actually wrong now, since the method parameters should be arrays containing both weight and settings.
Maybe another reason to store the weights inside the processors – maybe even just in the settings? Doesn't seem to make much sense to store them elsewhere, it would simplify the code and we could even get rid of
plugin_id
in all plugin settings properties.No, it doesn't (anymore). This can be fixed and unified now. (But could also be done in a follow-up.)
Why does this not use methods like for processors and fields?
Same for the tracker settings below.
Here and in the rest of the method:
sort()
returns a boolean and sorts the array by reference – comparing its return value won't do what intended.Also, why even add this? This should already be taken care of in
enableProcessor()
, more or less. Maybe check in the end whether all of them are enabled, if you like, but I don't think checking after every method call will get us anything.I see no reason why fields and processors shouldn't just use
set()
, or be included in the initial$values
, in a test case. We don't need to use the API there.I'd say that's a bit overly verbose for a parameter name – and it also doesn't really express the whole meaning, since it doesn't only influence the return value, but also the functionality of
Plugin::onDependencyRemoval()
.If you find the parameter is unclear, please expand the (doc) comments, but I don't think the rename helps.
Instead of
assertContains()
andarray_keys()
, just useassertArrayHasKey()
.Those two lines test exactly the same thing, one can be removed now. (Keep the lower one, but with the message of the upper one.)
Line could be broken later!
Hah! That I live to see this day … ;P
Why? That doesn't make any sense. For
TRUE
, the plugin is kept, yes, but the dependency isn't.Also, we could now probably unify most of the methods for all three plugin types, right? Could save quite a bit of code. But that's also possible in a follow-up, of course. (While we're at it, we could also add a public
createPluginInstance()
method to theIndex
class, which would be an easy way to substitute the current$only_enabled
parameter on the getter methods.)In general, we should probably take the same approach here as in #2574969: Add a Views-like UI for adding fields: make it work, then commit it, then polish it.
And to summarize my checklist for whether this issue is RTBC yet:
Index::resetCaches()
is gone.A bit of a simplification, but that's the main goal of this issue, I guess. I'd also just remove our persistent cache for now, and see later if we really need anything cached. Due to #2574969: Add a Views-like UI for adding fields it serves considerably less purpose now, I'd say. "Get rid of it and add it once someone complains" seems to be the smart move here to me.
Comment #87
borisson_$only_enabled
parameter for now.$tracker_config
and$tracker
)I like your notice of removing the plugin_id and moving the weights to the settings.
This helped in fixing the code. (The problem I had was that all processors were enabled all the time and the tests didn't catch that). I only added the sorts after I fixed the code for the first 2 processors there. I've changed the test so it actually tests what we expect it to test now.
Remaining work:
addDataSource
/removeDataSource
andsetTracker
as methods and remove all$index->[s|g]et("key", $value);
calls for datasources and tracker with those.resetCaches
Comment #90
drunken monkeyWhy would having no tracker in the array lead to schema errors? That doesn't seem to make sense to me.
Also, we still would never save an index without a tracker, it's merely about initializing. So the config schema shouldn't notice that change.
Or is the tracker actually never set currently? Then fixing that would of course be a separate issue.
Of course not, it just looks weird to have both styles in the same method. But yes, not a big issue, just keep it like that if you prefer.
Comment #91
borisson_Attached is the work I did on this patch yesterday, I moved the weights into settings and still get the same failures. For some reason there's a difference between
$old_processors
and$new_processors
inIndexProcessorsForm
. I'll try figuring that out further now. I really want to get tests green before doing any of the other todo items.Comment #94
borisson_This fixes the failing IntegrationTest, I feel very sad that this 2kb fix cost me close to a day of work.
Anyway, next up is removing all calls to resetCaches.
Comment #95
borisson_part 1 of removing resetCaches.
Comment #96
borisson_Removed resetCaches, still 1 fail in
\Drupal\Tests\search_api\Kernel\CustomDataTypesTest
that I can't explain.Comment #99
borisson_I figured this was something related to the caching of the field, but it looks like it isn't. More lines of testcode - still the same fail.
Comment #102
drunken monkeySince you removed caching from
getFields()
, this can't work.$this->index->getField()
will return a new field object, you set something on it, but right afterwards it's lost forever and can't possibly influence$item->getField()
, which will get a completely new field object from the index.The proper solution here is the next step – i.e., have one
Index::$fieldInstances
property with all the field objects, which you only ever populate a single time during a page request, and write back tofield_settings
inpreSave()
.For the moment, though, just calling
$index->addField()
should also do the trick.Comment #103
borisson_I spent some work on that removing all
$index->set(thing, value)
and$index->get(thing)
calls and got the tracker to work, that one's easiest. I also spent about 2 hours on doing the same for datasources and another hour on doing that for processors as well, but those are a little bit harder.Anyways, patch attached and I'm trying to do more on this today. I would've loved to get this patch in before sitting down to discuss all caching related things with Wim tomorrow.
Also: hurray 30+ patches.
Comment #104
borisson_Part of the processor changes are now done, still some fails in the integration tests that I can't pinpoint now but my day 's done for now.
Comment #107
borisson_Comment #108
BerdirI've just look at the issue summary which is probably outdated at this point and doesn't contain the solution but that sounds vey similar to the issues that we've been fighting with in #2541206: Consider field storage dependency removal on Index? If this addresses that, then we can hopefully simplify that other patch and finally get it in :)
Comment #109
borisson_I'm not sure if this patch will fix the other issue. This will 100% break that patch though. I did more work on #102 - adding a
$fieldInstances
variable that is the canonical storage.Comment #112
BerdirSorry, I possibly wasn't clear. I don't expect that it will *fix* that issue. But while testing that, with tests and manually, we also did run into all kinds of issues with persistent and static caching of $this->fields. We tried to solve that there too. If this does indeed fix that problem then it should allow us to focus on the actual problem there.
And yes, I expect it will conflict, that's OK :)
Comment #113
drunken monkeyYes, exactly that is the plan, and more or less the main motivation behind this issue: solve all those intricate problems, like the one in the other issue, caused by our chaotic caching code.
Comment #114
borisson_I don't understand the remaining fail yet but it's going in the right direction.
Comment #117
drunken monkeyAwesome job, Joris, looks really great already!
Since I'm gonna be on vacation next week, which is of course pretty bad timing, I'm gonna review this now so hopefully when I get back I can just commit. ;)
Maybe just create a new
search_api_processor_config
config type and use that as a base instead of mapping? That way you wouldn't have to copy that "weights" definition to all individual processor definitions.Views handlers, e.g., work a lot with that (since they all have a gazillion options, and mostly from the base class).
We can't use
getTrackerInstance()
ingetTrackerId()
, otherwise this and (worse)hasValidTracker()
might throw an exception.It should be the other way round – you already have a way to get the tracker ID in
getTracker()
, just move it togetTrackerId()
instead and call that there.I think weights should have getters and setters. We store it in the processor configuration by default, but I really don't think we should hard-code it – it's still the processor's "autonomous" configuration. (Also, that way the code for getting the default can just be internal to processors and we don't have to deal with it here (and in the admin UI).)
Minor nit-pick: use double quotes here instead of escaping the apostrophe.
Exactly what I meant, great!
However, instead of doing it twice, maybe just do it once at the very end of the method. And instead of resetting the locked/hidden flags in the settings, just reset them on the field objects and see whether the processors change them back.
I don't know whether that will work, though, there's probably a reason why you're currently doing it twice.
(Mega-quote, sorry.)
It seems to me that if you merge the writing of the settings with the locked-status check above, and update the processors with
addProcessor()
accordingly, you can get rid of those ugly last two lines.But I might be mistaken, of course, there seems to have been a need for them.
I think this whole block can just be removed now.
However, a lot of
onDependencyRemoval()
can now be improved, so we can also just leave it here and overhaul the whole method in a follow-up.I don't see any reason for that, I see no way how this could be
NULL
, or not an index. (Except a botched unserialization, but at that point I'd say failing is a pretty good option.Or, at the least, we should then check for whether the
$this->indexId
property is there and compare that. (And not unset it in__wakeup()
if the index load failed.))Is this needed for some tests, maybe? In that case, maybe just leave it in there and create a follow-up for fixing this according to my suggestion.
But all in all, again, great work! Thanks!
And maybe one of my suggestions will actually fix the patch fail, some of them could become bugs in certain cases. (Though the test fail doesn't look like it.)
Once you think this is RTBC, I guess either work on Facets, or create Search API patches already based off of your patch, and keep branches so you can easily rebase them?
Sorry, really bad timing with the vacation, having such an uncommitted giant lying around.
Comment #118
borisson_#117
tracker_settings
. It has a fallback to the default tracker - because there always has to be one tracker I don't think this will lead to any problems. The tests don't seem to think so either.Attached patch fixes/answers most of the remarks from #117. I don't really understand .3 - but I'll give it a go now.
Anyway, tests are green again locally - hopefully the testbot agrees with that.
Comment #119
borisson_Ok so I didn't really understand #117.3 - I did however read over the entire patch again and found a couple of things that could be better, so I fixed them. Some other fixes still to come.
Comment #120
borisson_There are still a couple of instances of $index->set('datasource_settings', ...) left in the code, going to rip those out now. This attached patch did that for processor_settings.
Comment #121
borisson_As promised earlier - I ripped out all occurrences where
datasource_settings
was being talked to from outside the index class.Comment #122
borisson_Comment #123
drunken monkeyAgain, thanks a lot for your work! Looks very good now, I guess we are about to finally commit this.
Shouldn't this be a boolean? But it seems it's unrelated anyways, so maybe just open a new issue for that?
Otherwise, I have just written my comments in patch form, please see the attachments.
Comment #126
drunken monkeyAlso, here are the follow-ups:
Comment #127
drunken monkeyComment #128
drunken monkeyComment #131
drunken monkeyComment #133
borisson_I think the failures in #131 are unrelated, there is one change in #127 I disagree with though.
I see you changed this variable, this is not correct though, we're creating an instance of the tracker plugin, not getting the ID.
Comment #134
drunken monkeyThanks for spotting that!
That's just an unintended side effect of a change further down in the same method, where
$tracker
was used to hold the tracker ID. All the better that you spotted it!The test fail looks indeed very strange. Let's just give the test bot another chance …
Comment #136
drunken monkeyExcellent, passing again. Got the OK from Joris, too, so: committed!
Thanks again everyone, especially of course Joris!
Comment #138
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedThe following change causes a
Fatal error: Nesting level too deep - recursive dependency?
becauseold_processors != $new_processors
will recursively compare an array with recursive data.