🇳🇱Netherlands @kevin.brocatus

kevin.brocatus → created an issue.

Ran PHPCS and no issues were found. Also went through the code and I have to say the code looks very clean and well-written. I have a few comments.

First of all, my IDE (PHPStorm in this case) does not like consecutive dots in comments in the TypedResourceObjectAutocompleteWidget class. I don't necessarily think it's an issue, but might be worth changing.

In the class doc of TypedResourceObjectStringFormatter, I think you want to change the following;

Plugin implementation of a 'basic_string' formatter.

to

Plugin implementation of a 'typed_resource_object_string' formatter.

I also see that you define your return types in every class except for the TypedResourceObjectStringFormatter class. I recommend adding those for consistency.

You are also missing the return type of the create function in the TypedResourceObjectStringFormatter class.

A quick run of phpcs --standard=Drupal,DrupalPractice seems to be all good!

Looking through the code in detail I do not see any issues either. Only thing I found was an unnecessary blank line in the form() function in the OptionsGridWidget.php;

    elseif ($property_selected === 'flex') {

      $field_widget_complete_form['#attributes']['data-flex-size'] = (int) $this->getSetting('size');
    }

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();

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

Thanks for reviewing @Willempje2. As far as I am concerned your suspicion about the jQuery .val() shouldn't case any security issues.

1. The value that is being set is put into a value attribute, not directly into the DOM.
2. The value that is being set comes directly from another select list which has been sanitized by Drupal.
3. Sanitizing the value could actually cause issues as <div>value</div> is a valid select key.

It looks like the administer openlayers6 configuration doesn't get used anywhere. Either remove it, or use it accordingly.

The constructor of the OpenlayersBlock has a wrong doc message.

Constructs a Drupalist object. should be changed to Constructs a OpenlayersBlock object.

The constructor of the OpenlayerController has a wrong doc message.

The controller constructor. should be changed to Constructs a OpenlayerController object.

The build function of the OpenlayerController does not have the parameter or the return type defined in the doc message.

  /**
   * Builds the response.
   */
  public function build(Node $node, $display = 'teaser') {

should be changed to something like

  /**
   * Builds the response.
   *
   * @param \Drupal\node\NodeInterface $node
   *   The node.
   * @param string
   *   (Optional) The display mode. Default is set to 'teaser'.
   *
   * @return \Symfony\Component\HttpFoundation\JsonResponse
   *   The json response.
   */
  public function build(Node $node, $display = 'teaser') {

This has already been solved in the 1.0.x. branch. I'm currently waiting for the security advisory process to make a full stable release.

I appreciate the feedback @vinaymahale! I have fixed the characters limit in the README file too.

kevin.brocatus → created an issue.