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

Use Symfony HttpCache #156

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

baiiko
Copy link
Contributor

@baiiko baiiko commented Jun 14, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no

Hi people,

To most performances, i use Symfony HttpCache go put in cache some call.

It's very interesting for some parallel batch :)

$store = new Store($path);
$httpClient = new CachingHttpClient($httpClient, $store, ['default_ttl' => 3600, 'allow_revalidate' => true, 'debug' => getenv('APP_DEBUG')]);

$client->setHttpClient(new HttplugClient($httpClient));
Copy link
Member

@TheGrimmChester TheGrimmChester Aug 9, 2023

Choose a reason for hiding this comment

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

When I try to import using your PR, I get this error:

In AkeneoPimClientBuilder.php line 103:
                                                                                                                                                                                       
  [TypeError]                                                                                                                                                                          
  Akeneo\Pim\ApiClient\AkeneoPimClientBuilder::setHttpClient(): Argument #1 ($httpClient) must be of type Akeneo\Pim\ApiClient\Client\ClientInterface, Symfony\Component\HttpClient\H  
  ttplugClient given, called in /SyliusAkeneoPlugin/src/Client/ClientFactory.php on line 47 

Copy link
Contributor Author

@baiiko baiiko Aug 9, 2023

Choose a reason for hiding this comment

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

Oh, maybe akeneo, change the contact since my pr is open ... @TheGrimmChester

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirm... Akeneo change that :

akeneo/api-php-client@e41246a

Copy link
Member

Choose a reason for hiding this comment

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

I think we can use the integrated akeneo cache system that creates a CachedResourceClient object to store resources.

$client = new AkeneoPimClientBuilder($apiConnection->getBaseUrl());
$client->enableCache();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is not a good idea, because he use memory cache, and if you have some data, you risk to throw memory limit.

With my system, cache use is file system.

Copy link
Member

Choose a reason for hiding this comment

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

thanks @baiiko. We will check if we have to block the 11.3.0 version to benefits from the possibility to implement caching as you did. I think we should provide a way to store it to different places (like redis...) to be able to use the same cache if we are working on multiple server nodes to process the queue.

Copy link
Contributor Author

@baiiko baiiko Aug 9, 2023

Choose a reason for hiding this comment

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

Actually, is not possible, CachingHttpClient use only file system.

I use this system with your plugin and my process work faster than standard process.

Copy link
Contributor Author

@baiiko baiiko Aug 10, 2023

Choose a reason for hiding this comment

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

You can talk with @oallain if necessary, he can talk with me directly :)

@TheGrimmChester TheGrimmChester requested review from tuyetrinhvo and removed request for tuyetrinhvo August 9, 2023 14:09
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.

3 participants