Skip to content

Conversation

@bukka
Copy link
Member

@bukka bukka commented Nov 28, 2020

This PR fixes bug 69625. The reason is that (SG).request_method did not get set because script_path_translated was empty. The fix sets request_method first so it has got value even if SCRIPT_FILENAME is empty. That will keep the behavior for empty REQUEST_METHOD but fixes it for empty SCRIPT_FILENAME.

@bukka bukka changed the base branch from master to PHP-7.4 November 28, 2020 21:38
@bukka bukka force-pushed the fpm_no_script_filename branch from 91e13a9 to bbd6fac Compare November 28, 2020 21:39
@mvorisek
Copy link
Contributor

I think we should return 5xx for an invalid request.

@bukka
Copy link
Member Author

bukka commented Nov 29, 2020

Invalid SCRIPT_FILENAME is already returning 404 so this is much more consistent. Also this is a client error so this needs to be 4xx. 5xx is for server errors...

@nikic
Copy link
Member

nikic commented Nov 30, 2020

Why is that script_path_translated check there at all? The comment says it allows specifying the script via argv, but I don't think FPM supports that, right? This code was copy&pasted from init_request_info in sapi/cgi/cgi_main.c, and I think the condition was just copied over even though it does not make sense in the FPM context.

@bukka
Copy link
Member Author

bukka commented Nov 30, 2020

The check seems to be mostly useful to save some cycles as the processing can basically end if it's null and request_method is set because then it will be early return from the request (200 with empty content if request_method is null or 404 if script_path_translated is null). Guess it would be probably better to just do

if (!script_path_translated) {
    return;
}

but that's more minor optimization unrelated to the fix...

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

I think it would be good to at least fix the comment.

@bukka
Copy link
Member Author

bukka commented Dec 13, 2020

Comment updated and merged via a221e17

@bukka bukka closed this Dec 13, 2020
@Felixoid
Copy link

Felixoid commented Dec 15, 2020

Hello. Could you say, please, in what release it will be included? 7.3, 7.4, 8.0?

upd: Oh, I see a php:PHP-7.4 tile on the top. Never mind!

Many thanks for the fix!

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