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

Allow theme config to set server-side required #1784

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

zerocrates
Copy link
Member

Makes "required" fields more robust, and fixes usage of elements that
Laminas defaults as required (here we're defaulting to not-required).

(fix #1779)

Makes "required" fields more robust, and fixes usage of elements that
Laminas defaults as required (here we're defaulting to not-required).

(fix #1779)
@zerocrates zerocrates requested a review from kimisgold November 29, 2021 17:35
@kimisgold
Copy link
Member

This works as expected. I tested using a date element.

  1. With no "required" attribute defined, the date element defaulted to not required.
  2. Setting attributes.required = false remained not required.
  3. Setting attributes.required = true made the element required, and I could not submit the form until configuring this field.

The one small bump was setting attributes.required = "false" with quotes around the value made it set required="required" on the element, but that may not be an issue with proper documentation or assumed familiarity with setting boolean values in Laminas.

@zerocrates
Copy link
Member Author

Hmm I guess that's just how the ini parser works. Not sure we can really change that. When you wrote it that way it did the whole thing, with the asterisk and all, right?

@kimisgold
Copy link
Member

When you wrote it that way it did the whole thing, with the asterisk and all, right?

Yup! Presentation matched functionality.

@zerocrates zerocrates merged commit a3a4f51 into develop Dec 13, 2021
@zerocrates zerocrates deleted the theme-config-required branch December 13, 2021 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to use date element in theme config
2 participants