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

Error if request URI provides relative path #269

Merged
merged 6 commits into from
May 10, 2020
Merged

Error if request URI provides relative path #269

merged 6 commits into from
May 10, 2020

Conversation

remorhaz
Copy link
Contributor

@remorhaz remorhaz commented May 8, 2020

No description provided.

@remorhaz
Copy link
Contributor Author

remorhaz commented May 8, 2020

The build is failing but it seems to have been failing for some time. Tests are segfaulting for some reason under 7.2 even locally (I failed to understand why) but run okay on 7.3 and 7.4. I've re-checked that at least my tests are running okay on all three versions.

Copy link
Member

@kelunik kelunik left a comment

Choose a reason for hiding this comment

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

The same check needs to be done in Http2Connection, should we put the check into DefaultConnectionFactory or duplicate it into Http2Connection?

src/Connection/Http1Connection.php Outdated Show resolved Hide resolved
$uri->method('withPort')->willReturnSelf();
$uri->method('withQuery')->willReturnSelf();
$uri->method('withFragment')->willReturnSelf();
$uri->method('__toString')->willReturn('http://localhost/foo');
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a mock here or can we just use league/uri?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need mock because league/uri doesn't allow relative paths and I don't think it's a good idea to require-dev another implementation that does allow them (laminas/laminas-diactoros, for example).

Copy link
Member

Choose a reason for hiding this comment

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

That's good to know, thanks! I wouldn't have a problem with requiring laminas/laminas-diactoros in require-dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not a big problem, but they can decide to change this behavior in future - I think it's better not to rely on it. Anyway, it's just one small negative corner-case.

Co-authored-by: Niklas Keller <me@kelunik.com>
@kelunik
Copy link
Member

kelunik commented May 9, 2020

If you rebase onto master or merge master, the tests should be working fine again. I've skipped the tests causing segfaults on PHP 7.2 and 7.3.

@remorhaz
Copy link
Contributor Author

remorhaz commented May 9, 2020

The same check needs to be done in Http2Connection, should we put the check into DefaultConnectionFactory or duplicate it into Http2Connection?

Hmm, I need to take a closer look...

@remorhaz
Copy link
Contributor Author

should we put the check into DefaultConnectionFactory or duplicate it into Http2Connection?

I think it's wise to make checks just before contract protects us; and in our case contract of UriInterface doesn't protect us from this particular problem, so I vote for duplication. Relying on DefaultConnectionFactory is out of contact at all, I believe that code shouldn't rely on some checks made by external code.

@remorhaz
Copy link
Contributor Author

Seems that some tests are flaky.

@kelunik kelunik changed the title Client fails if request URI provides relative path Error if request URI provides relative path May 10, 2020
@kelunik kelunik merged commit d52aaa7 into amphp:master May 10, 2020
@kelunik
Copy link
Member

kelunik commented May 10, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants