v1.5.0 contains breaking API changes

[acoulton]

Hi, first off - thanks for all the work that's been done recently to modernise the library.

However, I wanted to flag that the 1.5.0 release contains a number of breaking changes to the PHP API - and therefore I would have expected it to be 2.0.0.

These include:

• Changing the constructor signatures of the CurlPost and SocketPost classes in test: verify default request method fallback #598 (and deleting the wrapper classes that they previously took as their first argument). This will be a BC break for anyone who is creating either of those RequestMethod classes manually to pass into the Recaptcha constructor if they are passing arguments.
• Adding strict return types to methods throughout in 132d9a5 - including for the RequestMethod interface. These will be a BC break for anyone who has extended those methods or implemented that interface, as PHP will now enforce that the child method must have a strict return type that is compatible with the return type of the parent.
• Making non-final classes readonly in refactor: use readonly classes with promoted constructors for DTOs #618 - this will be a BC break for anyone who had extended that class (either for actual use, or for tests) because a readonly class can only be extended by a readonly class, and vice-versa.

Some of these may be more theoretical issues, or edge cases that only affect people using the library in unexpected ways. However, none of the affected code is e.g. marked as @internal or otherwise defined as not being part of the public API.

In particular, ARCHITECTURE.md specifically says that end-users can create their own implementations of RequestMethod - these will be broken in 1.5.0 as above (unless the user-implementation already has a strict return type).

Environment
All environments.

Proposed resolution

The safest approach would be to:

• publish a new 2.0 based on the current 1.5.0
• then revert the changes that went into 1.5.0 (either just the breaking ones, or all of them) and publish a new 1.5.1.

It might also be a good opportunity to document what is / isn't expected to keep backwards compatibility as part of the public API in future?

Thanks

[rowan-m]

Fair points. I'll have a think through this in a bit more detail. I had initially not gone for a 2.0 release as I felt the changes were more internal, but perhaps the BC break is big enough to merit it.

Was this something that caused an issue for you directly or just something you noticed?

[acoulton]

Thanks @rowan-m - I think the change to the RequestMethod interface definitely seems to be external, I agree the others are a bit more open to interpretation (but I'd lean towards treating things as external unless they're clearly marked as being internal).

Was this something that caused an issue for you directly or just something you noticed?

One of our projects failed in CI on the dependency update job.

As it happens that was actually on the change to add strict parameter types to verify(string $response). We had code like ->verify($request->post('g-recapatcha-response')) and the framework's method gives null when the POST field is not present. But arguably we should already have been coalescing that to empty string to match the original phpdoc @param string $response.

That prompted me to take a look at what else had changed.

[mbabker]

The changes to readonly classes broke some integration tests for me in a Symfony application.

3) Internal\RecaptchaBundle\Tests\Validator\Constraints\ValidCaptchaValidatorTest::testValidWhenValueIsValidated
PHPUnit\Framework\MockObject\ClassIsReadonlyException: Class "ReCaptcha\Response" is declared "readonly" and cannot be doubled

/tools/InternalRecaptchaBundle/tests/Validator/Constraints/ValidCaptchaValidatorTest.php:77

And the relevant snippet from a test class extending Symfony's Symfony\Component\Validator\Test\ConstraintValidatorTestCase:

/** @var MockObject&Response $recaptchaResponse */
$recaptchaResponse = $this->createMock(Response::class);
$recaptchaResponse->expects($this->once())
    ->method('isSuccess')
    ->willReturn(true);

/** @var MockObject&ReCaptcha $recaptchaResponse */
$recaptcha = $this->createMock(ReCaptcha::class);
$recaptcha->expects($this->once())
    ->method('verify')
    ->with('recaptcha-value', $request->getClientIp())
    ->willReturn($recaptchaResponse);

$this->validator->validate('recaptcha-value', $constraint);

$this->assertNoViolation();

Granted, I can rewrite this test to not mock the ReCaptcha\ReCaptcha class and use a mocked ReCaptcha\RequestMethod implementation to get around the readonly response class issue, but this is still quite the unexpected B/C break in a library that I'm assuming defaults to Semantic Versioning and didn't publish this last release as a major version bump.

[SNO7E-G]

Hi,
I went back through the current code and the changes I contributed, and I think the compatibility concern is valid.

The strongest examples are the documented RequestMethod extension point, the stricter public method signatures, and now @mbabker’s Symfony/PHPUnit example showing that readonly Response breaks real integration tests. For the readonly DTO change in #618, that one is on me. It is a reasonable design for a future major version, but it was too aggressive for a public non-final class in the 1.x line.

I also think we should separate the good hardening work from the BC-breaking API changes. I would keep:

• the curl_close() cleanup;
• the HTTP/1.1 SocketPost proxy fix;
• explicit nullable property initialization in ReCaptcha;
• the "0" response handling fix;
• fallback TLS verification hardening;
• PHPStan and CI/tooling modernization.

But I think a 1.5.1 compatibility PR should restore the 1.x public API expectations:

• remove readonly from public non-final DTO classes;
• restore RequestMethod::submit() compatibility and validate the return value inside ReCaptcha;
• relax public scalar parameter types where 1.x accepted broader input, especially verify();
• restore old CurlPost / SocketPost constructor compatibility;
• consider restoring the deleted Curl and Socket wrapper classes if we treat them as public API;
• add regression tests for custom RequestMethod, verify(null), old request method constructor usage, and subclassing/doubling Response.

While checking this, I also noticed two extra compatibility/quality details not yet mentioned: old calls like new CurlPost(null, $url) now silently ignore $url, and SocketPost can return early after stream_set_timeout() fails without closing the opened handle.

My suggested path is not to drop the modernization work, but to move strictness behind the public API boundary for 1.x. Then a future 2.0 can intentionally introduce strict public signatures, readonly/final DTOs, and removed legacy constructors with migration notes.

@rowan-m, if this matches how you want to handle 1.x compatibility, I can prepare the compatibility PR and include the regression coverage.

[rowan-m]

@SNO7E-G if you're happy to then that would definitely be appreciated!

@acoulton I'm not sure about reverting 1.5.0 as it's out there, but I'm more inclined to treat it like a bug fix, release 1.5.1 with the backwards compatibility issues addressed, and then release 2.0.0 with the current functionality.

[acoulton]

@SNO7E-G if you're happy to then that would definitely be appreciated!

Thanks @SNO7E-G!

@acoulton I'm not sure about reverting 1.5.0 as it's out there, but I'm more inclined to treat it like a bug fix, release 1.5.1 with the backwards compatibility issues addressed, and then release 2.0.0 with the current functionality.

Just to be clear, I wasn't suggesting reverting the 1.5.0 release.

Just to revert the changes (e.g. maybe in a 1.5.x branch starting from the current 1.5.0) and tag that as a new 1.5.1 - as the quickest way to get a release people can upgrade to without the BC breaks.

And then releasing a 2.0.0 with all the changes.

But if @SNO7E-G is able to selectively revert only the things that break BC that would obviously be better.

[rowan-m]

@acoulton gotcha, makes sense.

[SNO7E-G]

Working on it.

[jorgsowa]

Please improve the quality of the release notes for the next release. Putting everything under the section What's Changed is not transparent. And putting a breaking changes section in the release notes would be very helpful for maintainers.