Restore 1.x API compatibility while keeping 1.5 hardening#629
Restore 1.x API compatibility while keeping 1.5 hardening#629SNO7E-G wants to merge 9 commits intogoogle:mainfrom
Conversation
rowan-m
left a comment
There was a problem hiding this comment.
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.
|
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. |
acoulton
left a comment
There was a problem hiding this comment.
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.
|
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>
|
Thanks for the feedback — I updated the PR to restore 1.4.x compatibility while keeping the safe reliability fixes from 1.5.0. |
|
The problem is that the typehints broke semantic versioning. v1.5 really should've been v2.0.. |
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
readonlybehavior from public non-final DTOs (Response,RequestParameters) so extension/mocking remains possible.RequestMethod::submit()without a native return type (1.x-compatible contract).ReCaptcha::verify()to safely handle non-string request-method responses (E_BAD_RESPONSE).verify(...).new CurlPost(null, $url)new SocketPost(null, $url)CurlandSocketwrapper classes as public compatibility surfaces.2) Fixes compatibility/quality edge cases found during review
SocketPostearly-return path to ensure handle close when timeout setup fails.Response::fromJson(...)semantics needed for 1.x consumers.3) Keeps the good hardening/reliability improvements
SocketPostHTTP/1.1 handling remains in place.ReCaptcharemains."0"response handling remains valid.Tests and coverage updates
Added/updated regression tests for:
RequestMethodimplementation without native return typeRequestMethodresponse handlingverify(null)compatibility behavior"0"response handlingResponseandRequestParametersCurl/Socket)Documentation updates
Validation run
vendor/bin/phpunit --no-coveragevendor/bin/phpstanvendor/bin/php-cs-fixer check --using-cache=noOnly caution (not new, but important):
Note to @rowan-m
I already bumped the client version string to
1.5.1in 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.