- Issue created by @arisen
- 🇮🇳India vishal.kadam Mumbai
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 → .
- If you have not done it yet, you should run
- 🇮🇳India arisen Goa
Fixed the phpcs issues as per Drupal and DrupalPractice sniffs.
Except the below error related to javascript as it breaks the functionality.
TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
- 🇮🇹Italy apaderno Brescia, 🇮🇹
As per Drupal coding standards,
true
,false
, andnull
are used in JavaScript. PHP_CodeSniffer applies PHP rules to JavaScript files, with the effect of giving false positives like those. - Status changed to Needs work
about 1 year ago 10:47am 9 November 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
- 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
ezdevportal.profile
catch (\Exception $e) { \Drupal::logger('ezdevportal')->error($e->getMessage()); }
The first argument passed to
error()
must be a literal string. To log an exception, there iswatchdog_exception()
(Drupal 8/9/10.0) or\Drupal\Core\Utility\Error::logException()
(Drupal 10.1+).I apologize I cannot complete the review. Please wait before changing the project files, or doing any further reviews. I will complete mine in 6 hours (less or more).
- 🇮🇹Italy apaderno Brescia, 🇮🇹
src/Installer/Form/ModuleConfigureForm.php
/** * Provides the site configuration form. */ class ModuleConfigureForm extends ConfigFormBase {
If a form is not used to change configuration objects, its parent class must be different.
modules/custom/ezdevportal_admin/ezdevportal_admin.module
$user = Drupal::CurrentUser(); $roles = $user->getRoles(); if ($form_id == "user_form") { if (in_array('site_admin', $roles)) { $form['account']['current_pass'][EzdevportalAdminInterface::FORM_ACCESS] = FALSE; $form['account']['name'][EzdevportalAdminInterface::FORM_ACCESS] = TRUE; $form['account']['pass'][EzdevportalAdminInterface::FORM_ACCESS] = FALSE; $form['actions']['back'] = [ '#type' => 'button', '#value' => t('Back'), '#button_type' => 'primary', '#attributes' => [ 'onclick' => 'window.history.back();return false;', 'class' => ['back-btn'], ], ]; }
In Drupal, access permission is done by checking the permissions an account has, not which roles are assigned to that account.
There is no need to use an interface just to define a constant, also because the property to check whether users are allowed to access a form element will always be#access
.$url = '/content-management'; $current_request = \Drupal::service('request_stack')->getCurrentRequest(); $current_request->query->set( 'destination', $url );
For setting a redirect, there is the redirect.destination service.
preg_match('!\d+!', $current_path, $matches); if (str_contains($current_path, '/node')) { $node = Node::load($matches[0]); if (!empty($node)) { $title = $node->getTitle(); } } elseif (str_contains($current_path, '/user')) { $node = User::load($matches[0]); if (!empty($node)) { $title = $node->getAccountName(); } }
The correct code to get the node object, when the viewed page is for a node is
$node = \Drupal::routeMatch()->getParameter('node');
similar code is used to get a user object, when the viewed page is for a user account.
Also, since that function is supported to get information about a node, it should not check for account paths.if ($action == 'Edit') { $title = ezdevportal_admin_get_node_data($current_path); } else { $title = 'New'; } if (str_contains($referer, '/api-list')) { $banner_title = 'APIs - ' . $action . ' ' . $title; } elseif (str_contains($referer, 'content-management')) { $banner_title = 'Content Management - ' . $action . ' ' . $title; } elseif (str_contains($referer, 'forum-list')) { $banner_title = 'Forum - ' . $action . ' ' . $title; } elseif (str_contains($referer, 'admin/structure/taxonomy')) { $banner_title = 'Categories Management - ' . $action . ' ' . $title; } elseif (str_contains($referer, 'admin/structure/menu')) { $banner_title = 'Menu Management - ' . $action . ' ' . $title; } elseif (str_contains($referer, 'user-list')) { $banner_title = 'Manage Users - ' . $action . ' ' . $title; } elseif (str_contains($current_path, 'admin/structure/menu/item')) { $banner_title = 'Menu Management - ' . $action . ' ' . $title; } elseif (str_contains($current_path, 'taxonomy/term')) { $banner_title = 'Categories Management - Edit Term'; } return $banner_title; }
Strings shown in a page, like the page title, must be translatable.
modules/custom/ezdevportal_admin/ezdevportal_admin.install
/** * @file * Provides all the hooks for handling the installation of the module. */ use Drupal\menu_link_content\Entity\MenuLinkContent; /** * @file * Contains install functionality for ezdevportal admin. */
The @file tag must be provided once per file. The usual description for a .install file is Install, update, and uninstall functions for the [module name] module. where [module name] is the name of the module give in its .info.yml file.
modules/custom/ezdevportal_api_documents/src/Form/ApiFormatterOptionsForm.php
/** * Current path variable. * * @var mixed */ protected $currentPath; /** * Path alias service variable. * * @var mixed */ protected $aliasManager;
Those properties contains instances of classes that implement a specific interface. They do not contain any other values, like a string, an integer, or an array.
It is not necessary to add variable in their descriptions./** * {@inheritdoc} */ public function submitForm(array &$form, FormStateInterface $form_state) { // ... }
That method is still to be implemented.
modules/custom/ezdevportal_api_documents/src/Plugin/Block/ApiFormatterOptionsBlock.php
/** * Constructs a new DownloadSdkBlock. * * @param array $configuration * A configuration array containing information about the plugin instance. * @param string $plugin_id * The plugin ID for the plugin instance. * @param mixed $plugin_definition * The plugin implementation definition. * @param \Drupal\Core\Form\FormBuilderInterface $form_builder * The form builder instance. */ public function __construct(array $configuration, $plugin_id, $plugin_definition, FormBuilderInterface $form_builder) { parent::__construct($configuration, $plugin_id, $plugin_definition); $this->formBuilder = $form_builder; }
The description in the documentation comment is for a different class.
Method and function declarations must be written on a single line.modules/custom/ezdevportal_api_documents/src/ApiDocumentHelper.php
/** * File for Api Document Helper. */ class ApiDocumentHelper {
The description does not say what the class purpose is.
/** * Get node id from current path. */ public function getNodeId() { $current_path = \Drupal::service('path.current')->getPath(); $current_path_array = explode('/', $current_path); return end($current_path_array); }
That class should be used to implement a service, but I do not see any need to move that code in a class (which actually is not used by other classes) for that simple code.
Also, that is not the correct way to get a node ID from the current path.modules/custom/ezdevportal_api_documents/src/Node.php
/** * Object EntityTypeManager. * * @var Drupal\Core\Entity\EntityTypeManagerInterface */ protected $entityTypeManager;
A property description must not repeat the property name.
/** * Pass the dependency to the object constructor. */ public function __construct( Connection $connection, EntityTypeManagerInterface $entity_type_manager) { $this->connection = $connection; $this->entityTypeManager = $entity_type_manager; }
The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.
/** * Delete Api Reference. */ public function deleteApiReference($entity_id) {
The verb used in the description must be declined to the third person singular.
The parameter and the return value descriptions are missing.catch (\Exception $e) { $logger = $this->getLogger('ezdevportal-api-documents-reference'); $logger->error($e->getMessage()); }
The first argument passed to
error()
must be a literal string. To log an exception, there iswatchdog_exception()
(Drupal 8/9/10.0) or\Drupal\Core\Utility\Error::logException()
(Drupal 10.1+).modules/custom/ezdevportal_base/ezdevportal_base.module
/** * @file * Contains common hooks. */ use Drupal\Component\Utility\Html; use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Url; use Drupal\node\Entity\Node; use Drupal\node\NodeInterface; use Drupal\text\Plugin\Field\FieldWidget\TextareaWidget; use Drupal\user\Entity\User; /** * @file * EzDevPortal Base module file. */
The
@file
tag is used once per file.
The description for a module is Hook implementations for the [module name] module. where [module name] is the module name given in the .info.yml file./** * Implements template_preprocess_block(). */ function ezdevportal_base_preprocess_block__content_cards_block(&$variables) { $block_data = $variables['elements']['content']['#block_content']; $card_block['block_header'] = $block_data->field_title->value; $card_block['block_width_type'] = $block_data->field_block_position->value; preg_match_all('/\d+/', $block_data->field_cards_layout->value, $matches); $card_block['block_column_count'] = $matches[0][0]; if (!$block_data->field_read_more_link->isEmpty()) { $card_block['more_link'] = Url::fromUri($block_data->field_read_more_link[0]->uri)->toString(); $card_block['more_link_title'] = $block_data->field_read_more_link[0]->title; } $referenced_nodes = $block_data->field_referenced_nodes->getValue(); foreach ($referenced_nodes as $referenced_node) { $card_block['cards'][] = ezdevportal_base_get_node_card($referenced_node['target_id']); } $variables['card_block'] = $card_block; } /** * Implements template_preprocess_block(). */ function ezdevportal_base_preprocess_block__content_list_block(&$variables) { $block_data = $variables['elements']['content']['#block_content']; $banner_content['title'] = $block_data->field_title->value; $banner_content['link'] = Url::fromUri($block_data->field_read_more_link->getValue()[0]['uri'])->toString(); $banner_content['link_title'] = $block_data->field_read_more_link->getValue()[0]['title']; if (!$block_data->field_block_image->isEmpty()) { $banner_content['image'] = \Drupal::service('file_url_generator')->generateAbsoluteString($block_data->field_block_image->entity->field_media_image->entity->getFileUri()); } $banner_content['banner_image'] = $block_data->field_title->value; $referenced_nodes = $block_data->field_referenced_nodes->getValue(); foreach ($referenced_nodes as $referenced_node) { $banner[] = ezdevportal_base_get_node_card($referenced_node['target_id']); } $banner_content['content'] = $banner; $variables['banner_content'] = $banner_content; } /** * Implements template_preprocess_block(). */ function ezdevportal_base_preprocess_block__media_cards_block(&$variables) { $block_data = $variables['elements']['content']['#block_content']; $card_block['block_header'] = $block_data->field_title->value; preg_match_all('/\d+/', $block_data->field_cards_layout->value, $matches); $card_block['block_column_count'] = $matches[0][0]; if (!$block_data->field_read_more_link->isEmpty()) { $card_block['more_link'] = Url::fromUri($block_data->field_read_more_link[0]->uri)->toString(); $card_block['more_link_title'] = $block_data->field_read_more_link[0]->title; } $referenced_medias = $block_data->field_referenced_media->referencedEntities(); foreach ($referenced_medias as $referenced_media) { $card_block['cards'][] = \Drupal::entityTypeManager()->getViewBuilder('media')->view($referenced_media); } $variables['card_block'] = $card_block; } /** * Implements template_preprocess_block(). */ function ezdevportal_base_preprocess_block__video_collection_block(&$variables) { $block_data = $variables['elements']['content']['#block_content']; $referenced_videos = $block_data->field_video_items->referencedEntities(); foreach ($referenced_videos as $referenced_video) { $card_block['cards'][] = \Drupal::entityTypeManager()->getViewBuilder('paragraph')->view($referenced_video); } $variables['card_block'] = $card_block; } /** * Implements template_preprocess_block(). */ function ezdevportal_base_preprocess_block__sl ider_block(&$variables) { $block_data = $variables['elements']['content']['#block_content']; $block_content = $block_data->field_slide->referencedEntities(); $content_slides = []; foreach ($block_content as $key => $value) { $content_slides[$key]['heading_1'] = $value->field_slide_main_heading->value; $content_slides[$key]['heading_2'] = $value->field_slide_sub_heading->value; $content_slides[$key]['description'] = $value->field_slide_description->value; $content_slides[$key]['cta_link'] = Url::fromUri($value->field_slider_cta_link->getValue()[0]['uri'])->toString(); $content_slides[$key]['cta_link_title'] = $value->field_slider_cta_link->getValue()[0]['title']; $content_slides[$key]['image_url'] = \Drupal::service('file_url_generator')->generateAbsoluteString($value->field_slide_image->entity->field_media_image->entity->getFileUri()); } $variables['content_slides'] = $content_slides; }
Those are
hook_preprocess_HOOK()
implementation for block templates./** * Function to get node card fields by node id. */ function ezdevportal_base_get_node_card($nid) {
The documentation comment is missing the parameter and return value descriptions.
Also, it is not necessary to start the description with Function to.function ezdevportal_base_user_login($account) { $roles = $account->getRoles(); if (in_array('administrator', $roles)) { $url = '/admin/content'; $current_request = \Drupal::service('request_stack')->getCurrentRequest(); $current_request->query->set( 'destination', $url ); } }
The correct code to check if the account is for an administrator user is to call
$account->hasRole()
, which is possible as the parameter passed to the hook implements\Drupal\user\UserInterface
.modules/custom/ezdevportal_product/src/Plugin/Block/ProductContentBlock.php
/** * {@inheritdoc} */ public function build() { return [ '#markup' => '', ]; }
It seems that class does not contain complete code.
modules/custom/ezdevportal_product/templates/product-navigation.html.twig
{{ value.svg_icon | raw }} {{ value.text }}
Rendering the raw content of a SVG document is a possible security issue.
modules/custom/ezdevportal_user/src/Form/ChangePasswordForm.php
Since Drupal core already provides a form to reset a password, what is the difference between this class and the Drupal core class?
/** * The configuration factory. * * @var \Drupal\Core\Config\ConfigFactoryInterface */ protected $configFactory;
That property is already defined from the parent class. There is no need to redefine it.
See also the methods provided byFormBase
to handle that property./** @var \Drupal\user\UserInterface $account */ $this->userProfile = $account = $this->entityTypeManager->getStorage('user')->load($this->currentUser->id());
$userProfile
is not define by the class or its parent class. FurthermoresubmitForm()
loads the same user object that is already stored in that property.modules/custom/ezdevportal_user/templates/ezdevportal-login-with.html.twig
{% if (current_path == '/user/login') or (current_path == '/user/password') %} <div>Don't have an account? <a href="/user/register">Register</a></div> {% elseif current_path == '/user/register' %} <div>Already have an account? <a href="/user/login">Login</a></div> {% endif %}
Strings shown in the user interface must be translatable. That holds true also for strings used in template files.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
I am changing priority as per Issue priorities → .
- Status changed to Closed: won't fix
3 months ago 7:21am 13 August 2024 - 🇮🇳India vishal.kadam Mumbai
This thread has been idle, in the Needs work state with no activity for several months. Therefore, I am assuming that you are no longer pursuing this application, and I marked it as Closed (won't fix).
If this is incorrect, and you are still pursuing this application, then please feel free to re-open it and set the issue status to Needs work or Needs review, depending on the current status of your code.