Problem

  1. Try to install with either of postgresql, or SQLite.
  2. Install freezes and ultimatlely fails.

Background

  1. Filter_plugins are tried to store in cache with 'expires' column set to an Array, which borks.
  2. FilterPluginManager sets the cache with only three arguments, though CacheDecorator constructor takes 4 arguments. Cache tags array is erroneously being passed into expires column.
  3. In the contrary, MySQL database driver silently ignores this.

Proposed resolution

  • Call CacheDecorator with the correct number of arguments (expiration: CACHE_PERMANENT?), then cache tags.

Remaining tasks

CommentFileSizeAuthor
#2 filter-2008644-cachedecorator-arguments-2.patch1.43 KBpancho
#1 filter-2008644-cachedecorator-arguments-1.patch1.3 KBmradcliffe

Comments

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new filter-2008644-cachedecorator-arguments-1.patch1.3 KB

Block module is the only other module to call CacheDecorator with all of the arguments.

Patch for review: should it use CACHE_PERMANENT?

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Priority: Normal » Critical
StatusFileSize
new filter-2008644-cachedecorator-arguments-2.patch1.43 KB

Expanded the issue summary.
This bug keeps D8 from being installed on postgreSQL and SQLite and therefore is critical.

While there are additional bugs to be fixed (see #2001350: [meta] Drupal cannot be installed on PostgreSQL resp. #1998366: [meta] SQLite is broken), this is a confirmed and obvious bug and the fix allows the installer to get a bit further.
CACHE_PERMANENT is the default value for expiration, so unless otherwise set it should be just fine.

Just putting the USE statements into alphabetic order and breaking out two of the arguments to intermediate variables to avoid the horribly long line.
So please do another quick review, otherwise this patch should be RTBC, IMHO. Would be nice to go forward then.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Wow, nice catch!

I'm not 100% sure we can/should write tests for this.

I'll tentatively set RTBC, but if someone wants tests they should be prepared to suggest how to best do that :)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes

Expanded and partially reworded

catch’s picture

catch
he/him
Primary language English
commented

I assume if we had postgres test bots they'd have caught this via existing test coverage (or just not installing at all). Can't think of a test otherwise.

dries’s picture

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

I agree this isn't easily testable. Committed to 8.x. Thanks.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Thanks for committing, and yes, postgres test bots would have definitely caught this.
Checking the type of parameters on all API functions might help, too, but would add an incredible overhead given that PHP is a weakly typed language. Otherwise we unfortunatly can't completely avoid occasional wrong function calls when code is refactored.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Add related issue #1903346: Establish a new DefaultPluginManager to encapsulate best practices, and update summary.