[1.x] Field Label Override

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

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?

Hello @vishal.kadam, thank you for your quick reply.

I agree, the module is quite simple/small, but mainly because the purpose of it it's pretty simple too: allowing administrators to configure different labels for fields per entity view mode.

I have some other contributions, which you can see on my Drupal.org profile, but really the vast majority of my code work as a Drupal developer and solutions lead is client work, which I acknowledge you don't have access to view. I can send the link of those sites, if that works, but I'm not sure if this is of any help.

I'd be honoured if I was granted the ability of opting projects into security advisory coverage, but honestly, that's not my main purpose at this moment, and I'm fine with opening a new issue like this in the future for any new module I eventually contribute. All I'm looking for at this moment is having this project Field Label Override covered.

Please let me know what we can do to achieve this.

Thank you very much.

The purpose of these applications is to review a project to understand the applicant's comprehension of writing secure code that adheres to Drupal coding standards and utilizes the Drupal API correctly.

With only a small amount of code, we cannot assess your skills as a developer.

This application can only proceed with another project that contains sufficient PHP code.

Judging the code complexity can be a subjective activity. (It means that there could be different opinions about what is acceptable; I could even seem to have two different opinions for projects that seem to use code with similar complexity.)

I usually check how much code is boilerplate code and how much hooks are implemented. The maximum number of lines a PHP code has is relatively important; it would be relevant if PHP filed would not contain more than 15 lines, especially when most of them were comments or boilerplate code, for example. (Please, do not take that value are an absolute value; it is just an example value I use in this comment to show a this project cannot be used for these applications situation.)

In this case, there are three hook implementations and a submission handler; the simpler hook implementation is field_label_override_help(), but since that usually returns a short module description, I would never expect it contains complex code. IMO, the project is acceptable.

To the reviewers: Please, do not make a review basing on what PHP_CodeSniffer, PHPStan, or similar tools, report. Reported "issues" should be noticeable by a manual review.

• 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

README.md

The module uses a README.md file instead of a README.txt file. While the Drupal coding standards have not been yet updated about that, the Drupal.org community consider that positive. 👍
Since there is a README.md file, that should follow the content and formatting described in README.md template → .

composer.json

  "require": {
    "php": ">=8.0"
  }

Those lines are not necessary. Since the module already requires Drupal 10 or Drupal 11, the minimum PHP version acceptable for the module is at least PHP 8.1, required from Drupal 10.

field_label_override.libraries.yml

version: VERSION

That line is used only by Drupal core modules. Contributed modules should use a literal string that does not change with the Drupal core version a site is using.

field_label_override.module

/**
 * @file
 * Allows administrators to override a field label per entity view mode.
 */

The usual description for a .module file is Hook implementations for the [module name] module. where [module name] is the name of the module reported in its .info file.

/**
 * Implements hook_form_FORM_ID_alter().
 */

The description for that hook should also say for which form that hook is implemented, either by indicating that with the name of the class that implements the form (namespace included) or the form ID (which is usually indicated by getFormId()). (I saw Drupal core code following those methods to indicate the form targeted by those hooks.)

        $options_override = [
          'above_overridden' => new TranslatableMarkup('Above (Overridden)'),
          'inline_overridden' => new TranslatableMarkup('Inline (Overridden)'),
          'visually_hidden_overridden' => new TranslatableMarkup('- Visually Hidden (Overridden) -'),
        ];

Since the module code is already using t(), it would be better to always use t() for translatable strings. That function has not been yet deprecated, and there is not any pro in directly instantiating a TranslatableMarkup instance in procedural code. The following lines, found on t() documentation are more for code used in classes, especially for classes where the dependency injection container is available.

