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.

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

Add mixed parameter and return types to FilterInterface #111

Merged
merged 6 commits into from
Nov 6, 2023

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Nov 1, 2023

Q A
BC Break yes
QA yes

Description

Adds parameter and return type of mixed to FilterInterface::filter() and all implementations.

Some additional property/parameter/return types have been added but avoided where possible.

The plan here is to add types everywhere in another patch later on.

@gsteel gsteel added this to the 3.0.0 milestone Nov 1, 2023
@gsteel gsteel changed the title Mixed types for filter interface Add mixed parameter and return types to FilterInterface Nov 1, 2023
@gsteel gsteel requested a review from Ocramius November 1, 2023 21:06
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Some changes look like scope creep in here :D

Comment on lines 677 to 679
<InvalidArgument>
<code>array_values($processedParts)</code>
</InvalidArgument>
Copy link
Member

Choose a reason for hiding this comment

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

Anything worth investigating here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah. This is psalm better understanding how little it understands this existing code. My current plan is to get native types everywhere and whittle down the baseline as much as poss for v3, so it's something I should get to later on.

src/AbstractDateDropdown.php Show resolved Hide resolved
@@ -85,7 +85,7 @@ public function getList()
*
* Will return $value if its present in the white-list. If $value is rejected then it will return null.
*/
public function filter($value)
public function filter(mixed $value): mixed
Copy link
Member

Choose a reason for hiding this comment

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

Could be refined to return one of the values of the specified list, or null

@@ -206,9 +206,8 @@ public function getTranslations()
* Returns a boolean representation of $value
*
* @param null|array|bool|float|int|string $value
Copy link
Member

Choose a reason for hiding this comment

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

Lies :D

*/
public function filter($value)
public function filter(mixed $value): mixed
{
if (! is_string($value) && $value !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

wait, we decompress null? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have to look closer at this later on, but I think that some of the adapters only work on files passed as options (Those adapters might be dead now), therefore there's no argument to filter(), or something like that…

@gsteel
Copy link
Member Author

gsteel commented Nov 2, 2023

@Ocramius

Some changes look like scope creep in here :D

Not sure I understand this… This patch is just meant to nail down the contract of filter() to filter(mixed): mixed with minimal other changes. I'm planning on going through the filters after and adding BC breaking types everywhere, along with final and improving type inference where possible. Just trying my hardest to keep the patch size as small as possible 😬

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

@gsteel my bad: I think I initially reviewed this patch with the mindset that it was for a stable branch, but I think all is good here.

@gsteel gsteel force-pushed the mixed-types-for-filter-interface branch from 1f56f4e to 58aae6c Compare November 6, 2023 11:40
@gsteel gsteel self-assigned this Nov 6, 2023
@gsteel gsteel merged commit f32298e into laminas:3.0.x Nov 6, 2023
11 checks passed
@gsteel gsteel deleted the mixed-types-for-filter-interface branch November 6, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants