[1.0.x] Changelogify

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.

• New releases are not necessary for these applications, which could require changes that are not backward-compatible. Not creating new releases avoids any possible issue.
• Please do not change the branch to review once reviews started, except in the case the used branch needs to be deleted.
• If you have not done it yet, enable GitLab CI for the project, and fix what reported from the phpcs job. This help to fix most of what reviewers would report.
• For the time this application is open, only your commits are allowed. No other people, including other maintainers/co-maintainers can make commits.
• 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.
• Nobody else will get the permission to opt projects into security advisory policy. If there are other maintainers/co-maintainers who will to get that permission, they need to apply with a different module.
• 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. The configuration used for these tools needs to be the same configuration used by GitLab CI, stored in the GitLab Templates repository.
• 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.

1. main is a wrong name for a branch and should be removed. Release branch names always end with the literal .x as described in Release branches.

main will be a supported branch in future, but for the moment it is better not to use it. It is not wrong, but it is not completely supported on drupal.org.

2. There is No release on drupal.org.

Please go through #3

Regarding point 2 in comment #4,

2. There is No release on drupal.org.

A release is not required. Reviewers always verify the code directly from the branch.

1. FILE: changelogify.module

For a new module that aims to be compatible with Drupal 10/11, it is expected it implements hooks as class methods as described in Support for object oriented hook implementations using autowired services.

It would require increasing the minimum Drupal 10 version supported, but Drupal 10.1 is no longer supported.

/**
 * @file
 * Primary module hooks for Changelogify.
 */

Drupal does not have primary and secondary hooks. Instead of that, it is preferable to use the usual description: “Hook implementations for the [module name] module”, where [module name] is the name of the module given in its .info.yml file.

2. FILE: src/EventManager.php

    /**
     * The entity type manager.
     */
    protected EntityTypeManagerInterface $entityTypeManager;

    /**
     * The current user.
     */
    protected AccountProxyInterface $currentUser;

    /**
     * The time service.
     */
    protected TimeInterface $time;

    /**
     * Constructs an EventManager.
     */
    public function __construct(
        EntityTypeManagerInterface $entity_type_manager,
        AccountProxyInterface $current_user,
        TimeInterface $time
    ) {
        $this->entityTypeManager = $entity_type_manager;
        $this->currentUser = $current_user;
        $this->time = $time;
    }

FILE: src/ReleaseGenerator.php

    /**
     * The entity type manager.
     */
    protected EntityTypeManagerInterface $entityTypeManager;

    /**
     * The event manager.
     */
    protected EventManagerInterface $eventManager;

    /**
     * The current user.
     */
    protected AccountProxyInterface $currentUser;

    /**
     * The time service.
     */
    protected TimeInterface $time;

    /**
     * Constructs a ReleaseGenerator.
     */
    public function __construct(
        EntityTypeManagerInterface $entity_type_manager,
        EventManagerInterface $event_manager,
        AccountProxyInterface $current_user,
        TimeInterface $time
    ) {
        $this->entityTypeManager = $entity_type_manager;
        $this->eventManager = $event_manager;
        $this->currentUser = $current_user;
        $this->time = $time;
    }

FILE: src/Controller/DashboardController.php

    /**
     * The event manager.
     */
    protected EventManagerInterface $eventManager;

    /**
     * Constructs a DashboardController.
     */
    public function __construct(EventManagerInterface $event_manager)
    {
        $this->eventManager = $event_manager;
    }

FILE: src/Form/GenerateReleaseForm.php

    /**
     * The release generator.
     */
    protected ReleaseGeneratorInterface $releaseGenerator;

    /**
     * Constructs a GenerateReleaseForm.
     */
    public function __construct(ReleaseGeneratorInterface $release_generator)
    {
        $this->releaseGenerator = $release_generator;
    }

New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use constructor property promotion.

3. FILE: src/Entity/ChangelogifyEvent.php and src/Entity/ChangelogifyRelease.php

