Would not having it type int|null and fixing the consumers be cleaner?

Would not having it type int|null and fixing the consumers be cleaner?

@jtojnar That would change the contract, and my personal preference is towards simpler types rather than more complex types, but besides that I do not really mind. In the case of my client, it would add a bit of code to check the returned type and/or cast to int (null to 0) before sending it to the rest of the pipeline that is expecting an HTTP status to be an integer.

Yeah, it would change the contract, but so does changing empty string to zero. Technically, zero would be an okay choice since it is not actually a valid status code but I generally prefer more explicit types for representing exceptional states over magic values. I have not really looked into the API but I would expect any consumer to filter such states out so they would never make it further in the pipeline anyway.

I have not really looked into the API but I would expect any consumer to filter such states out so they would never make it further in the pipeline anyway.

That is the point of the contract. SimplePie has been promising an int, which simplifies the code downstream. Having to check for non-int removes most of the advantage of the contract.

I do not really mind, but I believe my current suggestion is marginally more efficient.

That is the point of the contract. SimplePie has been promising an int,

That really depends on what you consider the contract. Observing Hyrum's Law you could just as well say the contract is int|''.

The only thing that is certain is that there is a discrepancy between the phpdoc annotation and the implementation.

As for API design, I generally prefer having fewer uncertainties by making corner cases explicit over code that something that appears to be simpler but the complexity is still there. You will almost certainly still need to have something like if ($simplepie->status_code() === 0) { return; } to avoid passing invalid status code downstream and having the invalid value as a different type makes it harder to forget about it and then have the downstream function crashing the app.

Thinking about this more, this is actually a BC break so we might need to change the PHPDoc annotation to int|'' instead, and only fix this in an major bump, if SimplePie follows semver.

Thinking about this more, this is actually a BC break so we might need to change the PHPDoc annotation to int|'' instead, and only fix this in an major bump, if SimplePie follows semver.

Strictly speaking, you are right. But the PHPDoc has always indicated that the value must be an integer. But that sometimes null or an empty string is set could be called a bug, which will be fixed now.

In the future you would avoid such bugs by defining the property type. But as long as we can't do that, I would work towards the simplified types.

You will almost certainly still need to have something like if ($simplepie->status_code() === 0) { return; } to avoid passing invalid status code downstream and having the invalid value as a different type makes it harder to forget about it and then have the downstream function crashing the app.

Just for the record, in the case of my client downstream, the only assumption was that I should get an int as per contract. A zero is perfectly fine and is not crashing anything (why should it?).

I would expect any function accepting int in lieu of HTTP status code to do basic sanity checks on it and throw an exception for obviously invalid ones. For example, I just checked the code of a random library and it does precisely that:

https://github.com/guzzle/psr7/blob/5c693242bede743c23402bc5b9de62da04a882d7/src/Response.php#L101

I would expect any function accepting int in lieu of HTTP status code to do basic sanity checks on it and throw an exception for obviously invalid ones. For example, I just checked the code of a random library and it does precisely that:

https://github.com/guzzle/psr7/blob/5c693242bede743c23402bc5b9de62da04a882d7/src/Response.php#L101

That is precisely going in the same direction than my argumentation, thanks :-)

$this->assertStatusCodeRange($status); calls a function that expects an integer (as it should)....

https://github.com/guzzle/psr7/blob/5c693242bede743c23402bc5b9de62da04a882d7/src/Response.php#L154

So such a code might currently be breaking in some cases, before this PR.

Right, the typehint of the $status argument of the Response constructor only accepts int, which means you would not be able to pass it '' or null. That would force you to handle invalid state before even constructing the Response object, which is IMO the clean code approach. Additionally, it would make static analysis tools like PHPStan be easily able to point out possible runtime crashes. Whereas when using a magic int value, it is much easier to forget to treat that edge case.

I think this is clear enough, if you want to propose a change @jtojnar I'm happy to look at it but will to merge this for now.

I opened #732