Refactor MonthSelect removing inheritance and improving tests

[ramchale]

Q	A
Documentation	yes
Bugfix	no
BC Break	yes
New Feature	no
RFC	yes
QA	no

Description

Draft pull request to discuss desired behavior of MonthSelect in v3. Some of this will also be relevant to other classes extending AbstractDateDropdown.

• Currently this throws a RuntimeException if an array is passed to the filter with the incorrect number of date parts. Should just pass through unprocessed for consistency with other filters?
• How do you want this to handle months that are outside of a valid date range?
• How do you want to handle values for month that are not numerical (strings, bools, null, etc)? (I'm guessing the original value should be returned unprocessed, but better not to assume)

[gsteel]

Looks to me like expected input is, like you say, an associative array of varying lengths depending on the descendant class that should produce a formatted string. The filter requires that array members be specificly year, month etc, so it's already very restrictive - I really don't see the point of this filter at all - I mean, on the frontend, why not just use an <input type="date|datetime-local"> - this filter pre-dates HTML5, so I'm wondering if we should just deprecate all of it and remove it completely.

cc @froschdesign

[ramchale]

Cool. Does that apply to the other two that inherit from AbstractDateDropdown (DateSelect and DateTimeSelect), as they share a lot of the same behavior at the moment?

[gsteel]

I'd say all 4 classes, MonthSelect, Abstract, DateSelect & DateTimeSelect - Let's wait for @froschdesign to give us an opinion too 👍

[froschdesign]

I'd say all 4 classes, MonthSelect, Abstract, DateSelect & DateTimeSelect…

Unfortunately, that won't work, because laminas-form uses these filters:

• Laminas\Form\Element\DateSelect: https://github.com/laminas/laminas-form/blob/653c869d10c361027ae6c660c991ec3e3f38ed65/src/Element/DateSelect.php#L175-L193
• Laminas\Form\Element\DateTimeSelect: https://github.com/laminas/laminas-form/blob/653c869d10c361027ae6c660c991ec3e3f38ed65/src/Element/DateTimeSelect.php#L335-L353
• Laminas\Form\Element\MonthSelect: https://github.com/laminas/laminas-form/blob/653c869d10c361027ae6c660c991ec3e3f38ed65/src/Element/MonthSelect.php#L325-L343

[ramchale]

I'd say all 4 classes, MonthSelect, Abstract, DateSelect & DateTimeSelect…

Unfortunately, that won't work, because laminas-form uses these filters:

* `Laminas\Form\Element\DateSelect`: https://github.com/laminas/laminas-form/blob/653c869d10c361027ae6c660c991ec3e3f38ed65/src/Element/DateSelect.php#L175-L193

* `Laminas\Form\Element\DateTimeSelect`: https://github.com/laminas/laminas-form/blob/653c869d10c361027ae6c660c991ec3e3f38ed65/src/Element/DateTimeSelect.php#L335-L353

* `Laminas\Form\Element\MonthSelect`: https://github.com/laminas/laminas-form/blob/653c869d10c361027ae6c660c991ec3e3f38ed65/src/Element/MonthSelect.php#L325-L343

Thanks both. Does that bring us back to the questions about behavior in v3 or is there more to consider?

[gsteel]

OK, so I think that in all erroneous cases, the filter(s) should return the unprocessed input that will hopefully then fail subsequent validation - so, out of range, incorrect array shapes etc should not throw exceptions, but just silently pass through unchanged.

I think we have to work on the premise that we're working with data that hasn't been validated. In typical usage with input-filter, throwing an exception for malformed input prevents validators from doing their job and reporting to the user that the input was rejected.

[froschdesign]

OK, so I think that in all erroneous cases, the filter(s) should return the unprocessed input that will hopefully then fail subsequent validation - so, out of range, incorrect array shapes etc should not throw exceptions, but just silently pass through unchanged.

Based on the form elements from above: The validators Date and Regex can handle the unprocessed input arrays so that the validators can fulfil their jobs in all cases and produce the related error messages.

[ramchale]

I think this covers the desired behaviour. Let me know if there's anything not quite right, or any other test cases that would be useful. Once we've got the code doing what it should I'll look at adding some docs.

Also, psalm isn't loving returning $value after $month = $value['month'] ?? null;. While I don't want to restructure just to appease static analysis, if you've got any suggestions to avoid the error suppression that'd be useful.

[ramchale]

Thanks. Still needs the suppression for the return statement, but the rest is looking a lot healthier

[gsteel]

Thanks @ramchale - I figured out the reason for the InvalidReturnStatement - Sorry to be pedantic 😬

[ramchale]

Thanks @ramchale - I figured out the reason for the InvalidReturnStatement - Sorry to be pedantic 😬

No. Really appreciate the feedback and help. Thanks

[gsteel]

Thanks @ramchale 👍

[gsteel]

Is there anything else needed here or can I merge it?

[ramchale]

Is there anything else needed here or can I merge it?

I've got some accompanying documentation I can tidy up and add to the v2_to_v3.md and standard-filters.md this afternoon. So might want to hold off for that?

[gsteel]

I've got some accompanying documentation I can tidy up and add to the v2_to_v3.md and standard-filters.md this afternoon. So might want to hold off for that?

Cool - I'll wait for the docs so they go in together 👍

[gsteel]

Thanks again @ramchale 👌