Needs review
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
16 Sep 2026 at 04:49 UTC
Updated:
19 Oct 2026 at 13:45 UTC
Jump to comment: Most recent
Comments
Comment #2
vishal.kadamComment #3
ajiseco commentedComment #4
avpadernoThank 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.
Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
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.
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.
Comment #5
avpadernoComment #6
avpadernoThe part about similar projects must be given in the project page. That section has been added, but it does not say anything about similar projects.
mortgage_calculators.info.yml
Projects used in these applications are supposed to be compatible only with supported Drupal releases. If the project needs to be be used with older Drupal releases, the code compatible with older Drupal releases should be in a different branch.
In most of the cases, the same code cannot be compatible with, for example, Drupal 9.x and Drupal 11.x. That means that splitting the project in two branches is required most of the times.
src/Form/CalculatorForm.php
Documentation comments for inherited methods or methods defined in interfaces just need the
{@inheritdoc}line. If the parameter descriptions do not say anything that is specific for that method, they can be removed.mortgage_calculators.module
The long description does not say anything that is specific for this module; it can be removed.
Also the short description does not say anything specific for this module, since which Drupal releases support procedural code and which Drupal releases support
#[LegacyHook]is a detail about Drupal core. I would rather use the usual Hook implementations for the [module name] module. where [module name] is the module name reported in the .info.yml file.Comment #7
vishal.kadam1. FILE: src/Entity/Calculator.php
Projects that are compatible with Drupal 10 or higher versions should use attributes instead of annotations.
2. FILE: src/Plugin/Derivative/CalculatorBlockDeriver.php
Modules which are compatible with Drupal 10 and higher versions are expected to use constructor property promotion.
Comment #8
ajiseco commentedThank you both for the reviews. All points are addressed on the 1.x branch. Details below, including two places where I did not do exactly what was asked and want to explain why.
Supported Drupal releases
core_version_requirementis now^11.1, andcomposer.jsonmatches. The previous^10.3 || ^11allowed 10.3, 10.4 and 10.5, none of which are supported.I chose 11.1 rather than 11.0 because that is the release where both things this module relies on arrived: object oriented hook implementations, and attributes for entity type plugins. Raising the floor let me delete the compatibility scaffolding rather than keep it, which resolves the next two points as a side effect.
Attributes instead of annotations
src/Entity/Calculator.phpnow uses#[ConfigEntityType]. The class docblock previously carried a comment claiming the attribute form only existed from 11.4, which was simply wrong, and that error is what had kept the annotation in place. The comment is gone along with the annotation.The procedural hook wrappers
mortgage_calculators.modulehas been deleted entirely rather than reworded. It existed only to carry#[LegacyHook]wrappers for releases before 11.1, so with the new floor it has no purpose.src/Hook/MortgageCalculatorsHooks.phpalready declares both implementations with#[Hook], and the kernel and functional suites confirm nothing stopped being registered.Constructor property promotion
src/Plugin/Derivative/CalculatorBlockDeriver.phpnow promotes the entity type manager in the constructor and the separate property declaration is gone.Documentation comments on inherited methods
This is the first qualification. I removed the prose from the
{@inheritdoc}blocks insrc/Form/CalculatorForm.php, including a paragraph that explained PHPStan internals and had no business being there. I did not remove the@paramlines themselves.The reason is that
phpstan.neonruns at level 6, and that level requires a value type for iterable parameters. With the tags fully removed, PHPStan reportsmissingType.iterableValuefor$formon four methods. I verified this rather than assuming it. So the blocks now carry a bare@param mixed[] $formwith a one line description, and Drupal standards want$form_statedocumented alongside it once$formis. If you would rather see the tags gone entirely, I am happy to drop the PHPStan level instead, but I did not want to lower a quality gate without asking.Similar projects on the project page
Correct, and thank you for catching it. The section existed but described this module rather than comparing it to anything. It now names the three projects and says how this one differs: Real Estate Mortgage Calculator (single calculator, designed for the Real Estate module, no supported stable release), Financial Calculators (marked no longer developed, no supported stable release), and Simple Calculator (last updated 2016, no supported stable release).
Other changes in the same push
While addressing the above I ran the rest of the checks that GitLab CI performs and fixed what they found:
.gitlab-ci.yml. The project had none, so nothing had ever been tested on drupal.org. Previous major is deliberately not opted in, since the branch no longer supports Drupal 10.css/calculators.css, property order, a missing blank line, and fourrgba()calls that core's config wants asrgb(). All fixed.js/math.jsdeclaresglobalThisat file level, followingcore/misc/jquery.form.js, because that file loads both as a Drupal library and as a CommonJS module in the test that proves the JavaScript and PHP produce identical figures..cspell.jsonfor a handful of project specific words.phpstan.neon, the PHPUnit configuration and the test runner. Those were useless to anyone else and should not have been there.Verification
The GitLab CI pipeline for this commit is green on every job: composer, composer-lint, phpcs, phpstan, phpunit, cspell, eslint and stylelint. That is the first pipeline this project has ever run, since it had no CI configuration until now.
Locally on Drupal 11.4.6 I also ran phpcs with Drupal and DrupalPractice, PHPStan at level 6, and the unit, kernel and functional suites, plus a Node test asserting the JavaScript and the PHP produce identical figures from a shared fixture.
One thing I should flag rather than leave implicit: the branch declares
^11.1, but I have only executed the suites against 11.4.6. I confirmed from the change records that both object oriented hooks and entity type attributes arrived in 11.1, but I have not run the tests on 11.1 itself.Setting back to Needs review.
Comment #9
vishal.kadamI am changing priority as per Issue priorities.
Comment #10
ajiseco commentedSimplified and ready for review on branch 2.x (release 2.0.0): one block with five calculator choices, calculations in the browser, Drupal 11.3 and later.
Comment #11
ajiseco commentedComment #12
ajiseco commentedComment #13
avpaderno