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

[Cache] Allow ISO 8601 time intervals to specify default lifetime #38253

Merged
merged 1 commit into from
Oct 5, 2020
Merged

[Cache] Allow ISO 8601 time intervals to specify default lifetime #38253

merged 1 commit into from
Oct 5, 2020

Conversation

lstrojny
Copy link
Contributor

@lstrojny lstrojny commented Sep 21, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Psr\Cache\CacheItemInterface::expiresAfter() takes DateInterval for expiration but Symfony’s cache config only takes an integer. Time intervals are a bit better to read so this adds the capability.

@fabpot
Copy link
Member

fabpot commented Sep 22, 2020

As this is a new feature, this should be based on master.

@nicolas-grekas nicolas-grekas modified the milestones: 4.4, next Sep 24, 2020
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to master September 24, 2020 13:29
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

It would be great to allow using regular strings also, via e.g. strtotime('5 seconds', 0)

@fabpot
Copy link
Member

fabpot commented Sep 30, 2020

@lstrojny Can you take Nicolas's comment into account; we will close 5.2 for new feature tomorrow, would be great to have this one in.

@lstrojny lstrojny changed the base branch from master to 4.4 September 30, 2020 22:02
@lstrojny lstrojny changed the title [Cache] Allow ISO 8601 time intervals to specify default lifetime [Cache] Allow ISO 8601 time intervals to specify default lifetime, handle env variables properly Sep 30, 2020
@lstrojny
Copy link
Contributor Author

@fabpot @nicolas-grekas while working on the issue I found a bug with how the prefix seed is handled. Because the prefix seed is hashed before setting it as a constructor argument, env variables will not be properly handled. In fact what gets hashed is the %%env(…)%% expression, not the value. The same issue happened with the previous solution for the default lifetime. That’s why I went a different direction and introduced a parameter normalizer that converts the date expression and hashes the namespace expression on instantiation by configuring the normalizer as a factory method. As this is arguably a bugfix now I leave the work based on 4.4 for now.

Questions:

  • Does the general direction make sense? If so, I'll add and fix tests
  • Is 4.4 the right target?

@nicolas-grekas
Copy link
Member

I'd prefer not adding a runtime factory for this.
The seed was resolved at compile-time, which is enough, isn't it?
Can't we do the same for the default lifetime?

@lstrojny
Copy link
Contributor Author

lstrojny commented Oct 1, 2020

@nicolas-grekas unfortunately not, the compile time resolving is broken and additionally only resolving at compile time leads to unexpected results, where one would expect to pass a different env value for the seed and it‘s not picked up

@nicolas-grekas
Copy link
Member

If it's broken ("broken" to be defined) we should fix it. Computing the seed (aka the namespaces of the pools) at runtime is not supported and shoudln't be IMHO. There are many config options that are not compatible with runtime configuration, this is one of them (although this one is compatible with compile time evaluation, which is not the case for many options).

Reconsidering this is another story, not for this PR at least.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 5, 2020
@nicolas-grekas nicolas-grekas changed the title [Cache] Allow ISO 8601 time intervals to specify default lifetime, handle env variables properly [Cache] Allow ISO 8601 time intervals to specify default lifetime Oct 5, 2020
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to master October 5, 2020 10:24
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I rebased on master, removed the part about the seed, and made some minor CS tweaks)

@fabpot
Copy link
Member

fabpot commented Oct 5, 2020

Thank you @lstrojny.

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.

None yet

6 participants