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

Cover invalid arguments to Stream constructor #178

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Nov 10, 2023

Q A
QA yes

Description

@gsteel gsteel self-assigned this Nov 10, 2023
try {
new Stream('foo');
} catch (DiactorosRuntimeException $error) {
self::assertStringContainsString('Invalid stream reference provided', $error->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we expect to see "Bad news" here?

Copy link
Member

Choose a reason for hiding this comment

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

Probably previous exception? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

'Bad News' is swallowed by src/Stream try/catch - it should be there in $error->previous

@Xerkus
Copy link
Member

Xerkus commented Nov 11, 2023

What exactly is this supposed to achieve?
fopen() failing to open stream is covered by further checks.

This is in response to a question in our slack about fopen() with url that gives 404 where false is returned and warning is triggered, right?
Expecting some outside error handler to convert warnings to exceptions is not normal and should not be done in tests. To deal with it properly error handler should be added around fopen() call.

@gsteel
Copy link
Member Author

gsteel commented Nov 11, 2023

@Xerkus - Yes, I added these tests last night after talking to @ramsey on Slack. I couldn't find any tests that checked for that specific exception. Whether it's valid or not to consider user defined error handlers, I'll leave up to you. Feel free to merge or close.

Adding an error handler to /src/Stream to ensure that warnings from fopen are not suppressed and are correctly raised as Throwable would probably be a worthwhile addition.

@gsteel gsteel assigned Xerkus and unassigned gsteel Nov 11, 2023
@gsteel gsteel closed this Mar 15, 2024
@gsteel gsteel deleted the cover-invalid-string-argument-to-stream-constructor branch March 15, 2024 14:56
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.

4 participants