[1.0.x] Status Block

Thank you for applying!

Before giving links helpful to understand how the review process works, what to expect from a review, and what to do to avoid a review takes more time than needed, I would like to thank all the reviewers for the work they do.
These applications are volunters-driven, which also means it is not possible to predict when an application will be marked fixed and the applicant will get the permission to opt projects into security advisory policy. While we aim to make an application as quick as possible, it is also important for us that more people review the project used for an application. In this way, we make sure applications do not miss some important points that should be instead reported.
Applications are not meant to be complete debugging sessions that eliminate every existing bug, though. I apologize if sometimes applications seem to go into too-detailed reviews.

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.

Remember to change status, when the project is ready for review, as in this queue Active means Don't review yet the project I am using for this application.

1. main is acceptable as branch name, but it is not yet fully supported on drupal.org. For the moment, it is better to avoid it.

2. FILE: README.md

The README file is missing the required sections - Requirements, Installation, and Configuration.

3. FILE: composer.json

    "require": {
        "drupal/core": "^10 || ^11"
    }

It is also not necessary to add the Drupal core requirements in the composer.json file: The Drupal.org Composer façade will add them from the .info.yml file.

4. FILE: status_block.module

/**
 * @file
 * Hooks implementations for the Text Clarity Checker module.
 */

The module name in the description is wrong. It should be the name of the module given in .info.yml file.

5. FILE: src/Plugin/Block/StatusBlock.php

  /**
   * The StatusBlock plugin manager.
   *
   * @var \Drupal\status_block\StatusBlockDataPluginManager
   */
  private StatusBlockDataPluginManager $statusBlockPluginManager;

  /**
   * The route match component.
   *
   * @var \Drupal\Core\Routing\RouteMatchInterface
   */
  private RouteMatchInterface $routeMatch;

  /**
   * Constructs a \Drupal\Component\Plugin\PluginBase object.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   *   The route_match component.
   * @param \Drupal\status_block\StatusBlockDataPluginManager $status_block_plugin_manager
   *   The StatusBlock plugin manager.
   */
  public function __construct(
    array $configuration,
    $plugin_id,
    $plugin_definition,
    RouteMatchInterface $route_match,
    StatusBlockDataPluginManager $status_block_plugin_manager,
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->routeMatch = $route_match;
    $this->statusBlockPluginManager = $status_block_plugin_manager;
  }

FILE: src/Plugin/StatusBlockData/CurrentTheme.php

  /**
   * The theme manager.
   *
   * @var \Drupal\Core\Theme\ThemeManagerInterface
   */
  private ThemeManagerInterface $themeManager;

  /**
   * The theme handler.
   *
   * @var \Drupal\Core\Extension\ThemeHandlerInterface
   */
  private ThemeHandlerInterface $themeHandler;

  /**
   * Create a new instance.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin ID for the plugin instance.
   * @param array $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
   *   The theme handler.
   * @param \Drupal\Core\Theme\ThemeManagerInterface $theme_manager
   *   The theme manager.
   */
  public function __construct(
    array $configuration,
    string $plugin_id,
    array $plugin_definition,
    ThemeHandlerInterface $theme_handler,
    ThemeManagerInterface $theme_manager,
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);

    $this->themeManager = $theme_manager;
    $this->themeHandler = $theme_handler;
  }

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.

Hi @tleuillier,

You should use camel case for properties, to comply with Drupal's coding standards. Like so:

public function __construct(
  array $configuration,
  $plugin_id,
  $plugin_definition,      
  private readonly RouteMatchInterface $routeMatch,  
  private readonly StatusBlockDataPluginManager $statusBlockPluginManager,
) {
  parent::__construct($configuration, $plugin_id, $plugin_definition);
}

Use property promotion with camelCase even when it creates visual inconsistency with parent parameters. The inconsistency is a temporary artifact of Drupal's evolution, and your code should follow current standards, not legacy patterns.

The visual inconsistency isn't your fault, and it's perfectly fine. It aligns with the coding standards.

I reviewed the 1.0.x branch of status_block (commit 8f5126a).

Previous reviewer issues

- Branch naming (1.0.x): Fixed.
- README sections: Fixed.
- composer.json core requirement removed: Fixed.
- Module docblock: Fixed.
- camelCase promoted properties (Comment #8): Not fully fixed -- `$theme_handler` and `$theme_manager` in `CurrentTheme.php` (lines 43-44) are still snake_case. Also `$display_label` in `StatusBlockData.php` (line 17).

Security issues

1. No access control on the block (`StatusBlock.php` lines 68-69): `blockAccess()` checks for entity_browser routes but has no permission check. The block exposes Drupal version, environment name, and active theme to any user who can view the region, including anonymous users. Should require a permission like `administer site configuration` or a custom permission.

2. Unguarded `$_ENV` access (`StatusBlock.php` line 179, `Environment.php` line 39): `$_ENV[$env_name]` will throw a PHP warning if the environment variable is not set. Most sites won't have the expected variable. Should use `$_ENV[$env_name] ?? ''`.

3. Unsanitized CSS class from environment variable (`StatusBlock.php` line 188): The raw `$_ENV` value is concatenated into a CSS class (`'status-env-' . $env`). Should use `Html::cleanCssIdentifier()`.

Bugs

4. Double hook_help on Drupal 11 (`status_block.module` line 14 + `Help.php` line 12): Both a procedural hook function AND a `#[Hook('help')]` attribute exist, so the hook fires twice on Drupal 11. Remove one or the other.

5. JavaScript bug (`config.js` line 23): `$rowElement.find('select.group-select').value = elem` sets a property on a jQuery object, not the DOM element. Should be `.val(elem)`. The drag-and-drop group reassignment is non-functional.

6. Missing null checks (`StatusBlock.php` lines 176-178): Code assumes `plugin_order['environment']` always exists in configuration. A freshly placed block with no saved config will throw warnings.

Coding standards

7. Loose comparisons (`==` instead of `===`): `StatusBlock.php` lines 124, 380.
8. Double-quoted array keys: `StatusBlock.php` lines 252, 269, 313.
9. Redundant `use StringTranslationTrait` in `Message.php` line 21 (parent already has it).
10. Missing </p> tag in `Help.php` line 33. Help text references "StatusBarData" instead of "StatusBlockData" (line 34).
11. Attribute placed before docblock in `StatusBlockData.php` (lines 8-9) -- should be after.
12. Missing return type on `DataGroup::getOptions()`.
13. `getCacheMaxAge()` returns 0 -- disabling caching entirely is heavy-handed for mostly static data. Consider cache tags/contexts.
14. README "Community Documentation" section says "To be done soon..." -- should be removed or completed.

The module's architecture (custom plugin system, enum, attributes) is well-designed. Once the security and functional issues are resolved, this would be a good candidate. Setting to Needs work.

@oivanov since u are using AI to help drafting your feedback, u could at least make an effort to format the output from Markdown to html or plain text so that people can easily read it...

me personally - I prefer Markdown to everything else, so I make an effort not to have to deal with HTML (definitely) or plain text (when I can).
If markdown is not acceptable - we can add that to the guidance. Let me know if you want me to do that

As a side note: We do not give any guidance about comment format because comments are expected to be formatted here as in any issue queue: HTML or plain text.
Since Markdown markup is not converted to HTML, and HTML is used to render both the comment and the issue page, it is better to stick with HTML or plain text. Otherwise, it happens what happened with point #10 in comment #11, which reads as Missing `` tag. (Yes, a ` character does not escape HTML tags.)

got it! cleaned up the comment above

Fair point that block visibility can be configured per-role. However, since the block exposes system information (Drupal version, environment, theme) that aids site fingerprinting, a default permission check in blockAccess() would follow the secure-by-default principle.

It's a one-line addition that prevents accidental exposure if an admin places the block without configuring role restrictions.

Here are a couple more things to work on:

GitLab CI is properly configured with the standard Drupal template, but there are still some warnings. Try to resolve these

(*) Missing permission check in blockAccess() — StatusBlock.php:69 only excludes entity browser routes but performs no permission check. This module displays server environment information (Drupal version, environment variable values, active theme). Without a default permission in blockAccess(), the block relies entirely on placement configuration for access control. A secure-by-default approach would check for a permission like administer site configuration. The previous reviewer raised this same concern. See Block API: access control.

(*) Unguarded $_ENV access — StatusBlock.php:182 reads $_ENV[$env_name] without a null coalescing fallback. If the configured environment variable does not exist, this triggers a PHP warning and returns null. Notably, Environment.php:39 already uses the correct pattern: $_ENV[$env_name] ?? "". Apply the same fix in the block's build() method.

(+) Arbitrary environment variable exposure — The env_name setting (admin-configurable text field) allows reading any environment variable. An admin could inadvertently expose DATABASE_URL, APP_SECRET, or similar sensitive values if the block is visible to lower-privileged users. Consider documenting this risk prominently in the form description, or validating against an allowlist.

(+) Loose comparison — StatusBlock.php:384 uses == instead of === to compare group values. Since DataGroup is a string-backed enum, use strict comparison for clarity and correctness.

Untranslated string "Not Found" in Version.php:29 — wrap in $this->t().

Also the README could use some attention:

Formatting issues

• Lines 3–4, 48: Text exceeds ~80 columns. Word-wrap for readability.
• Lines 7–40, 51–68: Raw HTML (<ol>, <ul>, <li>) is used instead of Markdown lists. Use dashes (-) for unordered lists and 1. for ordered lists.
• Line 44: Uses asterisk (*) for list — use dashes (-) per drupal.org convention.
• Line 48: Same asterisk issue.
• Missing two blank lines before ## headings (drupal.org convention).

For more info see: drupal.org README template

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No duplication found. This fills a niche for visual environment identification via a block plugin system.

Master Branch

Yes: Follows the guidelines for master branch. Development branch is 1.0.x.

Licensing

Yes: Follows the licensing requirements (GPL-2.0-or-later in LICENSE.txt).

3rd party assets/code

Yes: No bundled third-party libraries. Uses Composer\InstalledVersions which is part of Composer's runtime API.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation.

Code long/complex enough for review

Yes: Block plugin with custom plugin manager, four data plugins, enum, attribute class, form configuration, JS, and CSS.

Secure code

Good progress since prior reviews. No XSS, SQL injection, or CSRF vulnerabilities. The new view status blocks permission properly gates block access via blockAccess(). Environment variable values are rendered through Twig auto-escaping, and CSS class output uses Html::cleanCssIdentifier(). One concern remains: the admin-configurable env_name field (Environment.php:38) allows reading any $_ENV variable, and the value is displayed to all users with view status blocks. The inline warning text is a reasonable mitigation for a development-oriented module, but it should be translatable (see #1 below).

Coding style & Drupal API usage

1. (*) Untranslatable hardcoded HTML — Environment.php:28-33: The warning message about sensitive data exposure uses raw HTML in #markup without $this->t(). Both the <strong> warning and the description paragraph should be wrapped in $this->t() for translation support.
2. (*) Missing null check on plugin_conf access — StatusBlock.php:182: $this->configuration['plugin_conf']['environment']['env_name'] is accessed after checking isset($this->configuration['plugin_order']['environment']['group']), but there is no check that plugin_conf.environment.env_name exists. If the environment plugin is ordered but not yet configured (e.g., imported config or fresh install), this will throw a PHP warning. Use null coalescing or an isset() guard.
3. (+) $_ENV default value inconsistency — StatusBlock.php:184 falls back to "dev" when the env var is unset, but Environment.php:50 falls back to "". This means the CSS class shows status-env-dev while the rendered text shows nothing. Use the same default in both places.
4. (+) getCacheMaxAge() returns 0 — StatusBlock.php:79-81: This disables page caching for any page containing the block. Flagged in prior reviews and acknowledged. Consider using cache contexts (e.g., route) or tags instead of disabling caching entirely, or at minimum document this performance impact in README.
5. Missing declare(strict_types=1) in all module PHP files (only GenericTest.php has it).
6. once imported but unused in config.js:6 — the IIFE receives once as a parameter but never references it. Remove from the function signature and the argument list.

Status: Needs work — Untranslatable strings (#1) and missing null safety (#2) should be addressed.

Just to make clear the point about not duplicating the Drupal core requirements in the composer.json file: Applicants should know that the Drupal.org Composer façade adds the Drupal core requirements for the project when the composer.json file does not contain them. Not repeating them avoids any issue caused by the composer.json file having different Drupal core requirements than the ones contained in the .info.yml file.

I cannot think of any workflow where Composer is not used, given that Composer is required by Drupal core to gather the dependencies it needs. Therefore, I cannot think of any case where the Drupal.org Composer façade is not involved when site builders are requiring the modules necessary for their sites.

(I added the issue tag basing on the review done in the previous comment.)

I am changing priority as per Issue priorities.

src/Plugin/StatusBlockData/CurrentTheme.php

Since the properties are private, the class itself should be final.

status_block.permissions.yml

There is a typo in the warning for the permission: It should be This permission is dangerous, not This permissions is dangerous.

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.