Skip to content

Restore 1.x API compatibility while keeping 1.5 hardening#629

Open
SNO7E-G wants to merge 9 commits intogoogle:mainfrom
SNO7E-G:fix/1x-compatibility
Open

Restore 1.x API compatibility while keeping 1.5 hardening#629
SNO7E-G wants to merge 9 commits intogoogle:mainfrom
SNO7E-G:fix/1x-compatibility

Conversation

@SNO7E-G
Copy link
Copy Markdown
Contributor

@SNO7E-G SNO7E-G commented Apr 29, 2026

Closes #628

Thanks again, @acoulton and @mbabker, for raising and detailing the BC concerns.
This PR focuses on restoring 1.x compatibility expectations while preserving the useful reliability and hardening work already introduced.

What this PR does

1) Restores 1.x public API compatibility

  • Removed readonly behavior from public non-final DTOs (Response, RequestParameters) so extension/mocking remains possible.
  • Kept RequestMethod::submit() without a native return type (1.x-compatible contract).
  • Added runtime guard in ReCaptcha::verify() to safely handle non-string request-method responses (E_BAD_RESPONSE).
  • Relaxed public scalar signature strictness where 1.x behavior expected broader input, especially verify(...).
  • Restored legacy constructor compatibility for:
    • new CurlPost(null, $url)
    • new SocketPost(null, $url)
  • Preserved source-compatible constructor parameter names for named-argument usage.
  • Restored Curl and Socket wrapper classes as public compatibility surfaces.

2) Fixes compatibility/quality edge cases found during review

  • Fixed SocketPost early-return path to ensure handle close when timeout setup fails.
  • Ensured old constructor override behavior does not silently drop custom siteverify URLs.
  • Preserved legacy-compatible Response::fromJson(...) semantics needed for 1.x consumers.

3) Keeps the good hardening/reliability improvements

  • cURL handle cleanup remains in place.
  • SocketPost HTTP/1.1 handling remains in place.
  • Explicit nullable property initialization in ReCaptcha remains.
  • "0" response handling remains valid.
  • TLS peer verification hardening in fallback request flow remains.
  • PHPStan / tooling modernization remains.

Tests and coverage updates

Added/updated regression tests for:

  • custom RequestMethod implementation without native return type
  • non-string RequestMethod response handling
  • verify(null) compatibility behavior
  • "0" response handling
  • numeric-string score threshold usage
  • legacy constructor forms for CurlPost/SocketPost
  • extendability of Response and RequestParameters
  • socket timeout failure close path
  • wrapper compatibility behavior (Curl / Socket)

Documentation updates

  • Added/expanded Public API compatibility guidance.
  • Clarified 1.x public API expectations and major-version-only change types.
  • Added a compatibility release notes section for 1.5.1 behavior.

Validation run

  • vendor/bin/phpunit --no-coverage
  • vendor/bin/phpstan
  • vendor/bin/php-cs-fixer check --using-cache=no

Only caution (not new, but important):

  • CurlPost / Post / SocketPost still allow overriding the verify URL for compatibility. That is fine when it’s trusted config, but if any app passes user-controlled input there, it can become SSRF-like behavior.
  • Example files intentionally use raw request data for demo purposes; that is expected and not library-core behavior.

Note to @rowan-m

I already bumped the client version string to 1.5.1 in this work to keep the release path straightforward and avoid extra follow-up edits.
If that decision should stay maintainer-only, I’m sorry — please feel free to adjust/revert that part as you prefer.


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.

Thank you for jumping on this.

Quick initial thoughts:

I think type hints can stay because the minimum PHP version remains at 8.4.

The build fails due to a strict composer check as there are changes to composer.json without an updated lock file - even though the changes don't affect the dependencies. Not sure on the best approach there... probably just update the lock file as I should do a separate update of the dependencies after this PR.

@SNO7E-G
Copy link
Copy Markdown
Contributor Author

SNO7E-G commented May 2, 2026

Thanks for the feedback @rowan-m. That makes sense. Since the minimum PHP version is still 8.4, I agree that the type hints can stay.

The Composer failure was my oversight; I hadn’t run composer update after the composer.json changes, so the lock file was out of sync. I’ve refreshed it now and rerun the checks, so that part should be sorted.

@SNO7E-G SNO7E-G requested a review from rowan-m May 2, 2026 13:02
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 94.737% (-5.3%) from 100.0% — SNO7E-G:fix/1x-compatibility into google:main

Copy link
Copy Markdown

@acoulton acoulton left a comment

Choose a reason for hiding this comment

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

Thanks - I had a quick look through and noticed a few things that don't look quite right to me.

@rowan-m: I think type hints can stay because the minimum PHP version remains at 8.4.

It depends which type hints you mean?

Return type hints are a definite BC break.

Parameter type hints will potentially break people's runtime usage. Whether you consider that a BC break depends how strictly you think people should have been following/checking the phpdoc type hints in the past (and whether they have declare(strict_types=1) in their calling code. I'd suggest as a minimum verify should take a nullable string.

Property type hints may break people's usage if they are not strictly following the phpdoc of constructor params, as above.

IMO however this PR goes too far in trying to coalesce parameter/property values that would have been invalid/cause errors in the past.

Comment thread src/ReCaptcha/ReCaptcha.php Outdated
Comment thread src/ReCaptcha/ReCaptcha.php
Comment thread src/ReCaptcha/RequestMethod.php Outdated
Comment thread src/ReCaptcha/RequestMethod/Curl.php Outdated
Comment thread src/ReCaptcha/RequestParameters.php
Comment thread src/ReCaptcha/Response.php
Comment thread tests/ReCaptcha/ReCaptchaTest.php Outdated
@SNO7E-G
Copy link
Copy Markdown
Contributor Author

SNO7E-G commented May 3, 2026

Hi @acoulton, you were right on this one. I missed the fact that the goal here should be to preserve the 1.4.x behavior, not to make the API “more helpful” by coercing inputs or changing how errors surface.

I wasn’t trying to change the behavior for the sake of it. My thinking was that the stricter typing and the extra checks would make the code safer and easier to use, but in doing that, I crossed the line from compatibility into behavior changes. That was the wrong tradeoff for this release.

I’ve been working through the fixes since your comment, and I’m updating the implementation now so it matches the original behavior more closely while still keeping the reliability improvements that don’t affect BC. I’ll push the changes shortly.

Thanks for the comments.

Co-authored-by: Copilot <copilot@github.com>
@SNO7E-G
Copy link
Copy Markdown
Contributor Author

SNO7E-G commented May 3, 2026

Thanks for the feedback — I updated the PR to restore 1.4.x compatibility while keeping the safe reliability fixes from 1.5.0.
In short: I restored legacy input handling, ensured non-string JSON returns E_INVALID_JSON, removed the deprecated curl_close() call, added a regression test, and updated docs/examples for clarity. I ran the test suite, PHPStan, and PHP CS Fixer locally and everything is green. Please take another look and let me know if you want any changes.

@SNO7E-G SNO7E-G requested a review from acoulton May 3, 2026 17:31
@jonnott
Copy link
Copy Markdown

jonnott commented May 5, 2026

The problem is that the typehints broke semantic versioning. v1.5 really should've been v2.0..

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.

v1.5.0 contains breaking API changes

5 participants