Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

"Invalid status code provided" while processing the Http Response #178

Closed
2 tasks done
roypalacios opened this issue May 9, 2019 · 10 comments
Closed
2 tasks done
Milestone

Comments

@roypalacios
Copy link

roypalacios commented May 9, 2019

  • I was not able to find an open or closed issue matching what I'm seeing.
  • This is not a question. (Questions should be asked on chat (Signup here) or our forums.)

Issue #27 should be reopened.

This is 2019 and we should not fail to parse a response just because the HTTP Status is not strictly standard.

Adding a custom HTTP status is not an option if we are dealing with external Http servers. In my case I'm getting a 520.

Code to reproduce the issue

Response.php

    /**
     * Set HTTP status code and (optionally) message
     *
     * @param  int $code
     * @throws Exception\InvalidArgumentException
     * @return self
     */
    public function setStatusCode($code)
    {
        $const = get_class($this) . '::STATUS_CODE_' . $code;
        if (! is_numeric($code) || ! defined($const)) {
            $code = is_scalar($code) ? $code : gettype($code);
            throw new Exception\InvalidArgumentException(sprintf(
                'Invalid status code provided: "%s"',
                $code
            ));
        }

        return $this->saveStatusCode($code);
    }

Expected results

  • Treat anything >= 600 as a 500 - Internal Server Error
  • Accept any 5xx
  • Accept any 4xx
  • Accept any 3xx
  • Accept any 2xx as OK
  • No change to 1xx

Actual results

Zend-Http is able to understand "I'm a teapot" but unable to process LOTs of valid responses coming from a myriad of sources.

This really needs to be fixed. Otherwise people will accelerate the move out of Zend altogether.

Please let me know if you are willing to accept this issue, and need help implementing it.

@Ocramius
Copy link
Member

This is 2019 and we should not fail to parse a response just because the HTTP Status is not strictly standard.

That's not a helpful way to report issues: if you have a problem, send a patch with a test case, but writing "it should zomg why no fix yet?!" is not going to make you a lot of friends in OSS projects.

I'm pretty sure that we should not support anything above 599 anyway though, as zendframework/zend-diactoros also enforces that

@roypalacios
Copy link
Author

Hi Ocramius

Thanks for reading.

I'm more than happy to submit a patch for this. But first I need the approvers to agree on a design or expected results.

@roypalacios
Copy link
Author

Ok. No response from any maintainer. Good thing is I did not fix anything.

This seems to be abandonware. I will stop using zend-http, and as always try to get out of zend and PHP.

@MadCat34
Copy link
Contributor

MadCat34 commented May 20, 2019

Ok. No response from any maintainer. Good thing is I did not fix anything.

Nice state of mind ...

Maintainers have fulltime-job and family life, some are also involved in several open-source projects (like Ocramius; thanks for your job 👍 ) or go to events in the PHP community (as speakers or not).
They may not answer as fast as YOU would like...Be patient

Zend-Http is able to understand "I'm a teapot" but unable to process LOTs of valid responses coming from a myriad of sources.

Not strictly valid since HTTP code 520 itself is not documented by any of the HTTP specifications...

I will stop using zend-http, and as always try to get out of zend and PHP.

It's not a problem proper to Zend Framework nor PHP...

@roypalacios
Copy link
Author

roypalacios commented May 20, 2019

Hi MadCat34,

Let's try to solve this issue.

zend-diactoros: https://github.com/zendframework/zend-diactoros/blob/9a4b4a3c78c1a65e6096fb1f37ab424f33c68821/src/Response.php#L170

zend-http: https://github.com/zendframework/zend-http/blob/master/src/Response.php#L280

We need to make zend-http behave exactly like zend-diactoros:

  • setStatusCode should only throw an exception if the http status code is < 100 or > 599

Please note, the standard defines ranges and some codes. But, from my understanding, it doesn't prohibit anything like 520 or 555.

@roypalacios
Copy link
Author

If you guys are willing to accept this change, I'm fine to write the code and few tests.

@Ocramius
Copy link
Member

The approach is fine: send a patch with a failing test case and the fix for it, and it will most likely be merged.

Also, as @MadCat34 said, OSS is not my full-time job: I do this when I have time and the mood for it.

@michalbundyra
Copy link
Member

@roypalacios Are you willing to submit your PR?

@roypalacios
Copy link
Author

Hi @webimpress,

Yes, I can submit it in the next few days!

@roypalacios
Copy link
Author

Hello @webimpress @Ocramius @MadCat34

Please review the pull request -> #194

I have modified the code so that it behaves just as diactoros in this sense.

zend-diactoros: https://github.com/zendframework/zend-diactoros/blob/9a4b4a3c78c1a65e6096fb1f37ab424f33c68821/src/Response.php#L170

If anything else is needed, please let me know.

Roy

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

No branches or pull requests

4 participants