Restore 1.x API compatibility while keeping 1.5 hardening

[SNO7E-G]

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.

---

[rowan-m]

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]

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.

[coveralls]

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

[acoulton]

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.

[SNO7E-G]

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.

[SNO7E-G]

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.

[jonnott]

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

[garak]

Sorry for the "bump-like" comment, but is there any way to move this forward? Are there some blockers?

[SNO7E-G]

Thanks @acoulton, @rowan-m and @jonnott for the review — and sorry for the delay.

You were right that my first pass over-corrected: it coerced parameter/property values,
which wasn't 1.4.x behaviour and actually introduced a fail-open
(setExpectedHostname(new stdClass()) was coerced to '', so verify() silently passed
with hostname pinning disabled). I've reworked the branch from scratch to fix that.

What the latest commit delivers

The goal is now one sentence: the public API is byte-for-byte compatible with the 1.4.2
tag, plus 1.5.0's security hardening — nothing else. git diff 1.4.2 -- src on the public
surface is empty except the VERSION bump.

• Restored to exact 1.4.2 behaviour: untyped public signatures (phpdoc only), non-readonly
  DTOs, the Curl/Socket wrappers and their DI constructors, no return type on the
  RequestMethod::submit() interface, and raw assignment in the setters/DTOs — the coercion
  helpers are gone (floatval kept only in setScoreThreshold, as in 1.4.2). The fail-open
  is gone; every path in verify() now fails closed.
• Kept from 1.5.0 (internal, non-breaking): TLS peer + hostname verification and timeouts,
  URL validation + HTTP/1.1 handling + body-part guards in SocketPost, and fail-closed
  is_string() guards on the transport response.
• Fixes Function curl_close() is deprecated since 8.5, as it has no effect since PHP 8.0 #630 / PHP 8.5 compatibility: deprecation warnings #632: curl_close() is no longer called, and I checked the tree for the
  other 8.5 deprecations listed in PHP 8.5 compatibility: deprecation warnings #632 (implicit-nullable params, __wakeup,
  $http_response_header, long-form casts) — none are present, so the library adds no
  deprecation noise on 8.5.
• Green: phpstan level: max 0 errors, phpunit 52 passing, php-cs-fixer clean. The 1.4.x
  dependency-injection tests are restored, which also proves the documented mock-injection
  extension point still works.

(One transparency note: phpstan.neon now sets treatPhpDocTypesAsCertain: false, still at
level: max — it lets the defensive runtime guards pass on the untyped-but-documented public
API without re-adding native types that would break BC.)

Next steps

• A follow-up PR will prepare V2 — the enhancements and advanced/modernized surface (native
  types, readonly, promoted constructors, and further improvements) as a stable major release.
  Today's main is already most of that; it just belongs under a proper major.
• I'd suggest we set up the usual 1.x + 2.x/main branches soon: 1.x as a
  maintenance line (security, critical fixes, and PHP-version compatibility only — no new
  features) and 2.x for active development. This PR heals every ^1 consumer on their next
  composer update via 1.5.1, while V2 gives everyone a clean path forward.

Very open to other opinions on any of this at any point — happy to adjust the numbering,
branching, or scope to whatever the maintainers prefer.

[acoulton]

@SNO7E-G thanks for all the work on this. Obviously it really depends what @rowan-m thinks, as I'm just a user, not a maintainer here.

But IMO this PR is very hard to review, because it is partially unwinding changes that were already merged and released, but also introducing or keeping new things. This means there are lots of things in the diff that look odd, but may be correct. Without a line-by-line comparison between 1.4.2 and the merged result of this PR, it's impossible to tell whether it correctly fixes the BC breaks that were introduced in 1.5.0 without introducing any other problems. I don't really have time to do that.

If it was my project, I'd be strongly encouraging splitting this into separate things:

• First, a PR that reverts everything that went into 1.5.0, to allow for a 1.5.1 release that is basically identical to 1.4.2.
• Ideally, a PR that introduces https://github.com/Roave/BackwardCompatibilityCheck or similar tooling to detect BC breaks in future PRs. This is probably more robust and clearer than introducing unit tests to cover specific bits of BC.
• Then, one or more PRs that introduce just the hardening and other improvements from 1.5.0, breaking them into reviewable units and targeting them to either 1.6.0 (if they maintain BC) or 2.0.0 as appropriate.

[SNO7E-G]

Thanks, @acoulton — that's really helpful feedback — and a fair criticism. You're right that the PR is doing two things at once, and that makes it hard to review even where the individual changes are fine.

I'm reworking it along the lines you suggested: first a PR that just restores the 1.4.2 API, so git diff 1.4.2 -- src comes back empty and there's effectively nothing to read; then the BC-check tooling; then the hardening from 1.5.0 split into small, self-contained PRs. I'll open them one at a time over the next few days rather than all at once, so each one can be looked at on its own.

Two things I ran into that are worth mentioning in advance. 1.4.2 predates the phpstan level max setup that's now in CI and reports quite a few pre-existing errors, several of which can't be fixed with annotations alone — so the restore PR carries a phpstan baseline rather than changing code in a revert, and the later PRs empty it again. And while going through the Post transport, I noticed verify_peer is set in the http section of the stream context, where it doesn't actually do anything, so that one is a real fix rather than just a port from 1.5.0.

Thanks again for taking the time to look at it.