[1.0.x] Leaflet Dynamic Table

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 enable GitLab CI for the project and fix the PHP_CodeSniffer errors/warnings it reports.
• 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; once this application is closed, you will be able to change the project status from Not covered to Opt into security advisory coverage. This is possible only 14 days after the project is created.
  
  
  Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
• Only the person who created the application will get the permission to opt projects into security advisory coverage. No other person will get the same permission from the same application; that applies also to co-maintainers/maintainers of the project used for the application.
• 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 project moderator before posting the first comment on newly created applications. Project moderators 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.

1. master 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.

2. FILE: composer.json

It is not necessary to add the Drupal core requirements in the /composer.json/ file: The Drupal.org Composer Façade will add them.

3. FILE: leaflet_dynamic_table.info.yml

core_version_requirement: ^9 || ^10 || ^11

A new project should not declare itself compatible with a Drupal release that is no longer supported. No site should be using Drupal 8 nor Drupal 9, and people should not be encouraged to use those Drupal releases.

4. FILE: leaflet_dynamic_table.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.

5. FILE: src/Controller/LeafletDynamicTableController.php

  /**
   * The renderer service.
   *
   * @var \Drupal\Core\Render\RendererInterface
   */
  protected $renderer;

  /**
   * Constructs a LeafletDynamicTableController object.
   *
   * @param \Drupal\Core\Render\RendererInterface $renderer
   *   The renderer service.
   */
  public function __construct(RendererInterface $renderer) {
    $this->renderer = $renderer;
  }

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.

It is better not to create new releases during these applications, since a review could ask for a change that is not backward compatible with the existing releases. Just using a development version avoids those BC issues.

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.

Below u have my feedback:
- Missing schema for the Views Style plugin
- The controller's parent class uses the AutowireTrait, which means that the child static create method is redundant when the constructor uses property promotion. On the other hand, it looks like the parent controller is not really needed here since none of its functionality is being used.
- Plugins should be implemented as PHP Attributes instead of PHP Annotations
- The access on the route is way too open, not even CSRF, or custom access. Even though there's a bit of an access check at view level inside the controller, unauthorised requests should not hit the controller.
- You're manually building responses like 403, 404 that are not even checked in the JS. It would be recommended to use exceptions like
NotFoundHttpException if responses like that are still needed after route adjustments.
- Potential DoS through resource exhaustion caused by the following line in the controller $view->setItemsPerPage(0). The controller fetches all results into memory with no rate limit.
- Why the use of &drupal_static('leaflet_dynamic_table_entity_ids', NULL); (which btw the second argument is redundant) when the Views object can be used to pass data to the hook?

Review of leaflet_dynamic_table - 1.0.x branch

Thank you for addressing the feedback from the previous reviews. The module is well-structured overall, and the switch to OOP hooks with #[Hook] attributes, the PHP attribute-based #[ViewsDisplay], the schema file, and the HTTP exception usage are all correct. Below are the remaining issues I found, grouped by severity.

Critical

