[1.0.x] Cache tag expire

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

I ran PHPCS tests and found the following issues. I will do a more in-depth review later.

./vendor/bin/phpcs --standard=Drupal,DrupalPractice /ct_expire/

FILE: /usr/share/nginx/html/indicia/ct_expire/src/CtExpireScheduler.php
-----------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 10 LINES
-----------------------------------------------------------------------
  51 | ERROR | Missing short description in doc comment
  52 | ERROR | Missing parameter comment
  53 | ERROR | Missing parameter comment
  54 | ERROR | Missing parameter comment
  56 | ERROR | Description for the @return value is missing
 111 | ERROR | Missing short description in doc comment
 112 | ERROR | Description for the @return value is missing
 121 | ERROR | Missing short description in doc comment
 122 | ERROR | Missing parameter comment
 124 | ERROR | Description for the @return value is missing
-----------------------------------------------------------------------

Time: 66ms; Memory: 10MB

ct_expire.module

I've personally never seen the following method to provide an example of how to use your module. I would personally move this example to the README.md file and delete it from the .module file.

/**
 * Example using hook_ENTITY_TYPE_update().
 */
function _YOUR_MODULE_node_update(EntityInterface $entity) {
  $ct_expire = \Drupal::service('ct_expire.scheduler');
  $ct_expire->schedule('node:12', time(), 'test_cache_tag');
}

I also see that you use camel case and snake case naming conventions through eachother when it's not necessary. For example;

