[10.0.x] Custom Configuration

I am closing this application, since the person who created it does not seem to follow it.
I also asked which branch should be reviewed, but I did not get replies.

Hi apaderno,

Sorry for the delayed reply.
Please find the branch name: 10.0.x

Please review and grant the security advisory policy.

• 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/Form/ConfigurationForm.php

  /**
   * {@inheritdoc}
   */
  public function __construct(ConfigurationHelper $configHelper) {

{@inheritdoc} is not used in documentation comments for constructors, for which the description must start with Constructs a new followed by the class name (including its namespace), and end with object. Documentation comments for constructors must also describe the accepted parameters.

    if (!empty($machineName)) {
      $markup = $this->t('<div class="error_message custom-configuration_message"><font color="green">Machine name <strong><i>@name</i></strong>.</font></div>', [
        '@name' => $machineName,
      ]);
    }

The placeholder to render the string in <em> tags is @name.
HTML markup should be avoided as much as possible in strings passed to t() or $this->t().

    $return = $this->configHelper->createConfiguration($post);
    $this->messenger()->addMessage($this->t('@message', [
      '@message' => $return['message'],
    ]), $return['status']);

A string that contains only a placeholder is not translated. The output of $this->t('@message', ['@message' => 'Hello']) is not 'Ciao' when Italian is the selected language for the site; it is 'Hello' for every language.

src/Helper/ConfigurationHelper.php

  public function __construct(Connection $connection,
  ModuleHandler $moduleHandler,
  LanguageManager $languageManager,
  Container $serviceContainer) {

method declarations are written in a single line.

  /**
   * Create machine name. Replace all characters except alpha & number.
   *
   * @param string $name
   *   Name will check and replace the string.
   *
   * @return string
   *   It will return the machine name.
   */
  public function createMachineName($name) {

Verbs used in the description must be declined to the third person singular. A definite article is missing before machine name.
Return value description must not starts with It will return, Returns, nor Return.

  /**
   * Return message for the error code.
   *
   * @param int $code
   *   Error code.
   */
  public function getMessageByCode($code) {

The description for the return value is missing.

   * @return bool|array
   *   It will return the configuration value.
   */
  public function getValues($machine_name = NULL, $langCode = NULL, $domainKey = NULL) {

The method does not return a Boolean value; it returns NULL. The description should say when NULL is returned.

Why is a custom database table used for configuration values, when Drupal has an API to store configuration values?

@apaderno I am looking the points raised by you.

@jeetmail72, Also please make the 10.0.x branch as default branch. So it will be easy to review. Other reviewers will be confused about which branch needs to be reviewed.

@vinaymahale I set the latest as the default branch.
@apaderno I fixed the issues as you mentioned in the comments. I used the custom table to keep the configuration independent but in the future, I will use Config Entity instead of the custom table.

Looks good now! Let's wait for other reviewers!

Automated Review

FILE: /var/www/html/web/modules/contrib/custom_configuration/src/Helper/ConfigurationHelper.php
------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------
 7 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Language\LanguageManager.
------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/contrib/custom_configuration/src/Form/CustomConfigurationList.php
------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------
 8 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\custom_configuration\Helper\ConfigurationHelper.
------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/contrib/custom_configuration/src/Form/EditConfigurationForm.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
 9 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Database\Connection.
-------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/contrib/custom_configuration/src/Form/DeleteConfigurationForm.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
 8 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Database\Connection.
-------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------

Manual Review

I am changing priority as per Issue priorities → .

I removed the deprecated function db_field_exists and fixed Drupal coding issues

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.

Thank you so much @apaderno

We do not close issues, since they are automatically closed after 14 days.

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