Conversation
SvenRtbg
left a comment
SvenRtbg
left a comment
There was a problem hiding this comment.
Thanks for your contribution so far.
If you follow up on the original issue, I extended the feature request a bit, as it might be common to encounter the full array<keytype, valuetype> style as well.
| $array = null; | ||
| $subtype = null; | ||
| if ($this->isArrayOfType($type)) { | ||
| if (null !== $subtype = $this->isArrayOfType($type)) { |
There was a problem hiding this comment.
Here it's called $subtype, later down it is called $sub. I'd prefer a single name for a single thing.
| //array | ||
| $array = array(); | ||
| $subtype = substr($type, 0, -2); | ||
| } else if (substr($type, -1) == ']') { |
There was a problem hiding this comment.
I don't feel we should keep these two codepaths that deal with the contents of arrays separated. But this is risky to change.
| * @return bool | ||
| * @return ?string | ||
| */ | ||
| protected function isArrayOfType($strType) |
There was a problem hiding this comment.
Rename the method accordingly. I strongly believe a function with isSomething is supposed to return boolean for yes or no, and this has changed.
| } | ||
| if (strpos($strType, 'array<') === 0 && $strType[strlen($strType)-1] === '>') { | ||
| return substr($strType, 6, strlen($strType)-7); | ||
| } |
There was a problem hiding this comment.
There's this weird situation now that two of three cases where contents of arrays are being detected are covered here, the remaining one (examples are in the tests) with array[string] style is still outsourced in the logic.
Also, there are too many magic numbers happening here. I know we still support ancient PHP 7, so there is no way to utilize str_ends_with() etc. substr(..., -2) is borderline close to the threshold already, but standalone would pass as "cut last two chars off". The other case simply doesn't do it in an easier to understand way: What is the 6, what is the -7, why detect the last character in a different (array-access-like) way compared to the first case.
I see there is heavy complexity accumulating in this method now, and I know there's barely a way around it, but the responsible thing I ask for is to explain it better. And by explaining, I don't necessarily mean adding comments.
|
|
||
| /** | ||
| * @var string[] | ||
| * @var array<string> |
There was a problem hiding this comment.
Changing the only string array with the square bracket style into something different does not sound like the best idea. I don't know if the float[] case above covers all relevant test cases, and I really would like to see a dedicated test case for this new type style, as the test acts as documentation what is possible. Hiding it behind some test map class code does not sound like a transparent way to handle new features.
|
This is a very bare-bones, naive implementation that is intentionally kept simple as I saw #165. Ideally, this should just bring phpstan's phpdoc parser or even valinor and re-use their logic, but this would require a major overhaul. This PR, on the other hand, aims at a very simple and barebones implementation, changing as little as possible to avoid pouring time in a PR that might get refused. |
|
I understand this approach, and the reasoning behind it. It will lead to unmaintainable code in the long term, though, because everyone is scared to do more than just the required minimum if the invested work would be rejected. |
|
I'm sorry, but I don't have the bandwidth for a full implementation right now, I'm only upstreaming this patch as it is used by one of my clients. |
2 participants
FIxes #243