[1.0.x] Views Filter Last Delta

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.

1. Fix phpcs issues. You can use the PHPCS tool for checking and resolving issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml vfld/

FILE: vfld/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
  5 | WARNING | Line exceeds 80 characters; contains 267 characters
 12 | WARNING | Line exceeds 80 characters; contains 135 characters
 19 | WARNING | Line exceeds 80 characters; contains 81 characters
 24 | WARNING | Line exceeds 80 characters; contains 159 characters
 36 | WARNING | Line exceeds 80 characters; contains 96 characters
----------------------------------------------------------------------


FILE: vfld/src/Plugin/views/filter/LastDeltaFilter.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 33 | ERROR | Class property $always_required should use lowerCamel naming without underscores
--------------------------------------------------------------------------------

2. FILE: vfld.info.yml

core_version_requirement: ^8 || ^9 || ^10

Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.

@seogow

Usually, after reviewing a project, we allow the developer to opt projects into security advisory coverage.

This project is too small for us and it doesn't contain enough PHP code to really assess your skills as a developer.

Have you made any other contributions that we could instead review?

seogow already commented about $always_required. If that line needs to be changed, that is not because phpcs says snake case should not be used to name class properties.

Also, since seogow already used phpcs, it would be better to make a review that does not involve phpcs, at least in the case phpcs just reports too-long lines in a README file and a line for which the OP already commented. (Truly, the issue in that line is not how the variable name is spelled.)

I am going to accept this application. There is an issue in the code and I would like to see how seogow is going to fix that.

@vishal.kadam - You are right, I have fixed the vfld.info.yml file. I have also fixed the line lengths in the README.md. You can check my other contribution e.g. there: https://www.drupal.org/project/mail_safety/issues/3267796 🐛 No mail with symfony_mailer Needs work .

@apaderno - I do not see any issue with that variable per se - this is a Boolean filter and its value is either Set (filter) or Not set (do not filter), hence I force it not to be optional ("Not set" equals to that and it is far less confusing). However, my PHP Documentation should follow the inheritance. Fixed that.

Thank you both very much for your time and effort!

@seogow,

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.

Thanks

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/Plugin/views/filter/LastDeltaFilter.php

  /**
   * {@inheritdoc}
   */
  protected $alwaysMultiple = TRUE;

  /**
   * {@inheritdoc}
   */
  public $always_required = TRUE;

Properties declared from the parent class do not need to be re-declared.

  /**
   * Constructs a new LastDeltaFilter.

The class needs to include its namespace. Also, it is an instance/object that is created.

    $data = Views::viewsData()->get($this->table);
    $join_info = $data['table']['join'][$query_base_table];
    $query = "(($field IS NULL) OR ($field IN (SELECT MAX($field) FROM $this->tableAlias WHERE {$join_info['table']}.{$join_info['field']} = $query_base_table.{$keys['id']})))";

Field and table names must be escaped, when building a database query as that code does. The Drupal\Core\Database\Connection class has methods to do that.

@vishal.kadam, thank you, highly appreciated!

@apaderno,

See detailed answer below, but thank you for the security catch! Normally I would not deploy again until you are completely happy with the code, but since there was a security issue, I have done that immediately.

Query bugfix:

/**
   * {@inheritdoc}
   *
   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   * @throws \Exception
   */
  public function query() {
    // If filtering is disabled, do nothing.
    if (!$this->value) {
      return;
    }

    $this->ensureMyTable();
    $database = \Drupal::database();
    $field = $database->escapeField("$this->tableAlias.delta");
    $query_base_table = $this->relationship ?: $this->view->storage->get('base_table');
    $entity_type = $this->entityTypeManager->getDefinition($this->getEntityType());
    $keys = $entity_type->getKeys();
    $data = Views::viewsData()->get($this->table);
    $join_info = $data['table']['join'][$query_base_table];

    $join_info_table = $database->escapeTable($join_info['table']);
    $join_info_field = $database->escapeField($join_info['field']);
    $query_base_table_id = $database->escapeField($query_base_table . '.' . $keys['id']);

    $query = "(($field IS NULL) OR ($field IN (SELECT MAX($field) FROM $this->tableAlias WHERE $join_info_table.$join_info_field = $query_base_table_id)))";
    $this->query->addWhereExpression($this->options['group'], $query);
  }

Additional Documentation fixes deployed:

Class:

/**
 * Filter to handle last delta value.
 *
 * @ingroup views_filter_handlers
 *
 * @ViewsFilter("vfld")
 *
 * @package Drupal\vfld\Plugin\views\filter
 */

constructor:

/**
   * Constructs a new instance of LastDeltaFilter.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   Entity Type Manager Service.
   */

I do not see any issues with declaration of values I have done. The same approach to change the property value is used in Drupal Core by authors of the Views module themselves:

see: namespace Drupal\views\Plugin\views\filter;

However, I am happy, if you want me to (let me know), follow the classic software engineering practices by:

/**
   * Constructs a new LastDeltaFilter.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   Entity Type Manager Service.
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->entityTypeManager = $entity_type_manager;
    $this->alwaysMultiple = TRUE;
    $this->alwaysRequired = TRUE;
  }

Thank you for your feedback!

Manual Review

I was referring to the Constructs a new instance of LastDeltaFilter. line.

The Views module does not follow the Drupal coding standards very well and its classes cannot be used as example of what code following them should be written, like other Drupal core classes.
I recall the coding standards showed an example of comment for a class constructor, but I cannot find it. I will leave that as it, since without an explicit indication from the coding standards, any comment style is fine, and the trend on Drupal core cannot be used as standard.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find more contributors chatting on the Slack → #contribute channel. So, come hang out and stay involved → .
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

I thank all the reviewers.

@reszli, I agree that employing an exact match is always better. I'll make sure to update the code accordingly when implementing a functional test.

@apaderno, I sincerely appreciate all your insightful comments. They have been tremendously helpful.

@vishal.kadam, once again, thank you for your valuable input.

Automatically closed - issue fixed for 2 weeks with no activity.