Mortgage Calculators provides one Drupal block with five calculator choices:

  • Mortgage payment: principal and interest, property tax, homeowners insurance, HOA dues, conventional PMI and FHA options
  • Home affordability
  • Loan: auto loans and other fixed payment loans
  • Amortization schedule with an extra payment comparison
  • Refinance: monthly savings and break even month

Each block placement keeps its own default values, set in the block configuration with validation. Visitors calculate in their browser; entries are not submitted or stored.

The module needs nothing beyond Drupal core: no Composer package, no external service, no CDN, and no third party JavaScript library. JavaScript is required for calculations. A fixture of test vectors, tests/fixtures/vectors.json, drives the JavaScript tests, so results stay identical to 1.x.

Manual reviews of other projects

Project link

https://www.drupal.org/project/mortgage_calculators

Comments

ajiseco created an issue. See original summary.

vishal.kadam’s picture

Issue summary: View changes
ajiseco’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
avpaderno’s picture

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.

avpaderno’s picture

Assigned: Unassigned » avpaderno
avpaderno’s picture

Assigned: avpaderno » Unassigned
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus
  • The following points are just a start and don't necessarily encompass all of the changes that may be necessary
  • A specific point may just be an example and may apply in other places
  • A review is about code that does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
  • The single review points are not ordered, not even by importance

The 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

  /**
   * {@inheritdoc}
   *
   * @param mixed[] $form
   *   The form array. Not narrowed: it is passed by reference, and the nested
   *   settings subform it carries is a plain array.
   * @param \Drupal\Core\Form\FormStateInterface $form_state
   *   The current form state.
   */

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

/**
 * @file
 * Procedural hook wrappers for Drupal 10.3 through 11.0.
 *
 * Drupal 11.1 and later discover the attribute-based implementations in
 * \Drupal\mortgage_calculators\Hook\MortgageCalculatorsHooks and skip every
 * function marked with #[LegacyHook], so no hook runs twice.
 *

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.

vishal.kadam’s picture

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

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

  /**
   * Constructs the deriver.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
    $this->entityTypeManager = $entity_type_manager;
  }

Modules which are compatible with Drupal 10 and higher versions are expected to use constructor property promotion.

ajiseco’s picture

Status: Needs work » Needs review

Thank 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_requirement is now ^11.1, and composer.json matches. The previous ^10.3 || ^11 allowed 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.php now 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.module has 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.php already declares both implementations with #[Hook], and the kernel and functional suites confirm nothing stopped being registered.

Constructor property promotion

src/Plugin/Derivative/CalculatorBlockDeriver.php now 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 in src/Form/CalculatorForm.php, including a paragraph that explained PHPStan internals and had no business being there. I did not remove the @param lines themselves.

The reason is that phpstan.neon runs at level 6, and that level requires a value type for iterable parameters. With the tags fully removed, PHPStan reports missingType.iterableValue for $form on four methods. I verified this rather than assuming it. So the blocks now carry a bare @param mixed[] $form with a one line description, and Drupal standards want $form_state documented alongside it once $form is. 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:

  • Added .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.
  • stylelint reported nine issues in css/calculators.css, property order, a missing blank line, and four rgba() calls that core's config wants as rgb(). All fixed.
  • eslint reported formatting issues in both JavaScript files, now clean. js/math.js declares globalThis at file level, following core/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.
  • Added .cspell.json for a handful of project specific words.
  • Removed several absolute paths from my own machine that had been committed in 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.

vishal.kadam’s picture

Priority: Normal » Major

I am changing priority as per Issue priorities.

ajiseco’s picture

Title: [1.x] Mortgage Calculators » [2.x] Mortgage Calculators
Issue summary: View changes

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

ajiseco’s picture

Issue summary: View changes
ajiseco’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
avpaderno’s picture

Issue summary: View changes