Updated: Comment #N

Problem/Motivation

The KeyValueDatabaseExpirableFactory and KeyValueNullExpirableFactory classes currently implement the KeyValueFactoryInterface. This is not correct as these factories should be returning KeyValueStoreExpirableInterface instances and not KeyValueStoreInterface instances.

Proposed resolution

Add a KeyValueExpirableFactoryInterface, so the factories mentioned above can implement this instead.

Remaining tasks

Patch, reviews.

User interface changes

None

API changes

Addition of KeyValueExpirableFactoryInterface.

CommentFileSizeAuthor
#6 interdiff-2160495-6.txt2.74 KBdamiankloip
#6 2160495-6.patch5.76 KBdamiankloip
#2 interdiff-2160495-2.txt1.32 KBdamiankloip
#2 2160495-2.patch3.02 KBdamiankloip
#1 2160495-1.patch3.02 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2160495-1.patch3.02 KB

To start with, I think we can do this.

damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 2160495-2.patch3.02 KB
new interdiff-2160495-2.txt1.32 KB

Whoops, sorry!

The last submitted patch, 2: 2160495-2.patch, failed testing.

damiankloip’s picture

damiankloip commented

So we could get into issues with class KeyValueExpirableFactory extends KeyValueFactory {

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

(05:09:39 PM) berdir: damiankloip: we just need to update the type hint in FormBuilder
(05:10:04 PM) berdir: damiankloip: it has keyvalue.expirable as argument, so it needs to use that interface
(05:10:19 PM) berdir: damiankloip: probably only works otherwise because KeyValueDatabaseExpirableFactory implements both

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2160495-6.patch5.76 KB
new interdiff-2160495-6.txt2.74 KB

In the patch in #2 I changed the KeyValueDatabaseExpirableFactory to not extend the KeyValueFactory. So now it should only implement the KeyValueDatabaseExpirableFactoryInterface.

So you're just saying we should do this? (So the KeyValueExpirableFactory extends KeyValueFactory and implements KeyValueExpirableFactoryInterface, both just having the get() method).

The last submitted patch, 6: 2160495-6.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

6: 2160495-6.patch queued for re-testing.

sun’s picture

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

xjm
she/her
Primary language English
commented

6: 2160495-6.patch queued for re-testing.

damiankloip’s picture

damiankloip commented

6: 2160495-6.patch queued for re-testing.

catch’s picture

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

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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