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

Added support for automatic purging via cache tags #238

Open
wants to merge 16 commits into
base: 8.x-1.x
Choose a base branch
from

Conversation

stooit
Copy link
Contributor

@stooit stooit commented Apr 19, 2024

  • emits cache-keys header with appropriate cache tag values
  • added new purger plugins for purging via API based on path / everything / tag

Copy link
Contributor

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');
Copy link
Contributor

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.

@kepol kepol self-assigned this Jul 6, 2024
Copy link
Contributor

@kepol kepol left a 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.

modules/quant_purger/src/Entity/QuantPurgeSettings.php Outdated Show resolved Hide resolved
modules/quant_purger/src/Entity/QuantPurgeSettings.php Outdated Show resolved Hide resolved
Copy link
Contributor

@kepol kepol left a 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

@@ -0,0 +1,10 @@
quant_purge_header:
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor

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';
Copy link
Contributor

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';
Copy link
Contributor

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?

@kepol
Copy link
Contributor

kepol commented Jul 6, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants