Update casing on setting metadata for html_type = text #13048

eileenmcnaughton · 2018-11-01T07:27:36Z

Overview

Fixes an unreleased regression

Before

notices on urls screen

After

notices fixed

Technical Details

We have conventionally 'preferred' quick_form_type but we we not 'prefer' the more generic html_type.

We are fixing the places where the casing is inconsistent to support this

We have conventionally 'preferred' quick_form_type but we we not 'prefer' the more generic html_type. We are fixing the places where the casing is inconsistent to support this

civibot · 2018-11-01T07:27:38Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

totten · 2018-11-01T07:32:18Z

Does this signal a change in the metadata format? What would be the implication for extensions which have similar metadata?

[nix-shell:~/bknix/build/universe]$ grep html_type $( find *.* -iname '*.setting.php' )

au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => 'Text',
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => 'Text',
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => NULL,
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => '',
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => '',
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => NULL,
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => 'Text',
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => NULL,
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => '',
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => NULL,
au.com.agileware.civiquickbooks/settings/civiquickbooks.setting.php:    'html_type' => '',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Select',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Select',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Text',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Select',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Select',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Checkbox',
com.agiliway.civicalendar/settings/civicalendar.setting.php:    'html_type' => 'Text',
com.cividesk.email.sparkpost/settings/Sparkpost.setting.php:    'html_type' => 'password',
com.cividesk.email.sparkpost/settings/Sparkpost.setting.php:    'html_type' => 'text',
com.cividesk.email.sparkpost/settings/Sparkpost.setting.php:    'html_type' => 'radio',
com.giantrabbit.civimailchimp/settings/CiviMailChimp.setting.php:    'html_type' => 'Text',
com.giantrabbit.civimailchimp/settings/CiviMailChimp.setting.php:    'html_type' => 'Text',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => 'Select',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => 'Select',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => 'Select',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => '',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => '',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => 'Text',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => 'Text',
com.joineryhq.activityical/settings/Activityical.setting.php:    'html_type' => 'Text',
com.joineryhq.percentagepricesetfield/settings/Percentagepricesetfield.setting.php:    'html_type' => '',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'text',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'checkbox',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'checkbox',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'checkbox',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'checkbox',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'checkbox',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'text',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'checkbox',
com.osseed.eventcalendar/settings/EventCalendar.setting.php:    'html_type' => 'text',
com.webaccessglobal.simpledonate/settings/SimpleDonation.setting.php:    'html_type' => 'Select',
com.webaccessglobal.simpledonate/settings/SimpleDonation.setting.php:    'html_type' => 'Checkbox',
de.systopia.donrec/settings/donrec.setting.php:    'html_type' => 'Select',
de.systopia.donrec/settings/donrec.setting.php:    'html_type' => 'Select',
nz.co.fuzion.civitoken/settings/Civitoken.setting.php:  'html_type' => 'Checkboxes',
nz.co.fuzion.pageredirect/settings/PageRedirect.setting.php:    'html_type' => 'Text',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Select',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Select',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Select',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Select',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Text',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Text',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Text',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Text',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Text',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Checkbox',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Checkbox',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Checkbox',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Checkbox',
org.civicrm.smartdebit/settings/smartdebit.setting.php:    'html_type' => 'Text',
uk.artfulrobot.civicrm.autogroup/settings/autogroup.setting.php:    'html_type' => 'select',
uk.co.vedaconsulting.mosaico/settings/Mosaico.setting.php:    'html_type' => 'Select',
uk.co.vedaconsulting.mosaico/settings/Mosaico.setting.php:    'html_type' => 'Text',
uk.co.vedaconsulting.mosaico/settings/Mosaico.setting.php:    'html_type' => 'Text',

eileenmcnaughton · 2018-11-01T07:33:56Z

@totten nothing if they aren't overriding our inbuilt setting form (which it technically not supported) but if we find that once the core ones are done there is pain around extensions we can support the casing variant with a deprecation notice

