[1.0.x] Field extra

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

Release branch names always end with the literal .x as described in Release branches. The only exception is for the main branch, which is actually not fully supported on drupal.org and should be avoided.

Hi @vishal.kadam,

thanks for your feedback. Changed the default branch to .x

Hi @AstonVictor, here are some tips from me:

• In all php files (.module and .install files too) it's good practice to type function arguments and return types
• .module - In all functions there should be both - information about implemented hook and short function description what it does, separated by single empty line of comment
• You should check all php files with proper configured phpStan. Eg. in .module:151 $entity->id() does not always return int, sometimes its even null, then it will break your page, so please check your module with phpStan more info about installation and use here https://www.drupal.org/docs/develop/development-tools/phpstan →
• .routing.yml - _admin_route option is not necessary cause everything after /admin is already using administration page
• composer.json - worth to add what php version your module support
• FieldExtraForm.php:72 - for better styling it's nice to write this if() like this
  

        if (
          in_array('Drupal\Core\Entity\FieldableEntityInterface', class_implements($definition->getOriginalClass())) &&
          in_array('Drupal\user\EntityOwnerInterface', class_implements($definition->getOriginalClass()))
        ) {
          $options[$definition->id()] = $definition->getLabel()->render();
        }

PhpCs is clear, everything looks fine for me, i tested module and works as designed, good work! :D

Fixed all issues from the comment above. All changes were merged into 1.0.x branch.
thanks.

Other findings to fix-

FILE: field_extra/field_extra.module
---------------------------------------------------------------------------------------------------------
FOUND 15 ERRORS AFFECTING 5 LINES
---------------------------------------------------------------------------------------------------------
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
1 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be referenced with use statements
63 | ERROR | [ ] If there is no return value for a function, there must not be a @return tag.
63 | ERROR | [ ] Description for the @return value is missing
65 | ERROR | [ ] Type hint "array" missing for $form
129 | ERROR | [ ] If there is no return value for a function, there must not be a @return tag.
129 | ERROR | [ ] Description for the @return value is missing
131 | ERROR | [ ] Type hint "array" missing for $form
---------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------
Time: 67ms; Memory: 8MB

I removed @return void lines and added array before $form elements.

but I have no idea what 'Non-namespaced classes/interfaces/traits should not be referenced with use statements' means. I added all classes via use after the @file comment as it is for all core/contrib modules. Could you provide an example how it should be fixed?

thanks

Yes, I have used Drupal library for testing and the actual errors were from line no. 63-131 as mentioned which you tried to fix.

Ignore this 'Non-namespaced classes/interfaces/traits should not be referenced with use statements' as no coding standards fault.

Running phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml on the directory containing the module files, I do not get any warning/error.

Thank you for applying!
I have some suggestions:

contrib/field_extra/field_extra.module

  if (strpos($name, 'private_') !== FALSE) {}

I think you can use str_containsinstead of strpos

  if (str_contains($name, 'private_')) {}

Reverse the order of tests

  if ($field->isDisplayConfigurable('form') &&
      $field instanceof FieldConfigInterface &&
      $field->getThirdPartySetting('field_extra', 'private_field'))

it's good to check the $field is an FieldConfigInterface before using it

  if ($field instanceof FieldConfigInterface &&
      $field->isDisplayConfigurable('form') &&
      $field->getThirdPartySetting('field_extra', 'private_field'))

Remove unused parameters (Not mandatory but nice to have)

Unused parameter $route_match :
- function field_extra_help($route_name, RouteMatchInterface $route_match) {}

Unused parameter '$form_id' :
- function field_extra_form_field_config_edit_form_alter(&$form, FormStateInterface $form_state, $form_id) {}
- function field_extra_form_alter(&$form, FormStateInterface $form_state, $form_id) {}

Comment

I don't know if it's necessary to have this comment

 /**
 * @file
 * Contains field_extra.module.
 */

Hi nikral,

sorry, but I don’t see any useful suggestions.

as I can see str_contains() only uses for php 8.0.
@file comment is mandatory for the module file.
parameters from your comment are not empty and present in hooks.

Hi AstonVictor

For params $form_id, it's a last param so if you dont need it you can remove it.
You don't use it anywhere in your function.

For the $field, you must test it before using.
What happen if $field is not object but you use
($field->isDisplayConfigurable('form')

Hi nikral,

$form_id is a parameter of the hook. in our case, the form alter hook

there are many examples in the core. e.g.
https://api.drupal.org/api/drupal/core%21modules%21ckeditor5%21ckeditor5...
https://api.drupal.org/api/drupal/core%21modules%21book%21book.module/fu...
etc
they have the $form_id parameter but don't use it.

please, provide me steps how to reproduce when $field is not object.

Apart from warnings reported by tools like PHP_CodeSniffer, there are not warnings shown by PHP. Using that additional parameter, which is still passed from Drupal core, does not create any issue.

Let's remind that in these applications, we verify the code correctly uses the Drupal APIs, follows the Drupal coding standards, and it does not contain security issues. Other suggestions are welcome, but they should not be taken as a must from the applicant.

Good code, a clear README, Coder returns 2 errors in the phpunit test, PARreview looks good. I would suggest being more descriptive in the help so people better understand what the module does. I didn't see any noticeable security vulnerabilities. Since apaderno seems to be ok with your code Im going to push this to RTBC. Thanks

FILE: /var/www/html/global/web/modules/contrib/field_extra/tests/src/Functional/FieldExtraTest.php
--------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------------------------------
28 | ERROR | The array declaration extends to column 104 (the limit is 80). The array content should be split up over multiple lines
79 | ERROR | The array declaration extends to column 96 (the limit is 80). The array content should be split up over multiple lines
--------------------------------------------------------------------------------------------------------------------------------------

Time: 4.86 secs; Memory: 10MB

Hi @Trigve Hagen,

thanks for your review.
FYI both issues with limits were fixed and merged.

manual review:

1. project page is too short. What does "Provides private entity fields functionality." mean? What does the module do? What is the use case?
2. field_extra_form_alter(): You load your config on every single form in Drupal. Instead, it would be better to only load the config after you checked that this is an entity form.
3. field_extra_schema(): I would not combine key of multiple properties and instead have a column for entity type and field name, to be better query-able in the future.
4. field_extra_schema(): a primary key is already an index, no need to define it as index again.
5. field_extra_entity_field_access(): Cache context information is missing. Probably all access results are cachePerUser(), right?
6. FieldExtraForm::buildForm(): Don't put class names in strings, use PHP ::class syntax

Otherwise looks good to me!

Thanks for your contribution, Viktor!

I updated your account so you can opt into security advisory coverage now.

Here 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 lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.

thanks for your feedback

I’ll take into account your suggestions

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