Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add missing types #202

Open
wants to merge 1 commit into
base: 3.6.x
Choose a base branch
from
Open

Add missing types #202

wants to merge 1 commit into from
+10 −11

Conversation

ADmad
Copy link
Contributor

@ADmad ADmad commented Nov 11, 2024

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

Some properties and methods were missing type declarations.

gsteel
gsteel requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @ADmad but there are a few BC breaks that need reverting

src/MessageTrait.php Outdated
Comment on lines 33 to 42
* @var array
* @psalm-var array<non-empty-string, list<string>>
*/
protected $headers = [];
protected array $headers = [];

/**
* Map of normalized header name to original name used to register header.
*
* @var array
* @psalm-var array<non-empty-string, non-empty-string>
*/
protected $headerNames = [];
protected array $headerNames = [];
Copy link
Member

Choose a reason for hiding this comment

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

Adding types to protected properties is a BC break, also, removing @var array is fine - it would be good to rename the @psalm-var annotations to @var - the psalm prefix is unnecessary

src/Response/JsonResponse.php Outdated
* @return mixed
*/
public function getPayload()
public function getPayload(): mixed
Copy link
Member

Choose a reason for hiding this comment

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

Adding the return type is a BC break for inheritors: https://3v4l.org/BH4qV

src/Uri.php Outdated
@@ -53,7 +53,7 @@ class Uri implements UriInterface, Stringable
public const CHAR_UNRESERVED = 'a-zA-Z0-9_\-\.~\pL';

/** @var int[] Array indexed by valid scheme names to their corresponding ports. */
protected $allowedSchemes = [
protected array $allowedSchemes = [
Copy link
Member

Choose a reason for hiding this comment

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

Adding types to protected members is a BC break: https://3v4l.org/frBrF

The documented type could be improved with /** @var array<string, positive-int> */

@ADmad
Copy link
Contributor Author

ADmad commented Nov 12, 2024

All these are private properties, shouldn't it be safe to change them?

[BC] CHANGED: Type of property Laminas\Diactoros\RequestTrait#$protocol changed from having no type to string
[BC] CHANGED: Type of property Laminas\Diactoros\RequestTrait#$stream changed from having no type to Psr\Http\Message\StreamInterface
[BC] CHANGED: Type of property Laminas\Diactoros\RequestTrait#$method changed from having no type to string
[BC] CHANGED: Type of property Laminas\Diactoros\RequestTrait#$requestTarget changed from having no type to string|null
[BC] CHANGED: Type of property Laminas\Diactoros\RequestTrait#$uri changed from having no type to Psr\Http\Message\UriInterface
[BC] CHANGED: Type of property Laminas\Diactoros\MessageTrait#$protocol changed from having no type to string
[BC] CHANGED: Type of property Laminas\Diactoros\MessageTrait#$stream changed from having no type to Psr\Http\Message\StreamInterface

Constructors can be changed with impunity, changing the type shouldn't cause any error even if the constructor was overridden in a child class.

[BC] CHANGED: The parameter $data of Laminas\Diactoros\Response\JsonResponse#__construct() changed from no type to mixed

@gsteel
Copy link
Member

gsteel commented Nov 12, 2024

The change in JsonResponse constructor is fine IMO, but it is a technical BC break, just because PHP lets us change the signature of __construct, it's still part of the public API.

The private property changes to the traits are indeed a BC break if users have re-implemented a property without a type hint: https://3v4l.org/aaQTo - if they were class properties, it'd be fine, so very little of this can go into a 3.x release

@ADmad
Copy link
Contributor Author

ADmad commented Nov 12, 2024

Thank you for the trait example, I'll revert the related changes.

I am still not convinced the constructor change is a BC break :)
The arguments aren't being changed, just the untyped argument is explicitly typed as mixed. I can't think of any practical use case which would generate an error with this change.

@ADmad ADmad force-pushed the property-types branch 2 times, most recently from 3d44e31 to 5244e00 Compare November 12, 2024 15:53
@ADmad
Add missing types
88ac87d 
Signed-off-by: ADmad <[email protected]>
@ADmad
Copy link
Contributor Author

ADmad commented Nov 18, 2024

I don't think the test failures are related to the changes in this PR.

@gsteel
Copy link
Member

gsteel commented Nov 18, 2024 •
edited
Loading

No, but whatever they are, they'll need to be addressed separately 🤔

@gsteel
Copy link
Member

gsteel commented Nov 18, 2024

See #205

@ADmad
Copy link
Contributor Author

ADmad commented Nov 18, 2024

That's not a "me" problem, so I'll let you guys tackle that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsteel gsteel gsteel requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ADmad @gsteel