[1.0.x] Generative Summary

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 ;)

@Mattixonix Thank you for your review.
It seems the review was based on rc1. There were many /src codes fixed in rc2 version, although it was not set as the release version in the project page.

I fixed the Readme, yaml and module files in the root folder, should be ready for review. Could you review with 1.0.x dev or I have to make another release?

Regarding the error you have, it looks weird to me, too.
Could you elaborate a little more about where you got the error, please.

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

@Mattixonix Thanks for the information.

There is a global settings that includes the OpenAI API key and endpoint url.
What you encountered might be related to that.
However I don't see issue related to the 'edit any page content' permission, and cannot reproduce with the step.

The latest updates will catch the AJAX error, and the field settings page won't allow user to enable the feature when no API information has set.

Please kindly review with the latest update, and let me know if you find any issue.

• 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

@apaderno

Thank you for the detailed explanation.
I'd like to get some advice about the class implement of the TextareaWithGenerativeSUmmaryWidget class. If I understand correctly, I will be able to make the implementation starting from the WidgetBase according to the hierarchy of class TextareaWithSummaryWidget

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

Thank you @apaderno, I've updated the code based on the review comments.

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.

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