[1.0.x] Simple Content Notifications

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

1. FILE: simple_content_notifications.info.yml

core_version_requirement: ^8 || ^9

The Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.

2. FILE: src/Form/ContentNotificationSettingsForm.php and src/Form/ContentReviewNotificationSettingsForm.php

  /**
   * {@inheritdoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {

  }

You can remove this method since it is not used.

@vishal.kadam I addressed the items you listed.

For the info.yml file, I set the core version requirement to ^9 || ^10. The addition of ^10 is in an open issue on the module for automated D10 compatibility fixes. The addition of ^10 to the core version requirement was the only compatibility fix listed.

I also removed the unused methods from the Forms. I was previously under the impression that these methods needed to be declared in order for the Form controller to work, regardless of whether it was used. However, I see in manual testing that the forms work fine without the unused method being declared.

Rest looks fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

Please fix the below PHPCS issue:

FILE: /simple_content_notifications/README.md
----------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------
 8 | WARNING | Line exceeds 80 characters; contains 84 characters
----------------------------------------------------------------------------------

@vinaymahale the line exceeds 80 characters issue has been resolved on the 1.0.x branch, along with several other similar error notices.

Please fix the below PHPCS issues:

FILE: /simple_content_notifications/src/Form/ContentReviewNotificationSettingsForm.php
-----------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------
  62 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 154 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-----------------------------------------------------------------------------------------------------------------------------

Okay, the dev branch now includes the update to the ContentReviewNotificationSettingsForm to use dependency injection of the State.

Automated Review

FILE: /var/www/html/web/modules/contrib/simple_content_notifications/simple_content_notifications.module
-----------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 9 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\node\Entity\Node.
-----------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------


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

Manual Review Suggestions for Improving Code

Coding Style and Naming:

1. Make sure that your code is consistent in using similar words throughout. For example, if you use "Needing" in one place, use it consistently instead of mixing it with "Needs", for instance, your controller name is \Drupal\simple_content_notifications\Controller\NeedsReview, but all the routing and form names are Content Needing Review, etc. This will help avoid confusion.

Fixing a Missing Slash:

1. From ContentReviewNotificationSettingsForm, a forward slash is missing. Instead of saying @var Drupal\Core\State\StateInterface, it should be @var \Drupal\Core\State\StateInterface.

Simplifying Code:

1. There's an opportunity to simplify your code by using a function called str_contains instead of complex logic. For instance, this code:

$on_live_site = (strpos($base_url, $live_domain) !== FALSE) ? TRUE : FALSE;

Can be made simpler like this:

$on_live_site = str_contains($base_url, $live_domain);

Improving Code Reusability:

1. To make your code more efficient and easier to understand, consider using the "Code Reusability" concept. Instead of repeating the same code to fetch configuration settings, you can consolidate it. For instance, this:

$logging_enabled = \Drupal::config('simple_content_notifications.review_settings')->get('review_notifications_logging');
$due = \Drupal::config('simple_content_notifications.review_settings')->get('review_notifications_due');
$field = \Drupal::config('simple_content_notifications.review_settings')->get('review_notifications_field');
$published_only = \Drupal::config('simple_content_notifications.review_settings')->get('review_notifications_published');

Could be improved to:

$config = \Drupal::config('simple_content_notifications.review_settings');
$logging_enabled = $config->get('review_notifications_logging');
$due = $config->get('review_notifications_due');
$field = $config->get('review_notifications_field');
$published_only = $config->get('review_notifications_published');

Thank you @hemangi-gokhale for the specific recommendations. I have addressed the points listed in your message and updated the 1.0.x branch accordingly.

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 everyone who took the time to review this. I appreciate it.

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