[D9] Block ID

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

Since the project is being used for this application, for the time this application is open, only the user who created the application can commit code.

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• 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

function W3S_validation(&$form, FormStateInterface $form_state){
  // Block ID field value fatch.
  $inputs = $form_state->getUserInput()['third_party_settings']['block_id']['id'];
  // get the id of the block being submitted
  $self = isset($form['id']['#default_value']) ? $form['id']['#default_value'] : null;
  if(!empty($inputs)){
    // Checking id has not no space and special characters.
    if ( preg_match('/[^a-zA-Z_\-0-9]/i', $inputs) ){
      $form_state->setErrorByName('third_party_settings', t('Attribute ID must be unique, must not contain any space characters & must contain at least one character. Underscore (_) can be used.'));
    }
    // block list those using block id field.
    $block_ids = \Drupal::entityQuery('block')->condition('third_party_settings', '')->execute();
    // remove the block currently being submitted from the list of existing blocks so the submitted ID isn't checked as an external block ID
    if ($self) { unset($block_ids[$self]); }
    // Getting ID's already inserted in another block.
    foreach($block_ids as $ids){
      $block = Block::load($ids);
      if($block->getThirdPartySetting('block_id', 'id') == $inputs){
        $form_state->setErrorByName('third_party_settings', t('Attribute ID must be unique. This ID has added in another block.'));
      }
    }
  }
}

Function names must be prefixed by the module machine name.
The code doesn't follow the Drupal coding standards. In particular, there aren't spaces where the coding standards expect them.

/**
 * Implement w3 standards validation alter.
*/

The verb in the description should use the third person singular. The documentation comment should describe the function parameters and the value returned from the function.

Since there are similar modules, the project page should make clear what the difference between this module and those modules is.

Hi @heni_deepak,
I run Drupal coding standard , You can review and fix.

$ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml web/modules/contrib/block_id

FILE: C:\Ampps\www\my_coding_review\web\modules\contrib\block_id\block_id.module
------------------------------------------------------------------------------------------------------------------------------
FOUND 26 ERRORS AND 1 WARNING AFFECTING 13 LINES
------------------------------------------------------------------------------------------------------------------------------
  1 | ERROR   | [x] End of line character is invalid; expected "\n" but found "\r\n"
 55 | ERROR   | [x] Inline comments must start with a capital letter
 63 | ERROR   | [ ] Invalid function name, expected block_id_w3_s_validation but found block_id_W3S_validation
 63 | ERROR   | [x] Expected 1 space before opening brace; found 0
 66 | ERROR   | [x] Inline comments must start with a capital letter
 66 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 67 | ERROR   | [x] Use null coalesce operator instead of ternary operator.
 67 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected "NULL" but found "null"
 68 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 68 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 70 | ERROR   | [x] There should be no white space after an opening "("
 70 | ERROR   | [x] There should be no white space before a closing ")"
 70 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 73 | ERROR   | [x] Inline comments must start with a capital letter
 75 | WARNING | [ ] Line exceeds 80 characters; contains 140 characters
 75 | ERROR   | [x] Inline comments must start with a capital letter
 75 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 76 | ERROR   | [x] Newline required after opening brace
 76 | ERROR   | [x] There should be no white space after an opening "{"
 76 | ERROR   | [x] There should be no white space before a closing "}"
 76 | ERROR   | [x] Closing brace must be on a line by itself
 78 | ERROR   | [x] Expected 1 space after FOREACH keyword; 0 found
 78 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 80 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 80 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 95 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
 95 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 5

/**
 * Form validate.
 */
function block_id_form_block_form_validate(&$form, FormStateInterface $form_state) {
  
  // If the block id value empty, skip that.
  if(empty($form_state->getUserInput()['third_party_settings']['block_id']['id'])) {
    return FALSE;
  }
  
  // Get the id to be used.
  $inputs_id = $form_state->getUserInput()['third_party_settings']['block_id']['id'];

  // Block list those using block id field.
  $block_ids = \Drupal::entityQuery('block')->condition('third_party_settings', '')->execute();

  if(empty($block_ids)) {
    return FALSE;
  }

  foreach($block_ids as $ids){
    
    $block = Block::load($ids);

    // If this $inputs_id is present in the array means that the
    // $inputs_id is already in use in another block. Send a message for
    // the user.
    if($block->getThirdPartySetting('block_id', 'id') == $inputs_id){

      // Trigger the form set error with this message.
      $form_state->setErrorByName('third_party_settings][block_id][id', t('This ID: @inputs_id@ is already in use by another block.', [
        '@inputs_id@' => $inputs_id
      ]));

      // Skip the validation.
      return FALSE;
    }

  }

}

The documentation comment needs to describe the parameters and the return value (if the function returns a value).
A validation handler isn't expected to return values, not even Boolean values. In particular, returning FALSE after setting an error message doesn't skip the validation. The following code can be removed.

      // Skip the validation.
      return FALSE;

There are still missing spaces, for example after if or foreach, or before {. Verify also there aren't spaces before the end of a line.

The placeholders for t() are like @inputs_id, not @inputs_id@.

  if(empty($block_ids)) {
    return FALSE;
  }

FALSE returned from a validation handler doesn't mean anything for Drupal. If the code expects to cause Drupal to skip the validation, the code is wrong. Does the code really need to avoid Drupal validates the values submitted from users? If the answer is yes, then the code needs to be changed. Otherwise, the code could be simply changed as follows.

  if (empty($block_ids)) {
    foreach ($block_ids as $ids) {
      $block = Block::load($ids);

      // If $inputs_id is present in the array, it means it is already used by another block. Show a validation error.
      if ($block->getThirdPartySetting('block_id', 'id') == $inputs_id) {
        $form_state->setErrorByName('third_party_settings][block_id][id', t('@block_id is already used by another block.', [
          '@block_id' => $inputs_id
        ]));
    }
  }

I used @block_id to make clear to the translator users which ID is that.

    // Checking block validation.
    $form['#validate'][] = 'block_id_form_block_form_validate';

That comment isn't necessary, since the line following it is clear.

  $inputs_id = $form_state->getUserInput()['third_party_settings']['block_id']['id'];

The submitted values are available with $form_state->getValues(), which sanitizes the submitted values before returning them.

I have tested branch 2.0.x and it looks fine for me.

git clone --branch '2.0.x' https://git.drupalcode.org/project/block_id.git

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

That line needs to be changed, since core_version_requirement key requires Drupal 8.7.7, while semantic versioning requires Drupal 8.8.3. Drupal releases before Drupal 8.7.7 wouldn't install the module, since would not find any information about the required Drupal core release.

$block_ids = \Drupal::entityQuery('block')->condition('third_party_settings', '')->execute();

That line is returning all the Block entities for which the third_party_settings property is an empty string, but the property contains an array. Would not loading all the Block entities and verifying none of the existing entities use the block ID set for the currently edited block be better?

Just load the blocks without using any condition on the third_party_settings value, for example using Drupal::entityTypeManager()->getStorage('block')->loadMultiple(), and then check the value returned by $block->getThirdPartySetting().

Just a side note before I do a full review: a validation handler doesn't need to return anything. Instead of return FALSE; you can use return;.

The module dependencies should be namespaced; instead of block, it should be drupal:block, since the Block module is part of Drupal core.

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