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

Towards rationalising settings form & preferences form (partial of 12731) #12732

Merged
merged 1 commit into from
Aug 29, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 26, 2018

Overview

Adds a trait class & starts to align Settings form with preferences form

Before

The settings form & the preferences form do the same thing but evolved separately. Settings uses metadata, preferences doesn't.

After

This adds a trait that can be shared & the parts of both that use metadata can use the shared section

Technical Details

Over time we should remove both forms in favour of a new clean one based on the trait. I think the trait would be merged into that form.

Note that one issue TBC is how to define 'serialize' on the form. In the DAO we use a string but the generator converts it to a constant. I used just a constant here.

I can break this down into some smaller commits.

Comments
This is a cut down of #12731 for easier review

@civibot
Copy link

civibot bot commented Aug 26, 2018

(Standard links)

*
* @return array
*/
protected function getSettingsMetaData() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to the trait

@eileenmcnaughton
Copy link
Contributor Author

test this please

@colemanw
Copy link
Member

Looks simple and safe.

@colemanw colemanw merged commit 8fc4bac into civicrm:master Aug 29, 2018
@eileenmcnaughton eileenmcnaughton deleted the setting_form1 branch August 29, 2018 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants