Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array shape of ServiceManager config contains unspecific array #212

Closed
rieschl opened this issue Oct 14, 2023 · 10 comments · Fixed by #213
Closed

Array shape of ServiceManager config contains unspecific array #212

rieschl opened this issue Oct 14, 2023 · 10 comments · Fixed by #213
Labels
Bug Something isn't working
Milestone

Comments

@rieschl
Copy link
Contributor

rieschl commented Oct 14, 2023

Bug Report

Q A
Version(s) 3.22.0

Summary

Not really a bug but more of a QA issue.

The ServiceManagers ServiceManagerConfiguration array shape contains a services key with an unspecific array notation. That trips some static analyzers.

Current behavior

ServiceManagerConfiguration contains the definition: services?: array<string,object|array>, which includes a generic array.

How to reproduce

Use the ServiceManagerConfiguration configuration anywhere and run PHPStan (I tried 1.10) which emits PHPDoc tag @var for variable $serviceManagerConfig has no value type specified in iterable type array.

Expected behavior

PHPStan should not throw an error. This is achieved by using a more "specific" array definition: array<array-key, mixed>.

@rieschl rieschl added the Bug Something isn't working label Oct 14, 2023
rieschl added a commit to rieschl/laminas-servicemanager that referenced this issue Oct 14, 2023
@boesing
Copy link
Member

boesing commented Oct 16, 2023

Just my 2 cents here:
We do only support psalm, imho thats a phpstan specific "feature" and therefore not directly supported. If there would be an issue with psalm having support for something phpstan does not, I personally would not care.
I introduced changes to psalm a lot to make some stuff work with servicemanager and I do not see why we should spend too much time in fixing phpstan specific "style".

There absolutely 0 difference for psalm (and most probably phpstan) between array and array<array-key,mixed> (which is a bunch of useless boilerplate) which is absolutely fine for me.

Would you mind raising a question in psalm itself to see what the general idea of this kind of notation is on their side?
i.e.: do they want to align with phpstan regarding this topic in v6?

I am not against this change but I want to explain that we do not explicitly provide support for phpstan and therefore there might be changes happening in laminas which are only supported by psalm and not by phpstan.

@rieschl
Copy link
Contributor Author

rieschl commented Oct 16, 2023

Thanks for the feedback! It's of course up to you if you want to change that or not.
The way I see it, it is a "bug" in psalm (if there is even a rule to enforce to specify types in arrays?). It is a difference if I write array or array<array-key, mixed> because the second one explicitly states that the key can be anything (int or string) and the value can be anything, whereas the first one says "I don't know".
Maybe there is no difference for static analyzers, but I hate it (in our own code), if there is just an array doctype because then I don't know what the method expects/returns. If there's an array<array-key, mixed> then I know that it really accepts anything (although array<array-key, mixed> is quite unusual, except maybe for configuration arrays).

@Ocramius
Copy link
Member

I think array<mixed> would at least prevent us from referencing array-key, which is a bit messy.

Also not opposed to the change, as I understand that PHPStan screams about "no array shape specified".

@rieschl
Copy link
Contributor Author

rieschl commented Oct 16, 2023

I think array<mixed> would at least prevent us from referencing array-key, which is a bit messy.

Also not opposed to the change, as I understand that PHPStan screams about "no array shape specified".

PHPStan complains if there is no array value defined (PHPDoc tag @var for variable $serviceManagerConfig has no value type specified in iterable type array.).
Both PHPStan and psalm just complain if there is a mismatch in the array key. If no key is provided, both infer array-key.
Personally, I always specify the key (or array-key) to be explicit, i.e array<array-key, xxx>: "I don't care" vs. array<xxx>: "I don't know".

@boesing
Copy link
Member

boesing commented Oct 16, 2023

It is a difference if I write array or array<array-key, mixed> because the second one explicitly states that the key can be anything (int or string) and the value can be anything, whereas the first one says "I don't know".

So that also would be the case for int right?
Or do you usually state int<min,max> instead of int?

IMHO, that is some kind of phpstan specific personal flavor of how to write code. I am the last dude who says "lets not be specific" but unless the annotation gives more value, I tend to avoid it.

IMHO array === array<array-key,mixed>, there is 0 difference besides the fact that the other one is more verbose.
Same as int === int<min,max>, but somehow, phpstan is not encouraging their userbase to write that. Maybe because "they don't care". :trollface:


Again, I'd love to see an issue created in psalm where we try to figure out if there are plans for psalm to change this behavior at some point. If not, I'd rather have phpstan and psalm align on that decision.
If they don't, thats problematic but I usually also tell my devs to avoid using phpstan and psalm in the same codebase.

@rieschl
Copy link
Contributor Author

rieschl commented Oct 16, 2023

hrhr, that's a point 😄 although I'd say that an array is usually more complex than an int and most of the time there's no specific range to pass (and if there is, we do specify a range) whereas normally a method does require or return some specific array shape and thus writing array<mixed> or array<array-key, mixed> explicitly states that it really can return anything.
Granted, it doesn't occur often that you have to write array<mixed>. Probably I've just been bitten too often by older parts of our codebase where I've encountered just a generic array return type without a specific value, forcing me to check the whole method to determine what's actually being returned 😆

@boesing
Copy link
Member

boesing commented Oct 16, 2023

I think array would at least prevent us from referencing array-key, which is a bit messy.

I do not have a problem with array-key but if we can avoid it, I'm fine with having array<mixed> instead.

@boesing boesing added this to the 3.22.1 milestone Oct 16, 2023
rieschl added a commit to rieschl/laminas-servicemanager that referenced this issue Oct 16, 2023
@rieschl
Copy link
Contributor Author

rieschl commented Oct 16, 2023

I've opened an issue in psalm: vimeo/psalm#10287
Let's see what they think about it 🙂

@boesing
Copy link
Member

boesing commented Oct 24, 2023

Closed with #213

@boesing boesing closed this as completed Oct 24, 2023
@InvisibleSmiley
Copy link

It is a difference if I write array or array<array-key, mixed> because the second one explicitly states that the key can be anything (int or string) and the value can be anything, whereas the first one says "I don't know".

So that also would be the case for int right? Or do you usually state int<min,max> instead of int?

No, the PHPStan rule only applies to iterable types:

https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type

(unfortunately there's a bug that prevents this from getting applied to Generator but that's acdifferent story)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants