In order to simplify the filter-administration-tests, the method CacheBackendInterface::isEmpty() was introduced in #575360: API function to check if a cache bin is empty. It was already stated in the aforementioned issue that the method will be difficult/impossible to implement for some third-party cache-backends. Nevertheless the patch was included and third-party cache-backends are expected to simply always return non-empty. It was stated that the worst can happen is a test fail.
Regrettably from reading the documentation it is not clear at all, that this method is only intended for test-purposes. In fact it is used in non-test code (e.g. toolbar.module) resulting in bugs like #2254903: PostgreSQL install broken again in HEAD by toolbar module.
Also the invocation in FilterAdminTest::testFilterAdmin is bogus (the reason why this function was introduced in the first place). In fact the assertion there is useless, because the filter-cache will only ever be used when rendering filtered text (i.e. something calls check_markup). However that function is never invoked and nothing is written to the filter cache before the assertion of \Drupal::cache('filter')->isEmpty(). Therefore this assertion should be removed entirely (as demonstrated by the test-results of demonstrate-filter-test-admin-is-empty-assert-defect.patch).
Remove CacheBackendInterface::isEmpty() and reimplement the tests depending on it in a proper way.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 8738933-remove-cache-backend-isempty-8.patch | 14.39 KB | znerol |
| #6 | interdiff.txt | 1.95 KB | znerol |
| #1 | demonstrate-filter-test-admin-is-empty-assert-defect.patch | 2.3 KB | znerol |
Comments
Comment #1
znerol commentedThis actually should fail if the
FilterAdminTest::testFilterAdmin()was implemented properly.Comment #2
znerol commentedIn this patch:
CacheBackendInterface::isEmpty()and also implementations thereofFilterAdminTest::testFilterAdminClearsFilterCache()properly testing cache-clearing upon filter administration.GenericCacheBackendUnitTestBase::testDeleteAll()andGenericCacheBackendUnitTestBase::testInvalidateAll(): Ensure that only the specified bin is affected instead of usingisEmpty()toolbar.module.Let's see if this is already enough.
Comment #4
znerol commentedAdd missing
usestatement toFilterAdminTestand also fixBackendChainImplementationUnitTest.Comment #6
znerol commentedFixes
check_markupinFilterAdminTest::testFilterAdminClearsFilterCache()such that it actually uses the filter cache.Comment #7
znerol commentedComment #9
znerol commentedUpload the actual patch (both in #6 were interdiffs...)
Comment #10
bzrudi71 commentedFYI the isEmpty() stuff currently breaks PostgreSQL installation as identified in #2254903: PostgreSQL install broken again in HEAD by toolbar module so adding as related. Removing isEmpty() and doing the deleteAll() the way like in this patch makes PostgreSQL install again. So please let get this in ASAP.
Comment #11
znerol commentedComment #12
steinmb commentedChanging to critical since this blocks the pg installation, #10.
Comment #13
catchThis should never have gone in (and yes I posted on the original issue without blocking it, shame on me). Nice to see it go out again. RTBC.
Comment #14
catchCommitted/pushed to 8.x, thanks!
Comment #16
mradcliffeBecause this technically is an API change, I wrote a draft change notice: https://drupal.org/node/2263009.
Please review and publish.
Comment #17
berdirI think this only existed in 8.x? We can still do the change record but should mention that.
Comment #18
catchYes I left the change record because this was only an 8.x-8.x change (and should never have been used outside tests).
Comment #19
znerol commentedThere is cache_is_empty in Drupal 7. Let's update the New cache API change record instead of publishing a new one.
Comment #20
mradcliffeI updated https://drupal.org/node/1272696 to mention the removal of isEmpty() and removed it as part of the cache implementation section.
Comment #21
xjmThanks @mradcliffe and @znerol!