Title says it all. Caused by #636454: Cache tag support

CommentFileSizeAuthor
#13 1597888-cache-nullbackend-13-test-only.patch1.14 KBNiklas Fiekas
#13 1597888-cache-nullbackend-13.patch2.08 KBNiklas Fiekas
#7 1597888-cache-nullbackend-6-interdiff.txt806 bytesNiklas Fiekas
#6 1597888-cache-nullbackend-6-test-only.patch1.18 KBNiklas Fiekas
#6 1597888-cache-nullbackend-6.patch2.11 KBNiklas Fiekas
#6 1597888-cache-nullbackend-6-test-only.patch1.18 KBNiklas Fiekas
#3 1597888-cache-nullbackend-3-test-only.patch1.18 KBNiklas Fiekas
#3 1597888-cache-nullbackend-3.patch2.11 KBNiklas Fiekas
drupal8.cache-nullbackend.patch955 bytessun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Major

This gives a fatal error, so actually critical, but since unused, just bumping to major.

Crell’s picture

Crell commented

Well that's embarrassing. Should we add at least a trivial test to verify that the class works?

Niklas Fiekas’s picture

Niklas Fiekas commented
StatusFileSize
new 1597888-cache-nullbackend-3.patch2.11 KB
new 1597888-cache-nullbackend-3-test-only.patch1.18 KB

Adding a basic test.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

That should be a unit test rather than extending CacheTestCase no?

I opened #1599578: Add/adopt high level cache tests and run them against each backend to see if we can put something in place to avoid adding separate tests for each backend.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Needs work » Needs review
StatusFileSize
new 1597888-cache-nullbackend-6-test-only.patch1.18 KB
new 1597888-cache-nullbackend-6.patch2.11 KB
new 1597888-cache-nullbackend-6-test-only.patch1.18 KB

Yeah, I think it might be safe to assume that the NullBackend doesn't require the datbase, probably.

Niklas Fiekas’s picture

Niklas Fiekas commented
StatusFileSize
new 1597888-cache-nullbackend-6-interdiff.txt806 bytes

Last file should have been the interdiff :)

Status: Needs review » Needs work

The last submitted patch, 1597888-cache-nullbackend-6-test-only.patch, failed testing.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Needs work » Needs review
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

The test case should be created as PSR-0, no?

Niklas Fiekas’s picture

Niklas Fiekas commented

Since it's a system test, would it go into core/tests/? Does the autoloader find it there? In what directory exactly? In what namespace?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

See #1593058-2: Remove system.info's files[] entry.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Needs work » Needs review
StatusFileSize
new 1597888-cache-nullbackend-13.patch2.08 KB
new 1597888-cache-nullbackend-13-test-only.patch1.14 KB

Thank you, klausi!

(Not using core/tests or core/lib/Drupal/Core/Tests as of #1593058-4: Remove system.info's files[] entry. Maybe move around later.)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Looks good!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Fixed » Reviewed & tested by the community

Looks like this didn't make it up to git.drupal.org. Are you sure you pushed?

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Looks like not. I've gone ahead and committed/pushed this.

Niklas Fiekas’s picture

Niklas Fiekas commented

Excellent. Thanks.

Status: Fixed » Closed (fixed)

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