NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Cache] Apply NullAdapter as Null Object#40780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

derrabus merged 1 commit intosymfony:4.4fromroukmoute:ticket_40753

Apr 12, 2021

Merged

[Cache] Apply NullAdapter as Null Object#40780

derrabus merged 1 commit intosymfony:4.4fromroukmoute:ticket_40753

Apr 12, 2021

Conversation

Copy link

Contributor

roukmoute commentedApr 12, 2021•
edited
Loading

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#40753
License	MIT

There is a problem with the NullAdapter if I have to add an expression to work:

$adapter =newNullAdapter();$item =newCacheItem();$item->set('FooBar');if (!$adapter->save($item) && !($adapterinstanceof NullAdapter)) {thrownewException('Uoh oh');}

So the goal here is to modify the methods that are causing a problem to behave as a Null Object.

carsonbot added Status: Needs Review Bug labels

Apr 12, 2021

carsonbot added this to the4.4 milestone

Apr 12, 2021

nicolas-grekas added the Cache label

Apr 12, 2021

carsonbot changed the title~~Apply NullAdapter as Null Object~~[Cache] Apply NullAdapter as Null Object

Apr 12, 2021

nicolas-grekas approved these changes

Apr 12, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 12, 2021

Copy link

Member

derrabus commentedApr 12, 2021

While I do prefer the proposed behavior, I'm unsure if we should simply change it in a bugfix release. Chances are that there are applications out there that rely on the current behavior.

The current behavior is not completely unreasonable either: The cache never persists the passed value, which is whatfalse indicates. My preference would be to leave the adapter untouched on 4.4 and rather make the return value configurable on 5.x.

Copy link

Member

nicolas-grekas commentedApr 12, 2021•
edited
Loading

On my side, I think this is a really bug fix. The cache basically says: "true, I saved that item", then it expired it immediately, as it is allowed to do. No implementation should rely on the precise behavior ofNullAdapter anyway - and making this configurable would only increase complexity IMHO.

Copy link

ContributorAuthor

roukmoute commentedApr 12, 2021•
edited
Loading

The cache never persists the passed value, which is whatfalse indicates

If I followPSR-6,false on the methodsave() indicates an error not it was not persisted.

stof approved these changes

Apr 12, 2021

View reviewed changes

Copy link

Contributor

ro0NL commentedApr 12, 2021

No implementation should rely on the precise behavior of NullAdapter anyway

yet here we are :)

it expired it immediately

works for me 👍

Copy link

Member

stof commentedApr 12, 2021

No implementation should rely on the precise behavior of NullAdapter anyway
yet here we are :)

the change here is precisely about enabling that, by making sure that NullAdapter does not report an error for its expected behavior.

Copy link

Member

derrabus commentedApr 12, 2021

If I followPSR-6,false on the methodsave() indicates an error not it was not persisted.

true indicates that the items was persisted successfully,false indicates an error. Neither fully applies here, so we have to pick one. And I fully agree thattrue feels more correct, given Nicolas' explanation:

then it expired it immediately, as it is allowed to do

Then again, that does not render the current behavior inherently wrong. And we're talking about changing the existing behavior in a bug fix.

Copy link

Member

stof commentedApr 12, 2021

@derrabus the current code reports NullAdapter as always having errors when trying to save, which then forces you to do a special case for NullAdapter in any code wanting to report failures loudly, because not persisting the value in NullAdapter is not a failure. It is the expected behavior of the adapter to always miss the cache. See the PR description for such a code.
So to me, this indeed qualifies as a bugfix.