Small improvements found by my phpstan work #9661
Conversation
|
@Cheddam Could you help us? |
| @@ -503,7 +503,7 @@ public static function send_file($fileData, $fileName, $mimeType = null) | |||
| * The pattern can optionally start with an HTTP method and a space. For example, "POST $Controller/$Action". | |||
| * This is used to define a rule that only matches on a specific HTTP method. | |||
| * | |||
| * @param $pattern | |||
| * @param string$pattern | |||
There was a problem hiding this comment.
Here we need one more whitespace.
| @@ -136,6 +136,9 @@ public function __construct( | |||
| $titleConfirmField = null | |||
| ) { | |||
|
|
|||
| // This param is requested only for backward compat | |||
| $form; | |||
There was a problem hiding this comment.
Maybe ignoreErrors: is a better place than this noop.
| * Returns false if it's a bad data type, and if appropriate, throws an exception. | ||
| * | ||
| * @param SS_List $dataList | ||
| * @return bool |
| if ($dataList instanceof Filterable) { | ||
| return true; | ||
|
|
||
| } else { |
| { | ||
| if ($dataList instanceof Filterable && $dataList instanceof Limitable) { | ||
| return $dataList; | ||
| } else { |
| return $this->list->limit($limit, $offset); | ||
| if ($this->list instanceof Limitable) { | ||
| return $this->list->limit($limit, $offset); | ||
| } else { |
| return $this->list->byID($id); | ||
| if ($this->list instanceof Filterable) { | ||
| return $this->list->byID($id); | ||
| } else { |
| return $this->list->byIDs($ids); | ||
| if ($this->list instanceof Filterable) { | ||
| return $this->list->byIDs($ids); | ||
| } else { |
| return $this->list->exclude(...func_get_args()); | ||
| if ($this->list instanceof Filterable) { | ||
| return $this->list->exclude(...$arguments); | ||
| } else { |
| return $this->list->debug(); | ||
| if ($this->list instanceof ViewableData) { | ||
| return $this->list->debug(); | ||
| } else { |
There was a problem hiding this comment.
Please consider putting errors in conditional and normal execution without indentation.
if (we have a problem) {
// Error here.
throw new Exception('💣');
}
return $normal . $execution;Normal execution in a conditional is hard to read: zig-zag instead of straight down.
| protected function checkDataType($dataList) | ||
| { | ||
| if ($dataList instanceof Filterable && $dataList instanceof Limitable) { | ||
| return $dataList; |
There was a problem hiding this comment.
It's a bit odd that in the delete class you return true, but here you return $dataList. I didn't expect an object return from a check...() function, would prefer the inlined type hinting via @var
There was a problem hiding this comment.
The point was to return something of a known type but fair enough an @var works too.
I might try an assert() or something as well...
There was a problem hiding this comment.
OK so assert()s work with PHPStan: https://phpstan.org/r/9b405a35-5203-45f7-9cd0-2e611d9e2331
I'm checking PHPStorm now too.
There was a problem hiding this comment.
PHPStorm and VS Code both use assert(... instanceof ...) commands to augment type-hinting.
I'll use that.
There was a problem hiding this comment.
assert() will throw a warning but unless the php.ini setting assert.exception is set to true it won't break execution. I guess that's okay?
I'd note that /** @var */ is the worst possible solution here as it doesn't affect code execution. It just helps your IDE lie to you. :-P
src/ORM/Connect/MySQLDatabase.php
Outdated
| /** | ||
| * In error condition, set transactionNesting to zero | ||
| */ | ||
| protected function resetTransactionNesting() |
There was a problem hiding this comment.
With our new semver rules, that's technically an API breakage. What's the motivation for this change?
There was a problem hiding this comment.
Right I'll pull the change out of this PR and into the other one for more in-depth discussion
Untidiness picked up on by phpstan
These never worked, it seems
These variables are always set
Found by phpstan
Subsystems relying on DataObjectInterface made use of these methods but didn’t make it explicit. This is essentially a bugfix on DataObjectInterface, although if someone had implemented DataObjectInterface without ViewableData, they will need to implement some new methods as part of this change. However, their code is quite likely to be broken until they do so. This is also a rarely used API.#
This is a trimming back of a rebased version of PHPStan PR, #9103.
The work of preparing that led me to find lots of little things that could be tidied up that, while they would require PR, are safe changes.
I've moved all the 'dangerous' changes out of that PR but figure it's useful to get these into mainline so that they are less likely to rot.
In particular, I note that @sunnysideup has started some PHPStan work in parallel and it's a bit of a shame that this work was rotting on a branch.
The original PR still has 5 commits not included in this.