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

BUGFIX: Handle maximum cache lifetime of 0 #2300

Merged
merged 2 commits into from Feb 1, 2019

Conversation

Projects
None yet
4 participants
@daniellienert
Copy link
Member

daniellienert commented Dec 3, 2018

If a cached prototype had a maximumLifetime of 0
configured, which is meant as infinite cache
lifetime, this value is propagated upwards to all
surrounding prototypes as it is the selected
as minimum value

@daniellienert daniellienert changed the title BUGFIX: Handle maximum cache lifetime of 0 WIP: BUGFIX: Handle maximum cache lifetime of 0 Dec 3, 2018

@daniellienert daniellienert force-pushed the daniellienert:bugfix/handle-maxcachelifetime-of-0 branch 2 times, most recently from 4592641 to fb73067 Dec 3, 2018

@daniellienert daniellienert changed the title WIP: BUGFIX: Handle maximum cache lifetime of 0 BUGFIX: Handle maximum cache lifetime of 0 Dec 3, 2018

@daniellienert daniellienert requested a review from kitsunet Dec 3, 2018

BUGFIX: Handle maximum cache lifetime of 0
If a cached prototype had a maximumLifetime of 0
configured, which is meant as infinit cache
lifetime, this value is propagated upwards to all
souttounding prototypes as it is the selected
as minimum value

@daniellienert daniellienert force-pushed the daniellienert:bugfix/handle-maxcachelifetime-of-0 branch from fb73067 to 8b046e1 Dec 3, 2018

@kdambekalns
Copy link
Member

kdambekalns left a comment

Not sure about the change, can the null checks be dropped or is something else now broken?

@@ -211,11 +211,16 @@ public function preEvaluate(array &$evaluateContext, $fusionObject)
if (isset($evaluateContext['configuration']['maximumLifetime'])) {
$maximumLifetime = $this->runtime->evaluate($evaluateContext['fusionPath'] . '/__meta/cache/maximumLifetime', $fusionObject);
$maximumLifetime = (int)$this->runtime->evaluate($evaluateContext['fusionPath'] . '/__meta/cache/maximumLifetime', $fusionObject);
if ($maximumLifetime !== null && $this->cacheMetadata !== []) {

This comment has been minimized.

@kdambekalns

kdambekalns Dec 5, 2018

Member

After the type cast above, this can never be null, so this check is useless.

This comment has been minimized.

@daniellienert

daniellienert Dec 5, 2018

Author Member

Absolutely! The question is, why the tests aren't failing ..

@jonnitto jonnitto requested a review from bwaidelich Dec 6, 2018

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Dec 6, 2018

So lifetime of 0 should behave the same as null?

@kdambekalns

This comment has been minimized.

Copy link
Member

kdambekalns commented Dec 6, 2018

Not sure how it actually is, but I'd expect null to mean "whatever is the default" and 0 to mean "unlimited".

@jonnitto

This comment has been minimized.

Copy link
Member

jonnitto commented Dec 6, 2018

I‘m here with @kdambekalns

@daniellienert

This comment has been minimized.

Copy link
Member Author

daniellienert commented Dec 7, 2018

@kdambekalns / @bwaidelich
Thats correct. IMHO there are three cases:
0 --> infinite for the cache element where it is defined, should only be propagated to surrounding prototype cache segments if they don't define a maximum lifetime theirselves
null --> undefined, should not be propagated to surrounding prototype cache segments
int > 0 --> Should lower the cache lifetime of surrounding segments if it is lower

@daniellienert

This comment has been minimized.

Copy link
Member Author

daniellienert commented Feb 1, 2019

@kdambekalns , @bwaidelich , @jonnitto - fixed the typecast some time ago. Would you please take another look.

@bwaidelich
Copy link
Member

bwaidelich left a comment

by reading.
Thanks!

@kdambekalns
Copy link
Member

kdambekalns left a comment

Looks good.

@daniellienert daniellienert merged commit 78fc0c6 into neos:3.3 Feb 1, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daniellienert daniellienert deleted the daniellienert:bugfix/handle-maxcachelifetime-of-0 branch Feb 1, 2019

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