Skip to content

[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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 12, 2021
Merged

[Cache] Apply NullAdapter as Null Object #40780

merged 1 commit into from
Apr 12, 2021

Conversation

roukmoute
Copy link
Contributor

@roukmoute roukmoute commented Apr 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 = new NullAdapter();
$item = new CacheItem();
$item->set('FooBar');
if (!$adapter->save($item) && !($adapter instanceof NullAdapter)) {
    throw new Exception('Uoh oh');
}

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

@carsonbot carsonbot added this to the 4.4 milestone Apr 12, 2021
@carsonbot carsonbot changed the title Apply NullAdapter as Null Object [Cache] Apply NullAdapter as Null Object Apr 12, 2021
@derrabus
Copy link
Member

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 what false indicates. My preference would be to leave the adapter untouched on 4.4 and rather make the return value configurable on 5.x.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 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 of NullAdapter anyway - and making this configurable would only increase complexity IMHO.

@roukmoute
Copy link
Contributor Author

roukmoute commented Apr 12, 2021 •
edited
Loading

The cache never persists the passed value, which is what false indicates

If I follow PSR-6, false on the method save() indicates an error not it was not persisted.

@ro0NL
Copy link
Contributor

ro0NL commented Apr 12, 2021

No implementation should rely on the precise behavior of NullAdapter anyway

yet here we are :)

it expired it immediately

works for me 👍

@stof
Copy link
Member

stof commented Apr 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.

@derrabus
Copy link
Member

If I follow PSR-6, false on the method save() 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 that true 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.

@stof
Copy link
Member

stof commented Apr 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.

derrabus
derrabus approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. 🙂

@derrabus
Copy link
Member

Thank you @roukmoute.

@derrabus derrabus merged commit 8c43fac into symfony:4.4 Apr 12, 2021
@roukmoute roukmoute deleted the ticket_40753 branch April 12, 2021 14:27
This was referenced May 1, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug Cache Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@roukmoute @derrabus @nicolas-grekas @ro0NL @stof @carsonbot