Document generics order for PriorityQueue and SplPriorityQueue

laminas / laminas-stdlib

SPL extensions, array utilities, error handlers, and more

https://docs.laminas.dev/laminas-stdlib/

BSD 3-Clause "New" or "Revised" License

190 stars 40 forks source link

Document generics order for PriorityQueue and SplPriorityQueue #72

Closed gsteel closed 2 years ago

gsteel commented 2 years ago

Q	A
Documentation	yes
Bugfix	yes

Reference #71

Previously implemented template order did not match those of native SplPriorityQueue, i.e. \SplPriorityQueue<TPriority, TValue> - instead the templates were back-to-front.

It's decided that 'fixing' the order of the placeholders will be inconvenient for users that have already implemented templates with the existing order.

PriorityQueue value template renamed from T to TValue
Removes attempt to document internal array priority, adding resulting psalm issues to the baseline. Users will typically insert with an integer priority and shouldn't care that internally an array is used to represent priority - also, an array for \SplPriorityQueue priority is effectively an invalid argument according to stubs - this is better off in the baseline right?

gsteel commented 2 years ago

Not sure if this should target 3.12.1 or 3.13.0 ?

Ocramius commented 2 years ago

@gsteel if this is a bugfix, 3.12.x IMO.

boesing commented 2 years ago

This would possibly break upstream projects which already spent time in fixing their code.

Yes, this looks correct to me but technically, it was not incorrect before. Do we really want introduce this in a Patch?

boesing commented 2 years ago

ah this is only released for a few hours, maybe we can Patch it 👌🏻

gsteel commented 2 years ago

Generics were introduced and released in #58 and #59 back in June

boesing commented 2 years ago

Oh, thats been a while. Not sure if we should annoy upstream devs who spent time to fix their static analysis. Do we have other thoughts?

gsteel commented 2 years ago

Not sure if we should annoy upstream devs who spent time to fix their static analysis

I guess it depends whether we consider the original template order a bug or not - IMO it's a bug - aligning with PHP's SplPriorityQueue and psalms stubs should be a given. Not annoying devs with this is really valid, but the longer it's left, the more people we annoy?

This will already affect one or two laminas packages I think… view, maybe validator, possibly others. My bad! Sorry…

internalsystemerror commented 2 years ago

Oh, thats been a while. Not sure if we should annoy upstream devs who spent time to fix their static analysis. Do we have other thoughts?

It's also possible, if this is a bug, that devs who already implemented correct static analysis have been annoyed by this break, so would appreciate this fix.

Ocramius commented 2 years ago

Discussed in chat: clarifying template types is OK, but swapping order will likely just lead to unnecessary downstream breakages.

Yes, it is annoying that the generic types are inverted in our implementation, but it is just that: annoying. Not a major deal breaker.