1. Hardcoded AJAX URL breaks subdirectory
   installations

   In LeafletDynamicAttachment::attachTo() (line 376):

   'ajaxUrl' => '/leaflet-dynamic-table/update',

   A leading / is site-root-relative. If Drupal is installed
   in a subdirectory (e.g., https://example.com/drupal/), the
   JavaScript will send requests to
   https://example.com/leaflet-dynamic-table/update instead of
   https://example.com/drupal/leaflet-dynamic-table/update. This is
   a functional bug for any non-root installation.

   Fix: Import the Url class and generate
   the URL properly:

   use Drupal\Core\Url;
     // ...
     'ajaxUrl' =>
     Url::fromRoute('leaflet_dynamic_table.update')->toString(),

2. items_per_page is accepted from the client without
   server-side validation

   In LeafletDynamicTableController::update() (line 70):

   $items_per_page = max(0, (int)
     $request->request->get('items_per_page', 0));

   max(0, ...) allows the client to send
   items_per_page=0, which in handleViewportChange()
   skips the PHP slice entirely and returns all matching results in a single
   response, bypassing the cap. A client could also send an arbitrarily large
   value such as items_per_page=99999.

   The items_per_page is a display-level configuration value.
   The server already has it available via the loaded view display. It should
   not be trusted from client input.

   Fix: Read it from the display configuration
   server-side:

   $display = $view->getDisplay();
     $items_per_page = (int) $display->getOption('items_per_page') ?:
     25;

   If reading from the display is not straightforward at that point in the
   call stack, at minimum clamp it to a safe range:

   $items_per_page = max(5, min(200, (int)
     $request->request->get('items_per_page', 25)));

Major

1. declare(strict_types=1) is missing from all PHP
   files

   None of the PHP files — LeafletDynamicAttachment.php,
   LeafletDynamicTableController.php,
   LeafletDynamicTableHooks.php, or
   leaflet_dynamic_table.module — have
   declare(strict_types=1). This is expected for all PHP files in
   Drupal 11-targeting modules and is flagged by PHPCS on recent Drupal coding
   standard rulesets.

   Each PHP file should start with:

   <?php
   
     declare(strict_types=1);

   For the .module file, the @file docblock goes
   before declare():

   <?php
   
     /**
      * @file
      * ...
      */
   
     declare(strict_types=1);

2. Class constants are missing explicit public
   visibility

   In LeafletDynamicTableController (lines 32–37):

   const ENTITY_IDS_CAP = 10000;
     const TEMPSTORE_COLLECTION = 'leaflet_dynamic_table';

   PHP 7.1+ supports visibility on class constants. Drupal coding
   standards require explicit visibility:

   public const ENTITY_IDS_CAP = 10000;
     public const TEMPSTORE_COLLECTION = 'leaflet_dynamic_table';

3. Missing type hints on protected method parameters

   handleViewportChange() (line 128) and
   handleScroll() (line 199) both have an untyped $view
   parameter:

   protected function handleViewportChange(Request $request,
     $view, ...)
     protected function handleScroll($view, ...)

   These should be typed:

   use Drupal\views\ViewExecutable;
     // ...
     protected function handleViewportChange(Request $request, ViewExecutable
     $view, ...)
     protected function handleScroll(ViewExecutable $view, ...)

   Similarly, viewsQueryAlter() in
   LeafletDynamicTableHooks (line 94) has an untyped
   $query parameter:

   public function viewsQueryAlter(ViewExecutable $view, $query):
     void {

   Should be:

   use Drupal\views\Plugin\views\query\QueryPluginBase;
     // ...
     public function viewsQueryAlter(ViewExecutable $view, QueryPluginBase $query):
      void {

4. Dynamic property set on ViewExecutable is
   deprecated in PHP 8.2+

   In both handleViewportChange() and
   handleScroll():

   $view->leaflet_dynamic_entity_ids = [...];

   Setting an undeclared dynamic property on an object triggers a PHP 8.2
   deprecation warning when the class does not declare
   #[AllowDynamicProperties]. While ViewExecutable may
   currently tolerate this, it is fragile and will break under stricter PHP
   versions.

   Recommended fix: Pass this data via a request-scoped
   service or use a static property on your own class keyed by view ID and
   display ID, then access it in viewsQueryAlter().

5. Direct manipulation of $view->build_info['query']
   is fragile

   In handleScroll() (lines 229–231):

   if (isset($view->build_info['query'])) {
       $view->build_info['query']->range($offset, $limit);
     }

   build_info['query'] is an internal implementation detail
   of Views' Sql query plugin and is not part of any stable API. It may change or
   be removed in future Drupal releases. This should at minimum have a

   //
     @todo

   notice, and the fragility should be documented in the README or
   inline comments.

Minor

1. Classes should be final

   LeafletDynamicTableHooks and
   LeafletDynamicAttachment are not declared final. For
   hook classes and Views plugins in Drupal 11, final is the
   recommended modifier to prevent unintended subclassing:

   final class LeafletDynamicTableHooks { ... }
     final class LeafletDynamicAttachment extends Attachment { ... }

2. highlight_color is not validated
   server-side

   The highlight_color option is passed directly from view
   config to drupalSettings. The #type => 'color'
   form element enforces the #RRGGBB format in the browser, but
   nothing validates the stored value server-side. A malicious actor with
   sufficient permissions to edit views could craft a raw config save to inject
   an arbitrary string which would then be passed to jQuery.css().
   Add validation in submitOptionsForm():

   $color = $form_state->getValue('highlight_color');
     if (!preg_match('/^#[0-9a-fA-F]{6}$/', $color)) {
       $color = '#ffeb3b';
     }
     $this->setOption('highlight_color', $color);

3. JavaScript only supports a single map instance per
   page

   Drupal.leafletDynamicAttachment is a single shared object
   with instance state (currentPage, cacheKey,
   map, etc.). If two views each with a Leaflet map and a dynamic
   attachment appear on the same page, they will share this state and break each
   other. This should be documented in the README as a known limitation.

4. README heading case inconsistency and unnecessary
   License section

   The How It Works heading should be sentence case:
   How it works, per the drupal.org README template. The
   License section is not part of the standard README template and
   is redundant with the LICENSE.txt file. It should be removed.

5. Use a dedicated logger channel

   In services.yml, the module uses
   @logger.channel.default. This logs to the default channel, making
   it harder for site builders to filter module-specific log entries. Use a
   dedicated channel instead:

   $logger: '@logger.channel.leaflet_dynamic_table'

Summary

The module has made substantial progress since the initial reviews. The two
critical issues (hardcoded URL and unvalidated items_per_page)
must be addressed before approval. The major issues — particularly
declare(strict_types=1), missing type hints, and the dynamic
property concern — should also be resolved. The minor items are improvements
but less urgent.

Verified all 13 fixes against the actual code in commit
be790bb. 11 of 13 are confirmed fixed. 2 have remaining
issues.

Confirmed fixed (11/13)

1. Hardcoded URL -
   Url::fromRoute('leaflet_dynamic_table.update')->toString()
   correctly used in attachTo().
2. items_per_page from client — No longer
   read from POST. Now read server-side via
   $view->getDisplay()->getOption('items_per_page') with a 5–200
   clamp.
3. declare(strict_types=1) — Present in
   LeafletDynamicAttachment.php,
   LeafletDynamicTableController.php, and
   LeafletDynamicTableHooks.php. (see remaining issue below for
   .module)
4. Constant visibility -
   

   public const
     ENTITY_IDS_CAP

   and public const TEMPSTORE_COLLECTION
   confirmed.

5. Untyped $view parameter - Both
   handleViewportChange() and handleScroll() now typed
   as ViewExecutable.
6. Untyped $query in hooks class -
   QueryPluginBase $query confirmed on line 98 of
   LeafletDynamicTableHooks.php. (see remaining issue below for
   .module)
7. Dynamic property on ViewExecutable -
   Replaced with protected static array $entityIds on the
   controller, accessed via
   setEntityIds()/getEntityIds() static methods.
   viewsQueryAlter() now calls
   LeafletDynamicTableController::getEntityIds().
8. build_info['query'] comment - Detailed
   inline comment added referencing
   Drupal\views\Plugin\views\pager\None::query() to explain why
   direct SelectQuery::range() is necessary.
9. final + hasAttachment()
   visibility - LeafletDynamicTableHooks is now
   final and hasAttachment() changed to
   private.
10. highlight_color server-side validation -
    preg_match('/^#[0-9a-fA-F]{6}$/', ...) with #ffeb3b
    fallback confirmed in submitOptionsForm().
11. Dedicated logger channel -
    logger.channel.leaflet_dynamic_table registered in
    services.yml and wired to the hooks class constructor.
12. Known limitations in README -
    

    Known
      limitations

    section added documenting the single map per page
    constraint.

13. README heading case and License section -
    

    How it
      works

    (sentence case) confirmed, License section removed.

Two remaining issues

1. .module file is still missing
   declare(strict_types=1)

   The explanation given - that PHPCS prevents it - is not correct. PHPCS
   requires the @file docblock to appear before
   declare(), not instead of it. The correct order is:

   <?php
   
     /**
      * @file
      * Legacy hook implementations...
      */
   
     declare(strict_types=1);

   The @file docblock is already present in the file. Adding
   declare(strict_types=1); on the line after it is all that is
   needed.

2. Legacy shim in .module still has an untyped
   $query parameter

   The fix was confirmed in LeafletDynamicTableHooks.php, but
   line 44 of leaflet_dynamic_table.module still reads:

   function leaflet_dynamic_table_views_query_alter(ViewExecutable
      $view, $query): void {

   $query remains untyped in the legacy shim. It should
   be:

   use Drupal\views\Plugin\views\query\QueryPluginBase;
   
     function leaflet_dynamic_table_views_query_alter(ViewExecutable $view,
     QueryPluginBase $query): void {

Both are one-line fixes in the .module file. Everything else
is correctly and thoroughly addressed - great work on the round 3 fixes
overall.

I reviewed it again and I can confirm all the issues are fixed now.

Thankyou

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