[1.0.2] FIFO Lock (Ticket lock)

Thank you for applying!

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.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

Remember to change status, when the project is ready to be reviewed. In this queue, projects are only reviewed when the status is Needs review.

Solve following phpcs Error

phpcs --standard=Drupal,DrupalPractice web/modules/contrib/fifo_lock/

FILE: web/modules/contrib/fifo_lock/src/FIFODatabaseLock.php
------------------------------------------------------------------------------------------------
FOUND 42 ERRORS AND 3 WARNINGS AFFECTING 44 LINES
------------------------------------------------------------------------------------------------
11 | ERROR | [x] Missing class doc comment
12 | ERROR | [x] Opening brace should be on the same line as the declaration
25 | ERROR | [ ] Missing member variable doc comment
34 | ERROR | [x] Opening brace should be on the same line as the declaration
41 | ERROR | [ ] Missing short description in doc comment
42 | ERROR | [ ] Missing parameter comment
43 | ERROR | [ ] Missing parameter comment
44 | ERROR | [ ] Description for the @return value is missing
48 | ERROR | [x] Opening brace should be on the same line as the declaration
56 | ERROR | [x] Expected 1 space(s) after cast statement; 0 found
66 | ERROR | [x] Expected newline after closing brace
83 | ERROR | [x] Expected 1 space(s) after cast statement; 0 found
95 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "FALSE" but found "false"
111 | ERROR | [x] Expected newline after closing brace
115 | ERROR | [x] Expected newline after closing brace
122 | ERROR | [x] Expected newline after closing brace
127 | ERROR | [x] Expected newline after closing brace
136 | ERROR | [x] Expected 1 blank line after function; 2 found
139 | ERROR | [ ] Missing short description in doc comment
143 | ERROR | [x] Opening brace should be on the same line as the declaration
148 | ERROR | [x] Expected newline after closing brace
161 | ERROR | [x] Expected 1 space(s) after cast statement; 0 found
167 | WARNING | [x] There must be no blank line following an inline comment
167 | WARNING | [ ] There must be no blank line following an inline comment
169 | ERROR | [x] Expected 1 space(s) after cast statement; 0 found
178 | ERROR | [ ] Missing short description in doc comment
182 | ERROR | [x] Opening brace should be on the same line as the declaration
191 | ERROR | [x] Expected newline after closing brace
196 | ERROR | [x] Missing function doc comment
197 | ERROR | [x] Opening brace should be on the same line as the declaration
214 | ERROR | [x] Opening brace should be on the same line as the declaration
223 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 6
224 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 6
225 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 6
239 | ERROR | [ ] Missing parameter type
245 | ERROR | [x] Opening brace should be on the same line as the declaration
261 | ERROR | [x] Opening brace should be on the same line as the declaration
285 | ERROR | [x] Opening brace should be on the same line as the declaration
289 | ERROR | [x] Short array syntax must be used to define arrays
320 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
326 | ERROR | [x] Missing function doc comment
327 | ERROR | [x] Opening brace should be on the same line as the declaration
333 | ERROR | [x] Expected 1 space(s) after cast statement; 0 found
339 | ERROR | [x] Missing function doc comment
340 | ERROR | [x] Opening brace should be on the same line as the declaration
------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 36 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------

@Rushikesh Raval We don't review the application until the applicant changes the status to 'Needs review'.

1. FILE: fifo_lock.info.yml

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

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

2. FILE: fifo_lock.module

$dbResultArray = $database->query('SELECT SQL_NO_CACHE count(*) as `count` FROM {' . \Drupal\fifo_lock\FIFODatabaseLock::TABLE_NAME . '}', [])->fetchAssoc();

$database->query('TRUNCATE TABLE {' . \Drupal\fifo_lock\FIFODatabaseLock::TABLE_NAME . '}', []);

FILE: src/FIFODatabaseLock.php

$minIdResultArray = $this->database->query('SELECT SQL_NO_CACHE MIN(id) as `min_id` FROM {' . static::TABLE_NAME . '} WHERE name = :name', [':name' => $name])->fetchAssoc();

$firstResultArray = $this->database->query('SELECT SQL_NO_CACHE * FROM {' . static::TABLE_NAME . '} WHERE id = (SELECT MIN(id) as `min_id` FROM {' . static::TABLE_NAME . '} WHERE name = :name)', [':name' => $name])->fetchAssoc();

Pass variables using placeholders → .

Note to reviewers:
The main class (\Drupal\fifo_lock\FIFODatabaseLock) appears to be based on a copy of the Drupal Core DatabaseLockBackend class.

Care should be taken to only evaluate the portion of code written by the applicant that differ from Drupal Core.

• 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 does not correctly use the Drupal API; the single points are not ordered, not even by importance

src/FIFODatabaseLock.php

The LockBackendAbstract class is not part of the Drupal public API and should not be used as parent class for contributed classes.

  /**
   * Constructs a new DatabaseLockBackend.
   *
   * @param \Drupal\Core\Database\Connection $database
   *   The database connection.
   */
  public function __construct(Connection $database) {
    // __destruct() is causing problems with garbage collections, register a
    // shutdown function instead.
    drupal_register_shutdown_function([$this, 'releaseAll']);
    $this->database = $database;
  }

The documentation comment for constructors is not mandatory anymore. If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.

  /**
   * Get the earliest record ID for a given name.
   */
  private function getMinDbId($name) {

The description for methods needs to use verbs conjugated to the third person singular.
The parameter and the return value descriptions are missing.

  /**
   * Get the earliest record ID for a given name.
   */
  private function getMinDbId($name) {
    $minIdResultArray = $this->database->query('SELECT SQL_NO_CACHE MIN(id) as `min_id` FROM {' . static::TABLE_NAME . '} WHERE name = :name', [':name' => $name])->fetchAssoc();

    $minId = 0;

    if (!empty($minIdResultArray['min_id'])) {
      $minId = (int) $minIdResultArray['min_id'];
    }

    return $minId;
  }

  /**
   * Get the earliest record for a given name.
   */
  private function getEarliestRecord($name) {
    $firstResultArray = $this->database->query('SELECT SQL_NO_CACHE * FROM {' . static::TABLE_NAME . '} WHERE id = (SELECT MIN(id) as `min_id` FROM {' . static::TABLE_NAME . '} WHERE name = :name)', [':name' => $name])->fetchAssoc();
    return $firstResultArray;
  }

Why are the queries using SQL_NO_CACHE when Drupal core does not use them?

templates/fifo-lock.html.twig

Since the module does not output any page, it does not need template files. That template file does not even render anything, since it just contains a comment.

fifo_lock.module

The file does not respect the Drupal coding standards, in particular for the function declarations.

/**
 * Implements hook_theme().
 */
function fifo_lock_theme()
{
  return [
    'fifo_lock' => [
      'render element' => 'children',
    ],
  ];
}

Since the module does not render any page, it does not need to implement hook_theme().

I am changing priority as per Issue priorities → .

This thread has been idle, in the needs work state with no activity for several months. Therefore, I am assuming that you are no longer pursuing this application. If you are no longer pursuing this application then I mark it as Closed (won't fix).

If this is incorrect, and you are still pursuing this application, then please feel free to set the issue status to Needs work or Needs review, depending on the current status of your code.

This thread has been idle, in the Needs work state with no activity for several months. Therefore, I am assuming that you are no longer pursuing this application, and I marked it as Closed (won't fix).

If this is incorrect, and you are still pursuing this application, then please feel free to re-open it and set the issue status to Needs work or Needs review, depending on the current status of your code.

Let's keep this application open for two more months.