dev/core#1558 Data conversion for non-standard setting.

[eileenmcnaughton]

Overview

Transitional step in eliminating non-standard setting 'contribution_invoice_settings' .

Per https://lab.civicrm.org/dev/core/issues/1558 the 'setting'
does not follow our standard of one setting per key and this has led to all sorts of hacks in various places.

Before

The following settings are stored as keys in contribution_invoice_settings

  'credit_notes_prefix' 
  'invoice_prefix' 
  'due_date' 
  'due_date_period' 
  'notes' 
  'tax_term' 
  'tax_display_settings' 

After

The above can still be transitionally set & get by the now deprecated contribution_invoice_settings but can also be correctly set & get - ie

$var = Civi::setttings()->get('credit_notes_prefix');
Civi::setttings()->set('invoice_due_date_period', 'weeks');

For 3 of the settings the new setting name has invoice pre-pended for clarity but the keys retrievable from the deprecated key are unchanged.

  'due_date' => 'invoice_due_date',
  'due_date_period' => 'invoice_due_date_period',
  'notes' => 'invoice_notes',

Technical Details

In trying to unravel this I eventually concluded we needed an interim step where the data would be stored as the correct settings but set and get would still work on the deprecated setting.

The reason for going this way is that I found more than one place where extension code is accessing these settings and the amount of work to fix up all the places in core that access it and the settings form were too bigto deal with without a stepping stone.

With this merged the settings are correctly defined and a definition for the deprecated setting still exists.

Requesting the deprecated setting returns an array retrieved from the correct settings so is unchanged from calling
functions point of view. Saving the deprecated setting saves to the correct places. However, it's now also
possible to get & set the correct settings and once the core places are removed we will deprecate & remove the
deprecated setting.

Unit tests cover past & desired behaviour and the upgrade routine.

Comments

[civibot]

(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).

[eileenmcnaughton]

I tracked down that niggly error which has actually hit us before it was in toggle invoicing - using in_array was returning true & we were unsetting index 1. Since we compare against false (strictly) then array_search would cope with 0 (although logically it would never be zero as the key is appended but....)

[eileenmcnaughton]

This was removing key 1 not key 10 - see below we do a strict comparison on FALSE so a return of 0 is fine.

This was the cause of some weird test errors we occasionally saw

[colemanw]

Concept makes sense. Code looks good. Just waiting for tests to pass.

[JoeMurray]

Thanks for this nice cleanup to others' old code. +1 re stepping stone approach. @Monish or @Seamus could you review if any of our extensions use this? Include review of Cdn Tax Receipts by jake/karin and and Cdn Sales Taxes by us/Mathieu.

[eileenmcnaughton]

@JoeMurray to be clear - they won't break with this change but I agree it's good to fix them.

This is pretty much what the update would look like JMAConsulting/biz.jmaconsulting.lineitemedit#50 - note you would want to have a new release with version set to 5.23 so that only people with 5.23 + get the latest version with the switch over