eileenmcnaughton · 2018-11-01T07:35:06Z

I just checked the first of these & it doesn't extend but uses a variant on the civixero form - which I would think is most common https://github.com/agileware/au.com.agileware.civiquickbooks/blob/master/CRM/Civiquickbooks/Form/Settings.php

eileenmcnaughton · 2018-11-01T07:36:39Z

Ditto the second on the list https://github.com/agiliway/com.agiliway.civicalendar/blob/master/CRM/Calendar/Form/Settings.php

eileenmcnaughton · 2018-11-01T07:40:12Z

OK - com.cividesk.email.sparkpost DOES extend the core form - I think I will do a PR against it - will check the last handful

eileenmcnaughton · 2018-11-01T07:41:38Z

com.giantrabbit.civimailchimp doesn't

eileenmcnaughton · 2018-11-01T07:43:04Z

com.joineryhq.activityical/ uses the CIviXero code

eileenmcnaughton · 2018-11-01T07:44:35Z

com.joineryhq.percentagepricesetfield uses CiviXero code

eileenmcnaughton · 2018-11-01T07:45:33Z

com.osseed.eventcalendar does not extend core form

eileenmcnaughton · 2018-11-01T07:46:16Z

com.webaccessglobal.simpledonate extends core

totten · 2018-11-01T07:47:25Z

"extend core form" ==> I'd expect most extend CRM_Core_Form. Are you looking for classes which extend CRM_Admin_Form_Setting?

I'm still updating my copy of universe, but this might be an easy to spot those:

[nix-shell:~/bknix/build/universe]$ svngrep 'extends CRM_Admin_Form_Setting' *.*
ca.bidon.regionlookup/CRM/Admin/Form/Setting/RegionLookup.php:class CRM_Admin_Form_Setting_RegionLookup extends CRM_Admin_Form_Setting {
ca.bidon.reporterror/CRM/ReportError/Admin/Form/Settings.php:class CRM_ReportError_Admin_Form_Settings extends CRM_Admin_Form_Setting {
com.aghstrategies.smartystreets/CRM/Admin/Form/Setting/SmartyStreets.php:class CRM_Admin_Form_Setting_SmartyStreets extends CRM_Admin_Form_Setting {
com.cividesk.email.sparkpost/CRM/Admin/Form/Setting/Sparkpost.php:class CRM_Admin_Form_Setting_Sparkpost extends CRM_Admin_Form_Setting {
com.cividesk.helptab/CRM/Admin/Form/Setting/HelpTab.php:class CRM_Admin_Form_Setting_HelpTab extends CRM_Admin_Form_Setting {
com.cividesk.normalize/CRM/Admin/Form/Setting/Normalize.php:class CRM_Admin_Form_Setting_Normalize extends CRM_Admin_Form_Setting {
com.ixiam.modules.quicksearch/CRM/Quicksearch/Form/Setting.php:class CRM_Quicksearch_Form_Setting extends CRM_Admin_Form_Setting {
com.webaccessglobal.simpledonate/CRM/SimpleDonate/Form/SimpleDonationSetting.php:class CRM_SimpleDonate_Form_SimpleDonationSetting extends CRM_Admin_Form_Setting {
de.systopia.donrec/CRM/Admin/Form/Setting/DonrecSettings.php:class CRM_Admin_Form_Setting_DonrecSettings extends CRM_Admin_Form_Setting
de.systopia.householdmerge/CRM/Admin/Form/Setting/Household.php:class CRM_Admin_Form_Setting_Household extends CRM_Admin_Form_Setting {
nz.co.fuzion.pageredirect/CRM/Pageredirect/Form/Admin/Form/Setting/CustomRedirect.php:class CRM_Pageredirect_Form_Admin_Form_Setting_CustomRedirect extends CRM_Admin_Form_Setting {
uk.artfulrobot.civicrm.autogroup/CRM/AutoGroup/Form/Settings.php:class CRM_AutoGroup_Form_Settings extends CRM_Admin_Form_Setting {
uk.co.vedaconsulting.mosaico/CRM/Mosaico/Form/MosaicoAdmin.php:class CRM_Mosaico_Form_MosaicoAdmin extends CRM_Admin_Form_Setting {

eileenmcnaughton · 2018-11-01T07:49:36Z

@totten yes that is helpful - The Fuzion one is probably not really supported but I'll do PRs against them all once we get into rc mode & have all the parts merged

eileenmcnaughton · 2018-11-01T07:50:23Z

(I think can feel some personal shame on the Fuzion one - but in my defence that was a LONG time ago)

eileenmcnaughton · 2018-11-01T07:51:30Z

@totten do any extend CRM_Admin_Form_Preferences?

totten · 2018-11-01T07:57:17Z

Happily, nobody extends CRM_Admin_Form_Preferences.

Also, I updated universe and searched again -- the list remained the same.

Honestly, I think cleanup like this PR is good, but I'd advocate for the settings system to have a little automatic cleanup/normalization (and maybe status-check/deprecation-notice) for backwards-compat. It's going to be hard to coordinate code updates and deployments with that many extensions (esp. considering that universe only has published extensions).

eileenmcnaughton · 2018-11-01T08:07:36Z

Interestingly I downloaded https://github.com/systopia/de.systopia.donrec & checked & the form seems fine (uses select fields not text which seem fine) but it does have unrelated enotices where it has accessed a core function it probably shouldn't. Anyway once the last core PRs are merged before the rc I will do some more digging on the extensions & some more comms

eileenmcnaughton · 2018-11-01T08:13:43Z

@mattwire please take a look at the above - for those extensions that DO extend the settings form I intend to do a PR against them with an info.xml change to 5.8 & change the extends to CRM_Core_Form wtih a use SettingsTrait

I also think we should consider keeping 'wrong cases' working in core with just a bit of deprecation notice noise - but I propose to get the current PRs merged first

mattwire · 2018-11-01T10:35:59Z

@eileenmcnaughton Confirmed that without this PR some of the settings forms are broken (eg directories/resource URLS). With this PR they are fixed. I have come across this inconsistency with case before and don't really see why the html_type needs to be case-sensitive at all, but at least if it is going to be then it should be consistent (ie. all lowercase) as that seems more logical to a developer (otherwise you run into issues with things like "TextArea" or "Textarea".

This PR needs to be merged as currently core settings forms are broken. But I would like to see a follow-up PR that relaxes the case-sensitivity requirement (with a deprecation warning is fine).

eileenmcnaughton · 2018-11-01T10:57:48Z

@mattwire thanks - merging. I agree with your comments - once these are merged we should update docs & then look at casing

eileenmcnaughton · 2018-11-05T00:55:44Z

I've added #13062 to switch to deprecation notices

eileenmcnaughton · 2018-11-05T01:12:57Z

I created a PR against Mosaico here veda-consulting-company/uk.co.vedaconsulting.mosaico#276

I won't look at the others until my settings updates are made here civicrm/civicrm-dev-docs#555 & the Generic form is merged here #13054

eileenmcnaughton · 2018-11-05T01:43:55Z

Of the above
ca.bidon.regionlookup - doesn't 'really' use parent form - not affected
ca.bidon.reporterror - fully overrides 'buildQuickform - not affected
com.aghstrategies.smartystreets - is kinda weird WRT the form - would need testing - it may already not work - I would say that based on the xml we can ignore it - no mention of supporting 4.6 or greater (ping @agh1 )
com.cividesk.email.sparkpost - fully overrides the parent form - but might have an enotice as it doesn't define $_settings - I think best idea would be to break the extends - I have logged cividesk/com.cividesk.email.sparkpost#66
com.cividesk.helptab should be fine - it fully overrides the parent buildForm & $_settings is defined. Probably it gets no value from extending the parent & that was a boilerplate decision
com.cividesk.normalize - ditto -
com.ixiam.modules.quicksearch uses setting form but doesn't define html_type = fine
com.webaccessglobal.simpledonate overrides parent. Will probably have an e-notice about not defining $_settings (not necessarily new) - probably not supported any more
de.systopia.donrec fully overrides buildForm. I did log an issue about pre-existing e-notices
de.systopia.householdmerge fully overrides buildForm - will probably have enotices so I logged systopia/de.systopia.householdmerge#9
nz.co.fuzion.pageredirect - probably not used now but I fixed - eileenmcnaughton/nz.co.fuzion.pageredirect@dc5641f
uk.artfulrobot.civicrm.autogroup - already using lower case
uk.co.vedaconsulting.mosaico veda-consulting-company/uk.co.vedaconsulting.mosaico#276

Upshot - mosaico is hte only one of the above that seems to need updating for recent changes. I logged an issue for that

A few others will have e-notices - that may or may not have preceeded recent changes - they are basically the systopia & cividesk ones. I have logged tickets against at least one repo for each of those

agh1 · 2018-11-06T21:33:24Z

@eileenmcnaughton we have already been operating under the assumption that com.aghstrategies.smartystreets may not work with 4.7/5.x - the client we built it for ended up discontinuing the service well before they upgraded. My expectation is that it shouldn't take much to make it compatible, but it's so dependent upon UI specifics that it would need attention anyway.

eileenmcnaughton · 2018-11-06T22:34:16Z

@agh1 good to know - thanks

Update casing on setting metadata for html_type = text

c1a2145

We have conventionally 'preferred' quick_form_type but we we not 'prefer' the more generic html_type. We are fixing the places where the casing is inconsistent to support this

civibot bot added the master label Nov 1, 2018

eileenmcnaughton mentioned this pull request Nov 1, 2018

Notices on settings screen systopia/de.systopia.donrec#64

Closed

eileenmcnaughton merged commit 3983313 into civicrm:master Nov 1, 2018

eileenmcnaughton deleted the resource-urls branch November 1, 2018 10:57

eileenmcnaughton mentioned this pull request Nov 5, 2018

Fix handling of advmutltiselect (on localization form). Handle casing issues #13062

Merged

This was referenced Nov 5, 2018

enotice on settings form systopia/de.systopia.householdmerge#9

Open

Probably enotice on settings form cividesk/com.cividesk.email.sparkpost#66

Open

eileenmcnaughton mentioned this pull request Nov 5, 2018

dev/core/495 Introduce generic settings form (fully metadata driven) #13054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update casing on setting metadata for html_type = text #13048

Update casing on setting metadata for html_type = text #13048

eileenmcnaughton commented Nov 1, 2018

civibot bot commented Nov 1, 2018

totten commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

totten commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

totten commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018 •

edited

Loading

mattwire commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 5, 2018

eileenmcnaughton commented Nov 5, 2018

eileenmcnaughton commented Nov 5, 2018

agh1 commented Nov 6, 2018

eileenmcnaughton commented Nov 6, 2018

Update casing on setting metadata for html_type = text #13048

Update casing on setting metadata for html_type = text #13048

Conversation

eileenmcnaughton commented Nov 1, 2018

Overview

Before

After

Technical Details

civibot bot commented Nov 1, 2018

totten commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

totten commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

totten commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018 • edited Loading

mattwire commented Nov 1, 2018

eileenmcnaughton commented Nov 1, 2018

eileenmcnaughton commented Nov 5, 2018

eileenmcnaughton commented Nov 5, 2018

eileenmcnaughton commented Nov 5, 2018

agh1 commented Nov 6, 2018

eileenmcnaughton commented Nov 6, 2018

eileenmcnaughton commented Nov 1, 2018 •

edited

Loading