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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

  • bircher committed 0585504 on 8.x-1.x authored by tstoeckler
    Issue #2925084 by tstoeckler: FilteredStorage::createCollection() needs...
bircher’s picture

Status: Needs review » Fixed

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

bircher’s picture

Status: Fixed » Needs work

Hmm 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!

bircher’s picture

attached the proof with a test.
The clone in the base class is still a bug though.

bircher’s picture

Status: Needs work » Needs review

setting to needs-review to run the tests, but actually I will just commit it...

The last submitted patch, 5: 2925084-test-only.patch, failed testing. View results

  • bircher committed 1c4a1a0 on 8.x-1.x
    Issue #2925084: Add test for generic createCollection setup
    
  • bircher committed 6164517 on 8.x-1.x
    Issue #2925084 by bircher, tstoeckler: FilteredStorage::createCollection...
  • bircher committed f0500b7 on 8.x-1.x
    Revert "Issue #2925084 by tstoeckler: FilteredStorage::createCollection...
bircher’s picture

Status: Needs review » Fixed

now with a test it can be properly marked as fixed.

tstoeckler’s picture

Hey 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 the new 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!

Status: Fixed » Closed (fixed)

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