[10.0.x] Custom Configuration

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.

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.

@jeetmail72 It seems you have missed working on the coding standards. 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 custom_configuration/

FILE: custom_configuration/custom_configuration.info.yml
----------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------
1 | WARNING | "core_version_requirement" property is missing in the info.yml file
----------------------------------------------------------------------------------------

FILE: custom_configuration/src/Form/ConfigurationSetting.php
--------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------
18 | ERROR | Missing member variable doc comment
--------------------------------------------------------------------------------------------

FILE: custom_configuration/src/Form/CustomConfigurationList.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
15 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
-----------------------------------------------------------------------------------------------------------------

FILE: custom_configuration/src/Form/DeleteConfiguration.php
-------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------
20 | ERROR | Missing member variable doc comment
21 | ERROR | Missing member variable doc comment
22 | ERROR | Missing member variable doc comment
-------------------------------------------------------------------------------------------

FILE: custom_configuration/src/Form/EditConfiguration.php
-------------------------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
-------------------------------------------------------------------------------------------------------------------------------------------
21 | ERROR | Missing member variable doc comment
22 | ERROR | Missing member variable doc comment
23 | ERROR | Missing member variable doc comment
73 | ERROR | unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.
-------------------------------------------------------------------------------------------------------------------------------------------

FILE: custom_configuration/src/Helper/ConfigurationHelper.php
--------------------------------------------------------------------------------------------------------------------------------------------
FOUND 13 ERRORS AFFECTING 13 LINES
--------------------------------------------------------------------------------------------------------------------------------------------
159 | ERROR | The array declaration extends to column 119 (the limit is 80). The array content should be split up over multiple lines
185 | ERROR | The array declaration extends to column 102 (the limit is 80). The array content should be split up over multiple lines
199 | ERROR | The array declaration extends to column 119 (the limit is 80). The array content should be split up over multiple lines
202 | ERROR | The array declaration extends to column 101 (the limit is 80). The array content should be split up over multiple lines
227 | ERROR | The array declaration extends to column 119 (the limit is 80). The array content should be split up over multiple lines
243 | ERROR | The array declaration extends to column 112 (the limit is 80). The array content should be split up over multiple lines
246 | ERROR | The array declaration extends to column 108 (the limit is 80). The array content should be split up over multiple lines
251 | ERROR | The array declaration extends to column 125 (the limit is 80). The array content should be split up over multiple lines
254 | ERROR | The array declaration extends to column 107 (the limit is 80). The array content should be split up over multiple lines
259 | ERROR | The array declaration extends to column 102 (the limit is 80). The array content should be split up over multiple lines
369 | ERROR | The array declaration extends to column 105 (the limit is 80). The array content should be split up over multiple lines
372 | ERROR | The array declaration extends to column 101 (the limit is 80). The array content should be split up over multiple lines
497 | ERROR | unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.
--------------------------------------------------------------------------------------------------------------------------------------------

• Remove README.txt file. Update README.md file according to https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or-distribution-project/documenting-your-project/readmemd-template
• drupal-10.0.0 is a wrong branch name, as branch names end with the literal .x
  See https://www.drupal.org/docs/develop/git/git-for-drupal-project-maintainers/release-naming-conventions#release-branches

I am crediting users from the other applications.

Neither drupal-10.0.0 nor drupal10 are correct branch names.
The 10.0.1 branch the title suggest to review does not exist. Which branch should be reviewed?

I am changing priority as per Issue priorities.

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.

• 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 on 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?

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

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Default Branch

Yes: Follows the guidelines for default branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

1. src/Form/DeleteConfigurationForm.php
   class DeleteConfigurationForm extends ConfigFormBase {
      /**
       * Database connection object.
       *
       * @var \Connection
       */
      protected $database;
   

   there's no such thing called \Connection in Drupal, should be \Drupal\Core\Database\Connection instead.

2. db_field_exists is used in custom_configuration.install, it is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Instead, get a database connection injected into your service from the container, get its schema driver, and call fieldExists() on it. For example, $injected_database->schema()->fieldExists($table, $field);
3. hook_help doesn't contain much info.
4. Why do we need README.txt, if there's already a README.md
5. README.md says, This is new module in Drupal8, this can be improved or can be removed completely.

This review uses the Project Application Review Template.

I am changing priority as per Issue priorities.

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.

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