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

Freeze readonly fields #20204

Merged
merged 1 commit into from
May 11, 2021
Merged

Freeze readonly fields #20204

merged 1 commit into from
May 11, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

alternate to #20197

@civibot
Copy link

civibot bot commented May 2, 2021

(Standard links)

@colemanw
Copy link
Member

colemanw commented May 2, 2021

@eileenmcnaughton as a follow-up (not a blocker) I think we should add readonly to the APIv4 output. Would it make more sense on get or getFields? And how should set be handled in the case of readonly?

@demeritcowboy
Copy link
Contributor

Thanks @eileenmcnaughton this works for me and also still satisfies formRules etc.

@pradpnayak if you can confirm this also works for you then I think it's mergeable.

The @todo comment isn't necessary anymore in the trait, but that's minor.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I guess it could work on getfields - it's just a getMandatory() look up when compiling

@colemanw
Copy link
Member

colemanw commented May 3, 2021

Ok @eileenmcnaughton I added it to #20191

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label May 7, 2021
@demeritcowboy
Copy link
Contributor

@pradpnayak does this fix the problem with the logging setting for you? I think it's good to merge.

@pradpnayak
Copy link
Contributor

Yes it does,

thanks @eileenmcnaughton and @demeritcowboy

@colemanw colemanw merged commit 1b97efe into civicrm:master May 11, 2021
@colemanw colemanw deleted the freeze branch May 11, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants