- πΊπΈUnited States cilefen
π If notifications.email is a string, an error occurs Needs work seems closely-related. What is the connection of π Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed to this issue?
- π©πͺGermany GOT intermedia
Thank you for correcting the metadata. We suspect that the changes made for 3382510 π Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed caused this issue.
- Assigned to threeg
- @threeg opened merge request.
- Issue was unassigned.
- Status changed to Needs review
about 1 month ago 7:47pm 11 May 2024 - π¨π¦Canada threeg Vancouver
As suspected, this was probably introduced in 3382510. This MR should resolve two issues:
- Ensure that an empty values now save to update.settings as {} as they did before
- Ensure that any sites who's config has been saved as notification: emails: - '' register as an empty value
- Status changed to Needs work
about 1 month ago 5:35am 12 May 2024 - π³πΏNew Zealand quietone New Zealand
I've restored the template which should have the proposed resolution section complete. I'll add that I have had issues pushed back by committers for the lack of a proposed resolution.
I have only glanced at the MR and I see that it is on 10.2. Since the diff will be applied to HEAD first this should be on 11.x.
And I do think this will need tests. And based on #7.1 I'd like to know if the tests added π Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed were sufficient, it seems not if the type a configuration setting could change. But I can't look into that now.
- π¨π¦Canada threeg Vancouver
I'm not sure if it makes sense for me to be the one to complete the proposed resolution as I would be working backwards from the work I've already done.
In my opinion, as described on the original issue, blank lines entered into the field get saved to config as blank array elements. IE entering in a bunch of spaces and line breaks results in config like so:
notification: emails: - '' - '' - ''
This then causes the issue, because these come back as a keyed array of empty values when sending out the notifications. empty() doesn't see this as an empty array.
I believe the solution should take into account that 1. empty lines not be recorded and 2. if all lines are empty, then the config should instead be stored as an empty array / sequence
notification: emails: { }
This resolves the issue going forward, but not for sites that have already exported the incorrect config. I'm not sure if it makes sense to spread a wider net to capture empty values or rather correct the incorrect config.
I can apply the fix to HEAD. It looks like it would be near identical. But I will hold off in case, there is a better solution that's needed here.
As for tests in #3382510, it doesn't look like there were any. The issue was around introducing
ConfigTarget()
, but how it's been implemented on the Update Settings form is unique to it and the logic defined in it's from and to methods. - πΊπΈUnited States xjm
MRs need to be created against 11.x first. Thanks!