Skip to content

refactor: use readonly classes with promoted constructors for DTOs#618

Merged
rowan-m merged 2 commits intogoogle:mainfrom
SNO7E-G:refactor/readonly-dto-constructors
Mar 30, 2026
Merged

refactor: use readonly classes with promoted constructors for DTOs#618
rowan-m merged 2 commits intogoogle:mainfrom
SNO7E-G:refactor/readonly-dto-constructors

Conversation

@SNO7E-G
Copy link
Copy Markdown
Contributor

@SNO7E-G SNO7E-G commented Mar 27, 2026

Hi!

Since the project bumped its requirement to PHP 8.4, I noticed an easy win to clean up a lot of boilerplate in the two DTOs (RequestParameters and Response).

Right now, both classes declare every property manually with PHPDoc and then assign them one by one inside the constructor. I've updated both to use native PHP constructor property promotion, which squashes all those property assignments into a clean, single-signature constructor.

I also added the readonly class modifier to both. Since these objects hold request data and API responses, they really shouldn't ever be mutated once they're built. This locks them down at the engine level, so no one can accidentally modify them later.

Note: I didn't touch a single method or getter logic. toArray(), fromJson(), etc., are all the same, and the constructor signatures haven't changed, so anything calling these classes won't notice a difference.

I also added two quick ReflectionClass tests just to prove the readonly constraint is working properly.

Tests are perfectly green, and it deletes over 60 lines of unnecessary repetition! Let me know what you think.

What Changed

File Lines Summary
src/ReCaptcha/RequestParameters.php +5 / -24 readonly class + pro constructor
src/ReCaptcha/Response.php +12 / -52 readonly class + promoted constructor
tests/ReCaptcha/RequestParametersTest.php +6 / -0 Added testClassIsReadonly Reflection API check
tests/ReCaptcha/ResponseTest.php +6 / -0 Added testClassIsReadonly Reflection API check

All 68 tests pass, 183 assertions, PHPStan Level Max clean.

/**
* Constructor.
*
* @param array<string> $errorCodes
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, let's also keep the comments which are informative.

Suggested change
* @param array<string> $errorCodes
* @param bool $success Success or failure.
* @param array<string> $errorCodes Error code strings.
* @param string $hostname The hostname of the site where the reCAPTCHA was solved.
* @param string $challengeTs Timestamp of the challenge load (ISO format yyyy-MM-dd'T'HH:mm:ssZZ).
* @param string $apkPackageName APK package name.
* @param ?float $score Score assigned to the request.
* @param string $action Action as specified by the page.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, let's also keep the comments which are informative.

Hey @NiklasBr, totally agree — those descriptions are genuinely useful, especially for the less obvious params like $challengeTs and $score. I've updated the constructor PHPDoc to include all the descriptive @param tags. Done!

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 27, 2026

Coverage Status

coverage: 100.0%. remained the same
when pulling 049637d on SNO7E-G:refactor/readonly-dto-constructors
into ff0beab on google:main.

@rowan-m rowan-m self-requested a review March 27, 2026 09:50
@rowan-m rowan-m self-assigned this Mar 27, 2026
@SNO7E-G SNO7E-G force-pushed the refactor/readonly-dto-constructors branch from cc472ad to e76864b Compare March 27, 2026 18:21
Copy link
Copy Markdown
Contributor

@rowan-m rowan-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes make sense, but I'm not convinced on the addition of testClassIsReadonly(). This feels like it's testing implementation detail, not behaviour.

@SNO7E-G
Copy link
Copy Markdown
Contributor Author

SNO7E-G commented Mar 29, 2026

The code changes make sense, but I'm not convinced on the addition of testClassIsReadonly(). This feels like it's testing implementation detail, not behaviour.

Hey @rowan-m, fair point! Testing if a class is readonly is definitely overkill and just tests PHP itself.

I've dropped those tests in the latest commit. Thanks for keeping the suite focused!

@SNO7E-G SNO7E-G requested a review from rowan-m March 29, 2026 20:40
@rowan-m rowan-m merged commit a6c5041 into google:main Mar 30, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants