Skip to content

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Jun 2, 2024

These functions generate errors internally, so it is irrational to check for false every time after calling the function.

refs #2986

tempnam()

tempnam() uses php_open_temporary_fd. If it can't open the file in the specified directory and falls back to the system directory, an E_NOTICE is raised.

https://github.com/php/php-src/blob/d2a932b8af39f313d27278b4b76c0eebe7d3419c/main/php_open_temporary_file.c#L279-L325

tmpfile()

The tmpfile() function uses php_open_temporary_fd internally, as well as the tempnam() function.

https://github.com/php/php-src/blob/5b3574009855279716455024871dadaa5e91d38f/main/streams/plain_wrapper.c#L217-L248

@ondrejmirtes
Copy link
Member

I'm sorry, I don't understand the reasoning.

These functions generate errors internally, so it is irrational to check for false every time after calling the function.

I can't parse that sentence. Can you show an example why it's not effective to check for false?

@thg2k
Copy link
Contributor

thg2k commented Jun 3, 2024

These functions generate errors internally, so it is irrational to check for false every time after calling the function.

I can't parse that sentence. Can you show an example why it's not effective to check for false?

I think he means that it's a bit boring to check for false on something that should likely never return false on a healthy system. I have a similar problems with other cases, for example $im = imagecreatetruetype($w, $h);. I always put an assert($im !== false); to make phpstan happy, but it's a bit silly to check for false in that case as well.

My feeling is that there are some types of errors not worth checking, which is different from e.g. fopen($somefile, "r"); because a lot can go wrong and it's good that phpstan requests to disambiguate between resource|false.

Another approach I use is to call a custom function safe_fopen() which checks for false itself and throws an exception if it fails. I assume one can make a similar thing for every function, but I don't know what's the best approach.

@ondrejmirtes
Copy link
Member

Alright, I get it. I've never used those functions so I had no idea what they did, but after reading the docs it makes sense.

@ondrejmirtes ondrejmirtes merged commit ec13acf into phpstan:1.11.x Jun 3, 2024
@ondrejmirtes
Copy link
Member

Thank you.

@ondrejmirtes
Copy link
Member

I removed this part: a1da4bf

Because ideally we'd have some kind of dir-string that would carry the verified information that this directory exists. But we don't have that now. And non-falsy-string is just unnecesarily annoying without adding much value.

@zonuexe zonuexe deleted the fix/type-temp branch June 3, 2024 12:22
@zonuexe
Copy link
Contributor Author

zonuexe commented Jun 3, 2024

I'm sorry, thank you!

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.

3 participants