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

Flagsmith instance clonning #75

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

misakstvanu
Copy link
Contributor

Hi,
clonning the instance with every configuration change is very expensive and not necessary in context of this library, I propose just setting the property and returning original object.

This also fixes Flagsmith.php line 183

    public function withCache(?CacheInterface $cache): self
    {
        if (is_null($cache)) {
            $this->with('cache', null); <-- result of this expression is not used anywhere (someone probably missed the clonning)
        }

        return $this->with(
            'cache',
            new Cache($cache, $this->cachePrefix, $this->timeToLive)
        );
    }

@matthewelwell
Copy link
Contributor

I'm not sure why CI hasn't run on this PR but I'm definitely keen to see the tests run given this is a change to quite a fundamental part of the library. Can you make sure that you're up to date with the latest from the main branch in the target repository?

@misakstvanu
Copy link
Contributor Author

I synced the repos.

@matthewelwell matthewelwell merged commit 631bc64 into Flagsmith:main Aug 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants