Various minor code & phpdoc improvements#734
Various minor code & phpdoc improvements#734jtojnar wants to merge 6 commits intosimplepie:masterfrom
Conversation
- Most of the properties are nullable, update the annotations to match reality. - Use more specific types for arrays. - Add param types to constructor. - `length` is int in practice (there are no fractional bytes), make the annotations int instead of string and float. - Fix some undefined variables. - Remove redundant explicit namespaces.
- Most of the properties are nullable, update the annotations to match reality. - Use more specific types for arrays. - Fix passing false to strpos/substr where they expect int. - Remove redundant explicit namespaces. - Add missing/fix type PHPDoc type annotations.
| while ($row = $query->fetchColumn()) | ||
| { | ||
| $feed['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['entry'][] = unserialize($row); | ||
| assert(is_string($row)); |
There was a problem hiding this comment.
I'm not sure about this. Isn't the outcome of assert() probably not determinable because of different configuration of the server?
Assertions should be used as a debugging feature only. [...] Assertions should not be used for normal runtime operations like input parameter checks. As a rule of thumb your code should always be able to work correctly if assertion checking is not activated.
There was a problem hiding this comment.
The assertions are only used here as a hint for the static analyser – they should never trigger, unless the database is corrupted. But I do not really like them.
There was a problem hiding this comment.
Would that help the static analyser?
- assert(is_string($row));
+ $row = (string) $row;There was a problem hiding this comment.
Yes, that would make the type checker happy I assume. Thought I find this somewhat iffy too, since it casts a variable to string that already is a string (unless db is corrupted). Assertions, meanwhile, will be disabled on production.
I know this will have negligible performance impact but it is still a smelly code. Though maybe it is acceptable trade off, if we want static analysis to be happy.
|
|
||
| /** | ||
| * @var string Original feed URL, or new feed URL iff HTTP 301 Moved Permanently | ||
| * @var string|null Original feed URL, or new feed URL iff HTTP 301 Moved Permanently |
There was a problem hiding this comment.
We could also fix the typo (iff -> if)
| $query->bindValue(':id', $this->id); | ||
| if ($query->execute() && ($time = $query->fetchColumn())) | ||
| { | ||
| assert(is_int($time)); |
There was a problem hiding this comment.
Same question about assert().
|
looking good @jtojnar |
|
Thinking about this, I do not really like this PR – the commits are too scattered and hard to review, even for me as the author. There are also stylistic changes mixed in. Namely, I converted absolute namespaces to relative ones in PHPDoc because Psalm for some reason did not like those (probably a bug). We should probably use php-cs-fixer to deal with those first. I moved the only two clean commits to #735 |
|
What do you think about using php-cs-fixer to change the code style to PSR-12? I think that the timing is ideal for that at the moment. |
|
@jtojnar Would you like create a PR about PSR-12 change or should I create it? |
|
@Art4 I probably will not be able to get to it anytime soon, so feel free to do it. |
3 participants
I tried using Psalm to fix some random issues and improve the codebase a bit with regards to static analysis.