Improve type inference for element attributes

[gsteel]

Q	A
Documentation	yes
QA	yes

Description

Marks all attribute arrays and iterables as <string, mixed> which is slightly better than <array-key, mixed>

This is pretty minor stuff, but it really helps in consumer projects running psalm.

I considered that we could have gone for <string, scalar|null> but I'm fairly sure there are cases where arrays are accepted, or maybe even iterables at which point, <string, mixed> felt like a better middle ground considering ElementInterface::getAttribute(string $name): mixed

[Slamdunk]

I considered that we could have gone for <string, scalar|null> but I'm fairly sure there are cases where arrays are accepted

As far as I can test and tell, those cases are errors that come from a past (year 2012) when types weren't a thing, and we tested everything just to be sure but without a valid use case.
I'm talking about:

laminas-form/test/FactoryTest.php

Lines 99 to 105 in 943ece1

	'attributes' => [
	'type' => 'radio',
	'options' => [
	'foo' => 'Foo Bar',
	'bar' => 'Bar Baz',
	],
	],

laminas-form/test/View/Helper/FormElementTest.php

Line 87 in 943ece1

$element->setAttribute('options', ['option' => 'value']);

I'd say let's go all-in with <string, scalar|null> even for ElementInterface::getAttribute PHPDoc return type, remove the four test cases with invalid sets and release.

[Slamdunk]

Really good work, some minor improvements and we can go 💪

[Ocramius]

Very cool! One question on the patch, otherwise LGTM!

[Ocramius]

Does building the element from an array spec still work, after removing this?

[Slamdunk]

@gsteel seems a prudent guy:

https://github.com/laminas/laminas-form/pull/245/files#diff-9ed265bc15aed45c500eadeea5fec4faa6af32a6800c80a282eac53c51b604d4R188-R216

[Ocramius]

Was more thinking about the really wonky early-laminas "let's make an entire form from a single array" kinda thing :D

[gsteel]

Yes, it does still work - The factory tests were showing this, but I did adjust this test in da4a9f0 so that the deprecated version is no longer used.

[gsteel]

Thanks @Slamdunk - I've adjusted the tests to use setValueOptions where appropriate and also properly deprecated providing "value options" in the options key to setAttributes - Also added deprecated tests to cover the deprecation.

Sorry, the patch is a bit bigger now, but the change to scalar|null of course caused a lot of valid psalm issues that needed fixing

[Slamdunk]

What a refreshing patch, thank you @gsteel

[gsteel]

Thanks to you too @Slamdunk and @Ocramius for the reviews :)

[pavattt]

@Slamdunk Do you have any rules on what changes can go into a minor version and what cannot? Signature change in minor version is not something I'd expect.

[froschdesign]

@pavattt

Signature change in minor version is not something I'd expect.

Only allowed in major versions. Do you find a problem?

[pavattt]

@froschdesign I've upgraded laminas/laminas-form from 3.13.1 to 3.17, and now I have a ton of issues in static analysis I have to fix.

[Ocramius]

@Slamdunk Do you have any rules on what changes can go into a minor version and what cannot? Signature change in minor version is not something I'd expect.

It's a bit of a gray area here, because SA shifts regularly (even with patch versions of psalm and phpstan).

We keep BC on anything that would otherwise require changes in the code for:

• making it run as before
• making it "compile" as before, where "compile" is what PHP itself understands as such

For changes such as more precise data structures and types, it is very improbable that you'll get BC on that, as the types can easily shift with tiny changes, even in other packages that declare better types.

My general suggestion when doing a composer.lock change that introduces new type errors is to have a baseline file (supported by both PHPStan and Psalm), updating that, and deciding which errors that were introduced are worth fixing immediately.