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

Don't cache entities #46

Closed
ckilb opened this issue Aug 29, 2022 · 5 comments
Closed

Don't cache entities #46

ckilb opened this issue Aug 29, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@ckilb
Copy link

ckilb commented Aug 29, 2022

Right now hydrated entities are cached in
\Vin\ShopwareSdk\Repository\Traits\EntityHydrator::hydrateEntity

$cacheKey = $entityRaw['type'] . '-' . $entityRaw['id'];
if (array_key_exists($cacheKey, $this->cache)) {
return $this->cache[$cacheKey];
}

This should imo get removed without replacement.
I'd not expect the SDK to do caching for me. In case I want caching I could easily implement it on my own.

There are some downsides of caching:

  • If you search for an entity without associations and later on search for the same entity with association, the "new" hydrated entity is actually the old one without the associations added -> this is what took me quite some time to figure out.

  • If you use some web server that doesn't end the PHP progress after the request has been finished this value will be cached forever. Even if the next request comes in after minutes/days/weeks the old value of that entity is still cached. I know common webservers like Apache or Nginx with PHP-FPM start a new process with each request - but new approaches are coming and get more and more popular (Swoole, Roadrunner).

Like I said I don't think caching is necessary (the request to actually get the entity data from Shopware is probably much slower than hydrating the entity) so I'd remove it. In case you think caching should still be present it would be nice to be able to enable/disable it (via Context flag?) or at least to clear it (via method call).

===
Another proposal related:
For now I'd like to replace the method hydrateEntity with my own. If hydrateEntity would be public, EntityHydrator would be a class that get's initialized by some factory and injected to the EntityRepository this would be quite easy.
But as hydrateEntity is a private trait method inside a Trait which is hardly coupled to EntityRepository this is a bit tricky to do.
In general I'd suggest - at least for a library - to avoid Traits and rely on DI instead.

===
PR: #47

@vienthuong
Copy link
Owner

vienthuong commented Aug 30, 2022

I'd agree with your suggestion 👍🏽
Thanks for your contribution :)
The MR looks good but please add the changelog for what you have changed in the new version

@vienthuong vienthuong added the enhancement New feature or request label Aug 30, 2022
@ckilb
Copy link
Author

ckilb commented Sep 5, 2022

hey @vienthuong thanks for your feedback. just pushed the changelog 👍

@vienthuong
Copy link
Owner

vienthuong commented Sep 7, 2022

Hi @ckilb
Can you resolve the conflict :) after that I can merge

@ckilb
Copy link
Author

ckilb commented Sep 7, 2022

Hey @vienthuong
thanks for merging! Could you tag the new version so Composer is able to find it? Thanks!

@vienthuong
Copy link
Owner

vienthuong commented Nov 16, 2022

@ckilb Sorry for the late response,
Hi it should available on latest version 1.7.1 and 1.7.2
Feel free to reopen if you encounter any issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants