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

Fixing InvalidArgumentException with 'Cannot redirect to an empty URL' #198

Merged
merged 4 commits into from
May 28, 2019
Merged

Fixing InvalidArgumentException with 'Cannot redirect to an empty URL' #198

merged 4 commits into from
May 28, 2019

Conversation

iakunin
Copy link

@iakunin iakunin commented May 1, 2019

This exception occurs when 'Referer' header exists but empty.

In this case empty string goes to the first arg of \Symfony\Component\HttpFoundation\RedirectResponse constructor.

And then \InvalidArgumentException is thrown inside \Symfony\Component\HttpFoundation\RedirectResponse::setTargetUrl method.

@iakunin
Copy link
Author

iakunin commented May 27, 2019

@lsmith77, could you give me some feedback, please?

@lsmith77
Copy link
Contributor

makes sense .. could you also add a test case?

@iakunin
Copy link
Author

iakunin commented May 27, 2019

@lsmith77,

I added unit-test for all cases came into my mind + also I covered the case when NotFoundHttpException is thrown.

But it's hard to cover the case with $cookieOptions, because of the time() function.

If you want, I can extract this time() function to separate TimeProvider and mock it.


Also I added SymfonyResponseComparator to prevent build from failure. It happened during comparison two Symfony-responses: their Date-headers are are different from each other. That's because new DateTime() during response creation. When the time between creation of two responses differs more than 1 sec, build fails.

SymfonyResponseComparator removes Date-header from the comparison of two responses.

@lsmith77
Copy link
Contributor

nice!

SymfonyResponse might even be something for Symfony core ..?

for the call to time() .. it might be best to just switch to $request->server->get('REQUEST_TIME')

@lsmith77
Copy link
Contributor

perfect!

@lsmith77 lsmith77 merged commit b70d57b into liip:master May 28, 2019
@iakunin
Copy link
Author

iakunin commented May 28, 2019

@lsmith77, thank you so much for your feedback!

May I ask you to create a new Release (1.6.2)?

@lsmith77
Copy link
Contributor

I will make a release as soon as #200 is merged.

However the next release will be 1.7.0

@iakunin
Copy link
Author

iakunin commented May 29, 2019

@lsmith77, great!

I'll be waiting for it =)

@lsmith77
Copy link
Contributor

@iakunin
Copy link
Author

iakunin commented Jun 19, 2019

@lsmith77, thank you so much!

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

Successfully merging this pull request may close these issues.

2 participants