[1.0.x] Generative Summary

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 will not be changed by this application; 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 branch to review and the project name.

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, even to leave a comment similar to the following one. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• It is also preferable to wait before using a CLI tool to report what needs to be changed, especially because the comment left from Code Review Administrators suggests to use PHP_CodeSniffer. Before that, manual reviews should be done.
• Reviewers should not copy-paste the output of a CLI tool. They should use a CLI tool only once per application. When they do that, they should later verify the code has been correctly changed; this means, for example, that adding a documentation comment that is not correct just to avoid to get a warning/error is not a correct change that should be reported in a further comment.
• 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.

Hi @amourow, catch few tips from me ;):

• .info.yml file - change package to something more specific
• .info.yml file - You are including js file (if im right) to all pages and this is very bad practice, optimize that by creating .libraries.yml file and set library for this file, then include this library only where it is used, i believe in your case in edit entities forms only when that specific field is used
• .module - add @file description, ;
• .module - add functions descriptions with what they implements and also add short description why this hook is invoke but separate implementation information from short desc with single empty line
• .module - type functions arguments - it's very good practice
• .module - i see that You are using hook_form_alter() and then it starts from if() checking for proper $form_id, in this case to have cleaner code use hook_form_FORM_ID_alter() instead
• .module - remove AjaxResponse, InvokeCommand, GenerativeSummaryController form use section
• composer.json - is your module works fine with php7.3 to 8.2? Is it tested? Im asking because You support D9 and D10. D9.0 works even with php7.3 and some people will want to use this module on this php version. It's better to set php requirement in composer file, also You can change support in info.yml file form D9 to D9.5, but first test module on this versions.
• .routing.yml - there is options _admin_route: TRUE, it's not needed because everything after /admin use administrations theme
• In src files it's nice to type arguments and return types even if they are describe in @param | @return
• Check whole module and add class descriptions
• You can't use code like that \Drupal::messenger(), use Dependency Injections instead, everytime You are in OOP code, see more here https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio...
• When i tried to use this module to generate summary i get ajax error like this ""The 'edit any page content' permission is required." and that is very strange cause i'm on admin account

Please fix also phpcs errors:

FILE: /opt/drupal/web/modules/contrib/generative_summary/generative_summary.module
----------------------------------------------------------------------------------
FOUND 3 ERRORS AND 3 WARNINGS AFFECTING 6 LINES
----------------------------------------------------------------------------------
   3 | ERROR   | [ ] Missing short description in doc comment
   7 | WARNING | [x] Unused use statement
   8 | WARNING | [x] Unused use statement
  11 | WARNING | [x] Unused use statement
  22 | ERROR   | [x] Missing function doc comment
 108 | ERROR   | [x] Missing function doc comment
----------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------


FILE: /opt/drupal/web/modules/contrib/generative_summary/generative_summary.info.yml
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
 1 | WARNING | Remove "project" from the info file, it will be added by drupal.org packaging automatically
 1 | WARNING | Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically
 1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically
-------------------------------------------------------------------------------------------------------------


FILE: /opt/drupal/web/modules/contrib/generative_summary/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 10 LINES
----------------------------------------------------------------------
  1 | WARNING | Line exceeds 80 characters; contains 429 characters
  4 | WARNING | Line exceeds 80 characters; contains 114 characters
  5 | WARNING | Line exceeds 80 characters; contains 126 characters
  6 | WARNING | Line exceeds 80 characters; contains 165 characters
  7 | WARNING | Line exceeds 80 characters; contains 127 characters
  8 | WARNING | Line exceeds 80 characters; contains 125 characters
  9 | WARNING | Line exceeds 80 characters; contains 143 characters
 12 | WARNING | Line exceeds 80 characters; contains 88 characters
 13 | WARNING | Line exceeds 80 characters; contains 170 characters
 14 | WARNING | Line exceeds 80 characters; contains 86 characters
----------------------------------------------------------------------


FILE: /opt/drupal/web/modules/contrib/generative_summary/src/Form/GenerativeSummaryConfigForm.php
--------------------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
--------------------------------------------------------------------------------------------------------------
   8 | ERROR | [x] Missing class doc comment
  12 | ERROR | [x] Do not append variable name "$models" to the type declaration in a member variable comment
  26 | ERROR | [x] Missing function doc comment
  30 | ERROR | [x] Missing function doc comment
  34 | ERROR | [x] Missing function doc comment
 143 | ERROR | [x] Missing function doc comment
--------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------


FILE: /opt/drupal/web/modules/contrib/generative_summary/src/Plugin/Field/FieldWidget/TextareaWithGenerativeSummaryWidget.php
-----------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------
 74 | ERROR   | [x] Missing function doc comment
 85 | WARNING | [ ] Unused variable $max_chunk_tokens.
-----------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------


FILE: /opt/drupal/web/modules/contrib/generative_summary/src/Controller/GenerativeSummaryController.php
-------------------------------------------------------------------------------------------------------
FOUND 19 ERRORS AND 4 WARNINGS AFFECTING 20 LINES
-------------------------------------------------------------------------------------------------------
   9 | ERROR   | [x] Missing class doc comment
  11 | ERROR   | [x] Missing function doc comment
  18 | ERROR   | [ ] Missing parameter comment
  19 | ERROR   | [ ] Missing parameter comment
  20 | ERROR   | [x] Separate the @param and @return sections by a blank line.
  20 | ERROR   | [ ] Description for the @return value must be on the next line
  50 | ERROR   | [x] Missing function doc comment
  52 | ERROR   | [x] Expected 1 blank line after function; 2 found
  60 | ERROR   | [x] Separate the @param and @return sections by a blank line.
  78 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
  83 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
  88 | ERROR   | [ ] Missing short description in doc comment
  89 | ERROR   | [ ] Missing parameter comment
  89 | ERROR   | [ ] Missing parameter type
  90 | ERROR   | [ ] Missing parameter comment
  90 | ERROR   | [ ] Missing parameter type
  91 | ERROR   | [ ] Description for the @return value is missing
  94 | ERROR   | [x] Opening brace should be on the same line as the declaration
 120 | ERROR   | [x] Expected newline after closing brace
 121 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
 126 | ERROR   | [x] Missing function doc comment
 132 | ERROR   | [x] Missing function doc comment
 135 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------

It is really nice module in my opinion and i'm sorry i couldn't use it proper, hope You will fix this ;)

@amourow There is no need to create a release after every review. Reviewers will review the code from the 1.0.x branch.

Yes, the branch needs to be updated first for every change review before a release.

Hi again @amourow!

About fixes, that's my fault i look for released version, not the newest dev one.

About error, how to reproduce:

1. Install fresh Drupal 10.1.6
2. Install and enable Generative Summary module
3. In Simple Page node structure body field add Enable Generative Summary
4. Do not edit any settings - out of the box
5. Create Simple page and try generate summary

In first attempt i had to increase max_filesize because 2M was not enough.
Then sometimes i had error from console that You are trying to add message on null response.
Also sometimes i had error with "The 'edit any page content' permission is required." if You will see network tab and try display response, worth mention that im on admin account.

Do i need any API key or API organization? If yes they should be required.

• 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

src/Controller/GenerativeSummaryController.php

$this->messenger()->addMessage('Missing or incorrect OpenAI API Key for Generative Summary module.', 'error');

    catch (\Exception $ex) {
      $this->messenger()->addMessage($ex->getMessage(), 'error');
    }

The first argument passed to messenger methods must be a translatable string.
In the case of exceptions, it would be better they would logged, especially when the message would be read from users who are not administrators.
See watchdog_exception() to understand which code should be used to log an exception. I would not suggest to call that function, as it has been deprecated and it will be removed in future Drupal releases, but its code would still run.

    catch (\Exception $e) {
      $this->getLogger('generative_summary')->error('Failed generating summary: ' . $e->getMessage());
      return '';
    }

The first argument passed to the logger methods that log a message must be a literal string, not translatable strings nor string concatenations.

src/Form/GenerativeSummaryConfigForm.php

  /**
   * Returns the unique ID of the form.
   *
   * @return string
   *   The form ID.
   */
  public function getFormId(): string {
    return 'generative_summary_config';
  }

  /**
   * Returns the names of the configuration that this form will manipulate.
   *
   * @return array
   *   An array of configuration object names.
   */
  protected function getEditableConfigNames(): array {
    return ['generative_summary.settings'];
  }

Methods inherited from a parent class or defined in an interface use {@inheritdoc} as documentation comment.

    $form['openai_model'] = [
      '#type' => 'select',
      '#title' => $this->t('Model'),
      '#description' => $this->t('The model which will generate the completion. <a href="@link" target="_blank">Learn more</a>.', ['@link' => 'https://platform.openai.com/docs/models']),
      '#options' => $this->models,
      '#default_value' => !empty($config->get('openai_model')) ? $config->get('openai_model') : 'gpt-3.5-turbo',
      '#required' => TRUE,
    ];

The correct URL placeholders are the ones that start with a colon, which is the same URL placeholder Drupal core uses for links to drupal.org.

src/Plugin/Field/FieldWidget/TextareaWithGenerativeSummaryWidget.php

/**
 * Plugin implementation of the 'textarea_with_generative_summary' widget.
 *
 * -@FieldWidget(
 *   id = "textarea_with_generative_summary",
 *   label = @Translation("Text area with a generative summary"),
 *   field_types = {
 *     "text_with_summary"
 *   }
 * )
 */

The hyphen before the annotation seems a typo.

class TextareaWithGenerativeSummaryWidget extends TextareaWithSummaryWidget {

As per Drupal core backend backwards compatibility and internal API policy (Drupal 8 and above), classes that implement plugins are not part of the public API and must not be used as parent class by contributed projects.

      $controller = new GenerativeSummaryController();
      $summary = $controller->generateSummaryFromText($body, $max_length, $max_sentences, $max_chunk_tokens, $custom_prompt_system_role, $custom_prompt_user_role);

Controllers are not created with new. That controller should rather be a service, since:

• It is used from a plugin
• It is not associated to any route defined from the module

WidgetBase can be used as parent class, since it is a base class and it is not marked as internal. 👍

I am changing the issue priority as per issue priorities.

Thank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.

These are some recommended readings to help you with maintainership:

• Dries' post on Responsible maintainers
• Maintainership
• Git version control system
• Issue procedures and etiquette
• Maintaining and responding to issues for a project
• Release naming conventions.

You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thank you 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 also the dedicated reviewers as well.