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

FIX: Add _complete flag to ensure that missing record requests are ca… #475

Merged
merged 2 commits into from
Sep 28, 2018

Conversation

sminnee
Copy link

@sminnee sminnee commented Sep 28, 2018

…ched

FIX: Remove unnecessary 2nd cache implementation.

This code improves performance when there are a significant number of
draft pages. Related, there was a duplicate implementation of caching
that wasn’t really necessary.

…ched

FIX: Remove unnecessary 2nd cache implementation.

This code improves performance when there are a significant number of
draft pages. Related, there was a duplicate implementation of caching
that wasn’t really necessary.
@sminnee
Copy link
Author

sminnee commented Sep 28, 2018

FYI @ScopeyNZ found this wee issue missed.

@ScopeyNZ
Copy link
Collaborator

ScopeyNZ commented Sep 28, 2018

Haha we have options now...

#474

Actually I prefer yours. Removes one of the cache properties...

@ScopeyNZ
Copy link
Collaborator

Just running tests locally to double check 🙂

@ScopeyNZ
Copy link
Collaborator

Tests are passing 😉

@ScopeyNZ ScopeyNZ merged commit 8a94b24 into tractorcow-farm:4.1 Sep 28, 2018
$key = $table . '/' . $locale . '/' . $this->owner->ID;
if (isset(static::$localisedStageCache[$key])) {
return static::$localisedStageCache[$key];
} elseif (!empty(static::$idsInLocaleCache[$locale][$table]['_complete'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

elseif adds unnecessary complexity over just if since you're returning before this


// Checking the cache
$result2 = $extension->isPublishedInLocale('en_NZ');
$this->assertSame('foo', $result2, 'Cached result is returned');
$this->assertSame(true, $result2, 'Cached result is returned');
Copy link
Contributor

Choose a reason for hiding this comment

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

I realise the original wasn't representative of the API, but I'd used it so there was no chance of confusion about whether the mock was being used. The update is fine but just thought I'd note why I wrote it that way =)

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.

3 participants