* @ContentEntityType(

Projects that are compatible with Drupal 10 or higher versions should use attributes instead of annotations. This means requiring at least Drupal 10.3, but this is not an issue, considering that the minimum supported Drupal version is now Drupal 10.4.9.

4. FILE: src/Form/ReleaseForm.php

$this->messenger()->addStatus($message);

The first argument passed to MessengerInterface::addError(), MessengerInterface::addMessage(), MessengerInterface::addStatus(), and MessengerInterface::addWarning() must be a translatable string which uses placeholders.

5. FILE: src/Form/SettingsForm.php

With Drupal 10 and Drupal 11, there is no longer need to use #default_value for each form element, when the parent class is ConfigFormBase: It is sufficient to use #config_target, as in the following code.

    $form['image_toolkit'] = [
      '#type' => 'radios',
      '#title' => $this->t('Select an image processing toolkit'),
      '#config_target' => 'system.image:toolkit',
      '#options' => [],
    ];

Using that code, it is no longer needed to save the configuration values in the form submission handler: The parent class will take care of that.
For this change, it is necessary to require at least Drupal 10.3, but that is not an issue, since Drupal 10.2.x is no longer supported.

6. Fix the warnings/errors reported by PHP_CodeSniffer. (see attached changelogify-phpcs-issues.txt)

NOTE: I would suggest enabling GitLab CI for the project, follow the Drupal Association .gitlab-ci.yml template and fix the PHP_CodeSniffer errors/warnings it reports.

1. main branch is still present. It needs to be deleted.

2. FILE: changelogify.module

Since the module is declared compatible with Drupal 10.3, removing the function implementing the hook is not possible. The function still needs to be defined, but it calls the method defined by the service class, as described in Support for object oriented hook implementations using autowired services (Backwards-compatible Hook implementation for Drupal versions from 10.1 to 11.0).

3. I don't see a .gitlab-ci.yml file.

If you changed what has been reported, please change the status to Needs review. In this way, reviewers will know everything has been changed and can be reviewed again.

Fix the warnings/errors reported by PHP_CodeSniffer. (see attached changelogify-phpcs-issues.txt)

Rest seems fine to me.

Please wait for other reviewers and Project Moderator to take a look and if everything goes fine, you will get the role.

Adjusting the priority as per Issue Priorities.

Hi,

I don't have time for a full review, but that composer.json file must be changed. It looks like the contents are coming from "drupal/recommended-project". See Add a composer.json file for more info.

I don't know why there's a web folder there, and its submodules: web/modules/custom/changelogify.

Remove php: 8.1 from the module's info file.

In the definition of changelogify_release content entity, you are defining the admin route_provider, but you are also manually defining entity routes in changelogify.routing.yml. The latter is not needed.

Since your module is compatible with Drupal 11, and already has the hooks prepared for OOP, you are just missing the hook attribute above each function in ChangelogifyHooks.php.

Adjusting the priority as per Issue Priorities.

The priority is reset to Normal after a review, if it was before raised to Major or Critical.

I reviewed the code at 1.0.x branch at this commit: c9e03696a207ad643841a87222aa19891ba406b8 and the following are my comments:

Consider using LegacyHook attribute for all the hooks defined in changelogify.module. Without that this will cause duplicate execution in Drupal 11 (once through the module_entity_insert hook, and second directly through the Hook interface).

function changelogify_entity_insert(EntityInterface $entity): void {
  if (!class_exists('Drupal\Core\Hook\Attribute\Hook')) {
    \Drupal::service('Drupal\changelogify\Hook\ChangelogifyHooks')->entityInsert($entity);
  }
}

[#LegacyHook]
function changelogify_entity_insert(EntityInterface $entity): void {
   \Drupal::service('Drupal\changelogify\Hook\ChangelogifyHooks')->entityInsert($entity);
}

This may cause phpstan problems in Drupal 10, which will have to be ignored.

parameters:
    ignoreErrors:
        - '#Attribute class Drupal\\Core\\Hook\\Attribute\\LegacyHook does not exist.#'

1. You are using a customized gitlab CI yml.
https://git.drupalcode.org/project/changelogify/-/blob/1.0.x/.gitlab-ci....
I think using the default gives you better feedback on your code:
https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...
Using the default, also saves you from providing your own phpstan.neon and phpcs.xml.dist files.

2. You use an AI generated README.md that differs from the preferred template https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or...
For example the installation section should not assume that people use DDEV

Gitlab CI pipeline looks great now!

For the README I would suggest a couple of changes.

1. Replace the complete installation section by the simple instruction

## Installation

Install as you would normally install a contributed Drupal module. For further
information, see
[Installing Drupal Modules](https://www.drupal.org/docs/extending-drupal/installing-drupal-modules).

See https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or...

The rest only causes confusion. Or is there a reason to clone the github repo?

2. Remove Roadmap and Contributions section. These are not common for the readme, but could be placed on the drupal.org project page.

3. Remove License section. I do not think it is needed, because it is implicit for all Drupal hosted code. (But feel free to leave it in if you have a special reason)

4. It is noticeable that the README is generated by an AI agent. If the module code includes any AI generated content, then it is a good practice to disclose this (on the project page or in the application). See https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

Since each project is not stored in the same repository containing Drupal core, the license file should be provided, to make explicit the license under which the code is provided.
At the times drupal.org used CVS, this was not necessary, since all the projects were hosted in a single repository which included also Drupal core. There was a single license file which was valid for Drupal core and every contributed project.

Looks great!

Not sure. I am not involved in the approval process. It seems most issues have been addressed. I will mark this as approved in the coming days if there is no more feedback.

Hi @erics1337,

Please stop changing the priority when it doesn't respect the Issue Priorities.

It seems all feedback has been addressed. I have not seen any more reports over the last week. I set this to RTBC

Thank you for your contribution and for your patience with the review process!

I am going to update your account so you can opt into security advisory coverage any project you create, including the projects you already created.

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!
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 all the reviewers for helping with these applications.