function ct_expire_cron() {
  // @todo Also create drush command for this.
  /** @var \Drupal\ct_expire\CtExpireScheduler $ct_expire */
  $ct_expire = \Drupal::service('ct_expire.scheduler');
  $cacheExpireTags = $ct_expire->getExpired();

I see that you wrote an implementation for the hook_entity_postsave. That is currently not an official Drupal hook and will only work by appling the the following patch Add hook_entity_postsave hook ✨ Add hook_entity_postsave hook Needs work . I am not familiar with this hook, but it sounds like this can be replaced by the hook_entity_insert and hook_entity_update hooks.

In the ct_expire_cron function you can remove the following empty check as the result of $ct_expire->getExpired(); will always be an array.

if (!empty($cacheExpireTags)) {

ct_expire.info.yml

Your field widget has a dependency on the datetime module. While I think you can use this module without it, I would probably recommend to add this dependency.

dependencies:
  - drupal:datetime

SettingsForm.php

I am really confused why this file is in here. In the buildForm there is an example textfield and a quick search through the code results that the ct_expire.settings is never used anywhere in the module. I assume based on your TODO that this will be for future use. For now, I would remove this from the codebase.

As a follow up on this; that means that the ct_expire.settings_form route can be removed from the routing file, meaning that the ct_expire.routing.yml file can also be removed. On top of that, the permission administer ct_expire configuration is only used in this routing file, meaning that this can also be removed until these features have been implemented.

CtExpireScheduler.php

In the getExpired() function, I would personally rewrite the query so you don't have to write the WHERE expression yourself. This is not required, just a preference.

From:

    return $this->connection->query(
          "SELECT id, cache_tag FROM {ct_expire_item} WHERE :time > expire",
          [':time' => $this->time->getCurrentTime()]
      )->fetchAll();

To:

    $query = $this->connection->select('ct_expire_item', 'ct_expire_item');
    $query->fields('ct_expire_item', ['id', 'cache_tag']);
    $query->condition('time', $this->time->getCurrentTime(), '>');
    return $query->fetchAll();

Moving to Needs work based on the above feedback

main is a wrong branch name, as branch names end with the literal .x. That branch needs to be removed.

Drupal core is moving to use a main branch, but for what I know, there are still issues to be resolved before such branch is completely usable. I would suggest to wait before starting to use it.

@kevin.brocatus thanks for the review!
@shashank5563 Did not know this was a requirement, thanks for letting me know!

PHPCS issues.
This is now updated.

Example method.
Removed, readme has some examples so it was already kinda unnecessary.

Camel case and snake case.
This is now updated.

hook_entity_postsave
This is now updated.

empty check as the result of $ct_expire->getExpired();
This is now updated.

SettingsForm.php
Unless this leads to a security issue i would prefer to let it in.
Like you guessed i would like to make use of this in the near future, seems silly to remove it now and add it back later.

getExpired() function
As far as i know using the full expression like this is faster.
Unless other arguments are given i prefer to keep it as is.

Branch
Default branch changed to 1.0.x

@Willempje2, I have reviewed the changes, and they look fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

I am changing priority as per Issue priorities.

Please fix the below PHPCS issues:

FILE: /ct_expire/ct_expire.module
--------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
--------------------------------------------------------------------------------------------------
 13 | WARNING | [x] Unused use statement
 35 | ERROR   | [x] Namespaced classes/interfaces/traits should be referenced with use statements
 36 | ERROR   | [x] Opening brace should be on the same line as the declaration
--------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------

Is there anything else that needs to be changed? PHP_CodeSniffer should be only used to report lines to change once per application; those changes are not application stoppers, then.

Apart from that there are no more issues
Changing to RTBC

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form['example'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Example'),
      '#default_value' => $this->config('ct_expire.settings')->get('example'),
    ];
    return parent::buildForm($form, $form_state);
  }

  /**
   * {@inheritdoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {
    if ($form_state->getValue('example') != 'example') {
      $form_state->setErrorByName('example', $this->t('The value is not correct.'));
    }
    parent::validateForm($form, $form_state);
  }

It is not clear what purpose would a form with a textfield where users are asked to enter example have. It seems example code, not code a "real" module would use.

Example form
This was also mentioned by kevin.brocatus but up until now i did not have the time to implement this fully.
1.0.x now includes a useful settings form.

DateTimeDefaultWidget
Base class is now extended instead of the internal class.

Priority change as per Issue priorities. →

Everything seems fine. Moving to RTBC.

ct_expire.module

            catch (Exception $e) {
              \Drupal::messenger()->addError($e->getMessage());
              continue;
            }

The first argument passed to addError() must be a translatable string.

  $config->get('disable_cron');

  if (!$config->get('disable_cron')) {

The first line is not necessary.

        if (!empty($field['type']) && $field['type'] === 'ct_expire_date_and_time_cache_expire') {
          $dateTimeZone = new \DateTimeZone(DateTimeItemInterface::STORAGE_TIMEZONE);

          $timeStamp = 0;
          if (!empty($date = $entity->get($fieldKey)->getString())) {
            try {
              $dateTime = new \DateTime($date, $dateTimeZone);
            }
            catch (Exception $e) {
              \Drupal::messenger()->addError($e->getMessage());
              continue;
            }

            $timeStamp = $dateTime->getTimestamp();
          }

          $id = $entity->id();
          $ctExpire = \Drupal::service('ct_expire.scheduler');
          $ctExpire->schedule(
            $entityType . ':' . $id,
            $timeStamp,
            $entityType . '_' . $id . '_' . $fieldKey
          );
        }
      }

I gather what the module is achieving, but:

• A field widget just changes the form elements used to gather input for a field; it does not change the purpose of the field. In this case, it is more appropriate to implement a different field type, or altogether a different solution.
• The quoted code does not even consider there could be more fields using that widget. The code should probably limit the number of fields that are used for caching purposes.

FILE: ct_expire.info.yml

package: Custom

That line is used by custom modules created for specific sites. It is not a package name used for projects hosted on drupal.org.

Translatable error message:
This has been updated.

Redundant config get:
This has been updated.

Package in info.yml:
This has been removed.

Wrong use of field widget:
I understand the issue, but I'm struggling to find an appropriate place for this change and/or a practical solution for implementing it elsewhere. It doesn't affect the output of the field, and I don't want to change the type because using the widget allows implementation on existing fields.

Consider using multiple fields:
Unfortunately, I don't quite understand this comment. The 'hook_entity_update' loops over all fields using this widget and schedules separate expiration items. Could you elaborate on this?

If the field purpose is changed, that is not a matter of implementing a different field widget, which is just for changing how the value for that field is entered. It needs to be a different field type, in the same way there is an email field type and a string field type.

The module implements a widget type, which could be used for different entity fields (as long as their type is datetime). It does not seem the module keeps that in consideration; it schedules for expiring the same tag (which is just $entityType . ':' . $id) multiple times.

I understand, and thank you for the clarification. I genuinely hope that this doesn't become a deal-breaker for the module. For us, the practical benefit of changing the widget instead of the field type outweighs the downside of misusing the widget in a way that alters the output.

Regarding the second point:
I don't believe it's possible to specify the cache tag any further. If it is, I'm not aware of what difference it would make during invalidation. (Unless cache tags can also accept display types, but I'm not familiar with such a feature if it exists.)
If multiple fields of the same type have this widget, the value of the datetime field would likely be different, resulting in invalidation at different times.

It would be like if in Drupal, to create an email field, administrator users should first create a plain text field and then select Email as widget for that field. There must be a reason why Drupal has the following fields types, instead of having a single field type with different widget types.

Implementing a widget type in this case is a misuse of the Drupal API.
The other problem is that the widget is not chosen when the field is created, but after the field is created, and from the Manage form display tab, which probably does not allow to change widget for fields which has already content. This means that when administrator users create a field, and decide the field type, they do not know which widget they are allowed to choose for that field, and there is no indication of which one is the correct field to create a "cache tag expire" field.

Yes, the invalidation would be done at different times, but for the same tag, whose value is calculated as $entityType . ':' . $id, and therefore is the same for every field using that widget an entity has. Maybe that is wanted, but it seems inefficient to invalidate a cache tag once and (for example) again after 2 minutes simply because there are two fields using that widget set to two minutes of difference from each other.

I still agree that using the Drupal widget in this way is a form of misuse, although perhaps not to the extent you have in mind (or maybe I'm overlooking something, in which case I sincerely apologize).

Allow me to describe a use case for when we use this module:
Consider a content/node type named "festival event." These festival events have various fields, including a datetime field for the start of the event and another for the end of the event (which could be days or hours apart). On the website, we aim to highlight events that are currently ongoing within a view. This is achieved by adding a class based on a set of "if" statements. Unfortunately, these if statements are never checked once the page is stored in the cache.

By applying this widget to the existing "festival event start date" field, the page cache automatically invalidates once the start date has passed. This allows the if statements to be reevaluated.
Different fields utilizing this widget will indeed trigger multiple cache invalidations at different times, and this is intentional. (Note that not all dateTime fields necessarily need to utilize this widget.)

In this instance, I don't want to alter the input or the intended purpose (to some extent) of this field, and I also don't want to directly modify the output.

Hope this helps.

The only purpose of a field widget is rendering the form elements shown when an entity is edited; using a field widget for other purposes is a Drupal API misuse, especially because that is not the correct way to achieve the module's purpose, and there are alternatives.
It is not that a module which needs that one of its methods (which does not change the form elements used to enter data for a field) is invoked when an entity is edited implements a field widget and then instructs administrator users to select that field widget in order for the module to work.

Drupal core does not use a field widget to be able to mark a node as published/unpublished; it has code to add the Published field to entities. The Content Moderation module does not implement a field type that must be used for the Published field to allow workflows to be used in entities; it adds its own fields to the entity using a hook.

Ah bummer... Thanks for the review though!