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

Update CacheClearer adapter #10770

Merged
merged 12 commits into from Oct 17, 2018

Conversation

Projects
None yet
6 participants
@sarjon
Member

sarjon commented Sep 29, 2018

Questions Answers
Branch? develop
Description? When working with cache clearer i noticed that it does not provide clear interface for that. This PR aims to do that.
Type? improvement
Category? CO
BC breaks? no
Deprecations? yes
Fixed ticket? n/a
How to test? See code changes. Back Office behavior is not changed. It should clear cache as before from "Performance" page.

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 29, 2018

@matks @PierreRambaud @jolelievre can you review? if you think is worth having it, ill update more services to use this interface instead of adapter CacheClearer.

* @param CacheClearerInterface $mediaCacheClearer
* @param CacheClearerInterface $smartyCacheClearer
*/
public function __construct(

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 29, 2018

Contributor

Watch out, with this you introduce a BC break because of the necessary of having parameters.

This comment has been minimized.

@sarjon

sarjon Sep 29, 2018

Member

i know, but do Adapters promise BC? i think that modules should not depend on adapters, but if they do, then they should not expect BC as adapters are temporary services that can change/be removed at any time, no? 🤔

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 29, 2018

Contributor

There is always a difference between what we think and what users do :D Maybe we can for the moment use a function like addCacheClearer and call this class CacheClearerChain?

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 29, 2018

Contributor

With this, we sure we don't have BC or anything else

This comment has been minimized.

@sarjon

sarjon Sep 30, 2018

Member

i could just leave this CacheClearer untouched, because if it is used in some module (even though it shouldnt :D) it will break as you must call addCacheClearer to make it work, right?

This comment has been minimized.

@jolelievre

jolelievre Oct 2, 2018

Contributor

I am not sure about the backward compatibility for Adapter services as well, but anyway I think an addCacheClearer method here wouldn't hurt anyway if you need to add an extra cache clearer layer
If some module is using the service via the container (->get('service_name')) it won't make a difference anyway, if it instanciated it manually, there's nothing we can do anyway.. too bad for them

This comment has been minimized.

@sarjon

sarjon Oct 15, 2018

Member

@matks wdyt? i dont really want do add non interfaced method to a service. we could either dispatch event though which you could add more cache clearers or just use composition.

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Sep 29, 2018

I like the idea ;)

@matks matks added the migration label Oct 1, 2018

@jolelievre

nice job! The final step will be to have correct implementation of each Clearer (no more use of Tools and son on) But it's a good step forward

* @param CacheClearerInterface $mediaCacheClearer
* @param CacheClearerInterface $smartyCacheClearer
*/
public function __construct(

This comment has been minimized.

@jolelievre

jolelievre Oct 2, 2018

Contributor

I am not sure about the backward compatibility for Adapter services as well, but anyway I think an addCacheClearer method here wouldn't hurt anyway if you need to add an extra cache clearer layer
If some module is using the service via the container (->get('service_name')) it won't make a difference anyway, if it instanciated it manually, there's nothing we can do anyway.. too bad for them

Tools::clearXMLCache();
$this->clearMediaCache();
Tools::generateIndex();
$this->entireCacheClearer->clear();

This comment has been minimized.

@jolelievre

jolelievre Oct 2, 2018

Contributor

maybe a trigger_error deprecation notice as well here

/**
* Class EntireCacheClearer clears entire PrestaShop cache.
*/
final class EntireCacheClearer implements CacheClearerInterface

This comment has been minimized.

@jolelievre

jolelievre Oct 2, 2018

Contributor

not sure about the naming EntireCacheClearer which seems too "context oriented" for a class name, I would prefer a more generic CacheClearerCollection
At least for the class, it doesn't bother me that much for the service identifier which can be more contextual

This comment has been minimized.

@sarjon

sarjon Oct 3, 2018

Member

yes, i dont really like it as well, maybe just CacheClearer? 🤔

This comment has been minimized.

@jolelievre

jolelievre Oct 3, 2018

Contributor

I prefer CacheClearer to EntireCacheClearer but maybe it's a bit too generic, as it hides its ability to mix/compose different CacheClearerInterface

This comment has been minimized.

@sarjon

sarjon Oct 11, 2018

Member

or DelegatingCacheClearer?

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

I like the CacheClearerChain of @PierreRambaud as it carries the fact that it actually chains the process and calls other clearers behind

This comment has been minimized.

@sarjon

sarjon Oct 15, 2018

Member

yes! i think ill use this name. :)

{
$this->cacheClearers = $cacheClearers;
}

This comment has been minimized.

@jolelievre

jolelievre Oct 2, 2018

Contributor

I would add a addCacheClearer method which can be convenient if someone needs to extend this service

This comment has been minimized.

@sarjon

sarjon Oct 3, 2018

Member

hmm, this would require updating interface which is not relevant for other clearers. :/

This comment has been minimized.

@jolelievre

jolelievre Oct 3, 2018

Contributor

I thought about adding it just to this class, not the Interface It is specific to this class because it is a composition of CacheClearerInterface

@@ -10,3 +10,12 @@ services:
- { name: cache.pool, clearer: cache.default_clearer }
calls:
- ['setLogger', ['@logger']]
prestashop.core.cache.clearer.entire_cache_clearer:

This comment has been minimized.

@jolelievre

jolelievre Oct 2, 2018

Contributor

although I understand the purpose of the service (and it is a good idea), I am not sure about the entire naming..
I thought about global instead, but maybe it can be confusing as it is way too large

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 17, 2018

ping @jolelievre for another review ^^

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Oct 17, 2018

@sarjon nice work! ;)

@matks

matks approved these changes Oct 17, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 17, 2018

@matks

This comment has been minimized.

Contributor

matks commented Oct 17, 2018

Thank you @sarjon

@matks matks added this to the 1.7.6.0 milestone Oct 17, 2018

@matks matks merged commit c07596f into PrestaShop:develop Oct 17, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment