[PHP-wasm] allow requests to follow redirects #2206
Conversation
| /** | ||
| * Whether to follow redirects. | ||
| * - `follow`: Follow redirects. | ||
| * - `manual`: Return the redirect response. (Default) |
There was a problem hiding this comment.
Should manual stay the default behavior or should we follow by default like JS fetch and browsers do?
| }); | ||
| expect(response.httpStatusCode).toBe(200); | ||
| expect(response.text).toContain('Edit site'); | ||
| expect(response.text).toContain('Edit Site'); |
There was a problem hiding this comment.
This label changed in a recent version of WP, and the test stopped working.
| [301, 302].includes(response.httpStatusCode) | ||
| ) { | ||
| if (request.redirect === 'error') { | ||
| throw new Error( |
There was a problem hiding this comment.
Do we need to throw? Or could we just have two modes: follow and not follow?
There was a problem hiding this comment.
No, fetch Rejects the promise with TypeError: NetworkError when attempting to fetch resource., so I made it work in the same way now.
There was a problem hiding this comment.
Or could we just have two modes: follow and not follow?
We could, but I wanted to match the fetch. Would you prefer to only add follow and manual for now and add error if users ask for it?
|
I really like the idea. At the same time, I'm worried this may not be easy. When you log in, the http response redirects you and sets the auth cookies in one go. If we discard that response and redirect internally, the user won't be logged in. Also, the iframe src won't point to the target URL. I suspect this is why web servers typically do a network trip instead of internalizing the redirection like this PR proposes. |
I see now that this works well with the Node version of Playgorund, where cookies are stored internally, but not with the browser version. There is a good use case for this in the Node version of Playground, as you can see from our tests, but I'm not sure if the |
|
Documenting our call today:
|
Motivation for the change, related issues
Browsers and JS
fetchautomatically follow (301-302) redirects, but thePHPRequestHandlerdoesn't.To follow redirects, developers need to implement a loop that checks the response and run another request if the
locationheader is present.This isn't a big issue, but we already have this implemented in multiple places around Playground, and anyone who is looking into building integration tests with Playground needs to implement it.
This PR suggests that we add support for following redirects to the
PHPRequestHandler.Because this is new behavior, I made it
manualby default, so users will need to enable it by settingredirect: 'follow'.Implementation details
This PR adds a
redirectoption to thePHPRequestinterface.Depending on the option,
PHPRequestHandler.requestwillfollowthe redirect, thrown anderror, or return the 301/302 response and givemanualcontrol to the user.manualis the default.Testing Instructions (or ideally a Blueprint)