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

Make options optional again #66

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Make options optional again #66

merged 3 commits into from
Sep 4, 2024

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Sep 2, 2024 •
edited
Loading

This was done incorrectly in #65
/cc @dbu

Parameter #3 $options of method Http\Message\MultipartStream\MultipartStreamBuilder::addResource() expects array{headers: array<string, string>, filename: string}, array{filename: string} given.

@ruudk
Make options optional again
f0444ae 
This was done incorrectly in php-http#65
@ruudk
Copy link
Contributor Author

ruudk commented Sep 4, 2024

@dbu what about this one ?

dbu
dbu reviewed Sep 4, 2024
View reviewed changes
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

oh, absolutely, sorry about that one.

src/MultipartStreamBuilder.php Show resolved Hide resolved
dbu
dbu requested changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

cs fixer wants to align

src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
@dbu dbu merged commit 276eb91 into php-http:1.x Sep 4, 2024
13 checks passed
@dbu
Copy link
Contributor

dbu commented Sep 4, 2024

thanks for the fix! https://github.com/php-http/multipart-stream-builder/releases/tag/1.4.1

@aboks
Copy link

aboks commented Sep 4, 2024

This change seems to cause static analysis tools like (at least) Psalm to stumble over calls to addResource; see https://psalm.dev/r/1f410c9dd2 for a minimal reproduction case. I think the ? needs to be put outside the quotes.

@ruudk ruudk deleted the patch-1 branch September 4, 2024 13:07
ruudk added a commit to ruudk/multipart-stream-builder that referenced this pull request Sep 4, 2024
@ruudk
Unquote array shape field names
8157514 
Otherwise optionals don't work.

See php-http#66
@ruudk ruudk mentioned this pull request Sep 4, 2024
Merged
@ruudk
Copy link
Contributor Author

ruudk commented Sep 4, 2024

@aboks @dbu Ouch. Fixed in #67

I did not expect this at all.

dbu added a commit that referenced this pull request Sep 4, 2024
@ruudk @dbu
Unquote array shape field names (#67)
825153b 
* Unquote array shape field names

Otherwise optionals don't work.

See #66

* align parameter phpdoc

---------

Co-authored-by: David Buchmann <david@liip.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu 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.
3 participants
@ruudk @dbu @aboks