-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added support for automatic purging via cache tags #238
base: 8.x-1.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably delete this guy and at .DS_Store to the gitignore.
parent::__construct($configuration, $plugin_id, $plugin_definition); | ||
$this->settings = QuantPurgeSettings::load($this->getId()); | ||
// Note: We use the Quant HTTP client rather than the generic Guzzle client. | ||
$this->client = \Drupal::service('quant_api.client'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be injected with the create
method rather than accessing \Drupal::service
here.
Update doc block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be consistent.
Remove extraneous quote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't reviewed everything but there seems to be consistencies in "purge" vs "purger" and some probably should change
modules/quant_purger/config/schema/quant_purger.data_types.schema.yml
Outdated
Show resolved
Hide resolved
modules/quant_purger/src/Plugin/Purge/Purger/QuantPurgeBase.php
Outdated
Show resolved
Hide resolved
modules/quant_purger/src/Plugin/Purge/TagsHeader/QuantCacheTagsHeader.php
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,10 @@ | |||
quant_purge_header: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear what this is being used for
/** | ||
* Helper class that centralizes string hashing for security and maintenance. | ||
*/ | ||
class Hash { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make sense as a general utility instead of just in quant_purger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or not because it's dealing with cache tags?
* | ||
* @var string | ||
*/ | ||
public $invalidationtype = 'tag'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have 'path' listed so why is this just tag?
* {@inheritdoc} | ||
*/ | ||
public function getFormId() { | ||
return 'quant_purger.configuration_form'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be the same as the id for the queuer config form?
@stooit @steveworley There was a lot of extra/unnecessary complexity (e.g. abstract base classes) and naming confusions in the code because the queuer plugin was added first and called "Quant Purger" and then the actual purger plugin was added in this. I have tried to refactor things and not only clear up the confusion between these two plugins but also some other things while I saw them. Before I do a bunch of testing, please review the state of the PR to see if you notice anything I screwed up. |
cache-keys
header with appropriate cache tag values