After calling FilteredStorage::createCollection()
a new instance of FilteredStorage
is returned that is correctly set up with the new collection and the respective filters.
The filters, however, do not have their source and filtered storages set up correctly. If the filter does not override ConfigFilterBase::filterCreateCollection()
this means that the old instances of the source and filter storages are still held in the filters, i.e. those with the previous (incorrect) collection. If a filter (correctly) creates a new instance of itself (like TestSplitFilter
, the source and filtered storage will not be set at all. In both cases, bugs will arise when using the source or filtered storage.
The attached patch fixes this by setting the source and filtered storage just like in FilteredStorage::__construct()
.
It also modifies ConfigFilterBase::createCollection()
to actually clone the filter (since it is not really possible construct a new instance in a generic way) so that a new instance is returned.
Furthermore, I also added a note about creating a new instance to the docs of ConfigFilterInterface::filterCreateCollection()
.
Lastly, I noticed that the keys that the filters are internally keyed by are not retained by FilteredStorage::createCollection()
. Although the keys are not necessarily an API that anyone should rely on, but I do think it is strange that the data structure is inconsistent between ::createCollection()
having or not having been called prior. I hope it's OK to include this here, even though it's not strictly in scope of the bug.
Comment | File | Size | Author |
---|---|---|---|
#5 | 2925084-5.patch | 5.72 KB | bircher |
| |||
#5 | 2925084-test-only.patch | 5.04 KB | bircher |
config-filter-create-collection.patch | 2.1 KB | tstoeckler | |
|
Comments
Comment #3
bircherGreat catch! great patch.
Thanks for the contribution!
I don't mind small improvements that make sense even though they are not strictly part of the scope.
Comment #4
bircherHmm I was too fast with accepting the patch. I now have doubts that the claim in the second sentence is correct.
We need a test for that!
Comment #5
bircherattached the proof with a test.
The
clone
in the base class is still a bug though.Comment #6
birchersetting to needs-review to run the tests, but actually I will just commit it...
Comment #9
birchernow with a test it can be properly marked as fixed.
Comment #10
tstoecklerHey there, thanks for the commit and the test! So I guess the filters do get set up properly already by the call to
FilteredStorage::__construct()
that thenew static()
ends up being. Not sure what I was seeing, but this does work perfectly in my setup as well. I guess it was the missing clone/re-instantiation of the filter plugin that tricked me up. In any case, awesome that this is fixed so neatly and thanks for the quick response!