When possible, use the \Drupal\Core\StringTranslation\StringTranslationTrait $this->t(). Otherwise create a new \Drupal\Core\StringTranslation\TranslatableMarkup object directly.

        if (isset($field_settings['label']['#default_value'])
          && in_array($field_settings['label']['#default_value'], array_keys($options_override))
        ) {

As per Drupal coding standards, conditions should not be wrapped into multiple lines, especially when the code is easier to understand and read.

            $form_element = [
              '#type' => 'container',
              'label_override' => [
                '#type' => 'textfield',
                '#title' => t('Override label'),
                '#default_value' => $label_override ?? NULL,
                '#size' => 20,
                '#maxlength' => 255,
              ],
              'preserve_label' => [
                '#type' => 'checkbox',
                '#title' => t('Preserve original label'),
                '#default_value' => $preserve_label ?? FALSE,
              ],
            ];

Since $label_override has been already initialized, it is not necessary to use the null coalescing operator; the short ternary operator (?:) is more appropriate in this case.

            $form['fields'][$field_name] = array_slice($form['fields'][$field_name], 0, $label_index + 1, TRUE) +
            ['label_override_container' => $form_element] +
            array_slice($form['fields'][$field_name], $label_index + 1, count($form['fields'][$field_name]) - 1, TRUE);

Those lines are good to be written on more lines. 👍
It should be clear the second and third lines are a continuation of the first line, though. It is sufficient to correctly indent the second and third line.

        } else {
          // Add field name to array containing non-ovverridden fields.
          $no_overridden_fields[] = $field_name;
        }

This needs a small change: As per Drupal coding standards, the else part needs to go in a new line.

/**
 * Custom form submit to handle label override settings.
 *
 * @param array $form
 *   Form array.
 * @param FormStateInterface $form_state
 *   Form state object.
 */
function field_label_override_form_submit(array &$form, FormStateInterface $form_state) {

For form submission handlers, the documentation comment just say that is a form submission handler (not a form submit handler) and to which form is added (by the form class name or the form ID).

          $display_components[$field_name]['third_party_settings']['field_label_override'] = [
            'label_override' => $field_config['label_override_container']['label_override'] ?? NULL,
            'preserve_label' => !empty($field_config['label_override_container']['preserve_label']) ?? FALSE,
          ];

The null coalescing operator should be replaced by the short ternary operator.

        else if (isset($display_components[$field_name]['third_party_settings']['field_label_override'])) {
          unset($display_components[$field_name]['third_party_settings']['field_label_override']);
          $entity_view_display->setComponent($field_name, $display_components[$field_name]);
        }

This is another small change: As per Drupal coding standards, elseif needs to be used.

config/schema/field_label_override.entity_display.schema.yml

Every module should define the schema for the configuration object they use. 👍

@apaderno I really appreciate you taking this module into consideration, and even more the extensive review you reported in #7.

I'll carefully review your comments, and apply the necessary adjustments to the module. I'll keep you posted in this thread when I would expect you to review the adjusted code.

Once again, thank you!

@apaderno I applied the adjustments you recommended. They haven't been merged to the main branch yet, nor I have created a new release of the module. These adjustments are in the following merge request:

https://git.drupalcode.org/project/field_label_override/-/merge_requests...

Could you please review it and confirm the adjustments comply to the Drupal coding standards, and they should suffice? Then I'll merge the changes and create a new release of the module, which we can hopefully mark as covered by Drupal's security advisory policy.

Thank you.

The reported changes need to be committed in the project repository. We do not review merge requests.

@vishal.kadam Thank you for the update, I wasn't familiar with the review process.

The changes have been committed into the main branch of the project:
https://git.drupalcode.org/project/field_label_override

Thank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.

These are some recommended readings to help you with maintainership:

• Dries → ' post on Responsible maintainers
• Maintainership →
• Git version control system →
• Issue procedures and etiquette →
• Maintaining and responding to issues for a project →
• Release naming conventions → .

You can find more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thank you 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 also the dedicated reviewers as well.

Thank you @apaderno and @vishal.kadam for all your support with this. I really appreciate it.

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