Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clarification about expiration of items #1262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Seldaek
Copy link
Contributor

@Seldaek Seldaek commented Jan 12, 2022

See symfony/symfony#44995 for details. The spec isn't extremely clear about this but all implementations I checked (very reasonably) do this already. It does not hurt to spell out the "obvious" IMO as it may not be obvious to everyone.

I am not sure if this belongs here or rather in a ML post, but anyway this is a starting point for a discussion.

@Seldaek Seldaek requested a review from a team as a code owner January 12, 2022 14:10
Copy link
Member

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Depending on existing text, it could be considered either a clarification or an errata (that requires a vote).

Either way, @Crell is the editor and final authority here.

@Crell
Copy link
Contributor

Crell commented Jan 12, 2022

Hrm. I think the original logic from the spec authors was along the lines of "all cache writes are independent of each other. The existence of a previous record is irrelevant, it gets overwritten entirely." "Entirely" means "including its TTL." So of course load/save like that would result in a new TTL being set. I think that is still the correct behavior; A cache is not a database.

Editing the spec text directly is a no-go. But we can include an Errata that clarifies the above reasoning and specifically recommends that implementations always treat a cache write as a full-overwrite, and not persist a TTL.

@Seldaek
Copy link
Contributor Author

Seldaek commented Jan 13, 2022

The thing is all implementations I checked appear to do it correctly. What needs fixing is the user documentation to ensure people don't have the wrong expectations and try reusing / updating a cached item.

So maybe the wording can be improved, but IMO it makes a lot more sense to update the docblock with a clarification for users than an errata virtually no user will ever look at.

@Crell
Copy link
Contributor

Crell commented Jan 14, 2022

I think we can update the docblock in the actual package. We've tweaked those as part of the type updates before. I am not sure if updating the spec proper is kosher. However, I'm open to input from other CC members.

@Seldaek
Copy link
Contributor Author

Seldaek commented Jan 14, 2022

Ah yes an errata in spec and then a docblock patch in the spec would be totally fine for me too.

@shadowhand
Copy link
Contributor

an errata in spec and then a docblock patch in the spec would be totally fine for me too.

Agree.

@drupol
Copy link
Contributor

drupol commented Jan 15, 2022

Agree as well.

@Jean85
Copy link
Member

Jean85 commented Jan 15, 2022

Agree. Marking the errata in the meta document is what makes it possible.

@ashnazg
Copy link
Contributor

ashnazg commented Jan 15, 2022

+1 from me

@vdelau
Copy link
Member

vdelau commented Jan 21, 2022

Just to make sure that we are all talking about the same thing: The proposal is to add errata and documentation to clarify that TTL might not be set when retrieving a CacheItemInterface object from a CachePool and that when 'updating' a cache item it is the responsibility of the user to set a new TTL?

@Seldaek
Copy link
Contributor Author

Seldaek commented Jan 21, 2022

Yes, add errata and update at least the docblocks in the psr/cache repo, if the docblocks in the spec cannot be updated due to red tape. Exact wording is TBD as far as I am concerned, I just thought it'd be good to highlight the fact that yes TTL might not be set (probably SHOULD not be set) on a CacheItemInterface object retrieved from the cache, and that on save() a previously stored item with the same key will be overwritten completely, inclusive TTL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants