Cleanup contact summary tabs code

[colemanw]

Overview

This is code cleanup and function extraction which should have no impact on end-users or hooks.

Before

Contact summary tabs in hard-coded order.

After

Contact summary tabs still in the same hard-coded order, but extracted a couple functions to make it possible for the LayoutEditor extension to modify them.

Technical Details

This preserves the current order of tabs, but does slightly change the numeric weights of some of them. They were somewhat unpredictable before, now they all have fixed values.

Comments

Hoping to slip this into 5.7 to meet the Contact Layout Editor release schedule.

[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]

@colemanw I worked through this and it works fine but it alters the expectations for the hook a little. Notably the hook is 'used to' receiving an array with keys that can be referenced - e.g. I want to change the title of the events tab I can change $tabs['events']['title'] in my hook. You have changed them to be numerically indexed (presumably because you didn't think the keys were needed) but that would be a breaking change for the hook

I also think there is an implication extension writers adding hooks would now set the id?

This is the current docs for the hook & it should probably be updated if our best practice has changed https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_tabset/

[totten]

@eileenmcnaughton Agree that different indexing would be a breaking change -- however, I think the indexing changes affects the internal structure ($rest or basicTabs()), but the public $allTabs is still populated through a loop (in which items were and are registered with $allTabs[] = ...).

To test this, I made a small test extension to capture the hook-data (before+after the patch):

function _foobar_file($name) {
  return __DIR__ . '/data/' . CRM_Utils_String::munge($name . '-' . date('Y-m-d-h-i-s'));
}

function foobar_civicrm_tabs() {
  file_put_contents(_foobar_file('tabs'), json_encode(func_get_args(), JSON_PRETTY_PRINT));
}

function foobar_civicrm_tabset() {
  file_put_contents(_foobar_file('tabset'), json_encode(func_get_args(), JSON_PRETTY_PRINT));
}

function foobar_civicrm_summary() {
  file_put_contents(_foobar_file('summary'), json_encode(func_get_args(), JSON_PRETTY_PRINT));
}

The content of hook_civicrm_summary was completely unchanged.

The content of hook_civicrm_tabs and hook_civicrm_tabset were more interesting:

• The general indexing stayed the same.
• All weights were decreased by 30
• One item disappeared from the list:

        {
            "id": "summary",
            "url": "#contact-summary",
            "title": "Summary",
            "weight": 0
        },

[seamuslee001]

Jenkins re test this please

[colemanw]

@totten - thanks for the thorough testing. To your points:

• The general indexing stayed the same.

Correct - this PR does not break the hook contract.

• All weights were decreased by 30

Actually that depends on what components you have enabled. With all components enabled, the weights would be no different before/after this PR. This PR makes them more stable/predictable so they do not shift around when enabling/disabling a component.

• One item disappeared from the list:

You might have done the diff backwards :) Actually this PR adds it to the list, which makes the hook a little more robust -- now it's possible to change the weight of the summary tab and rename it via hook. This is necessary for civicrm/org.civicrm.contactlayout#39 to work.

[eileenmcnaughton]

OK - I feel my concerns have been addressed by the above -merging