Switch to CRM_Core_Form::setTitle() instead of CRM_Utils_System::setTitle() part 2

[mattwire]

Overview

Follows on from some work that @eileenmcnaughton started in 2019. Forms should use $this->setTitle() instead of CRM_Utils_System::setTitle() because it allows you to use $this->getTitle() on a form to get the title of the form that the user will see.

Before

Calls to CRM_Utils_System::setTitle()

After

Calls to $this->setTitle()

Technical Details

As can be observed CRM_Core_Form::setTitle() calls CRM_Utils_System::setTitle() internally after setting $this->_title showing that this function is a drop-in replacement. There are just over 200 instances that need converting and (for ease of review) this is the next 34.
I would suggest that r-run testing could be done with a few forms only instead of all of them - what needs to be verified is that it is a form that inherits from CRM_Core_Form (and not eg. a Page).

Comments

There are 4 commits in total that would complete this change and I'll follow up with the others - see https://github.com/civicrm/civicrm-core/compare/master...mattwire:settitle?expand=1

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

@mattwire I think you have correctly identified the biggest risk here - ie the form is a page not a form & we get a fatal.

This is also the sort of review work that is prone to mistakes (and to me wandering off after looking at the first few & losing my place :-))

How about adding the same function to CRM_Core_Page to reduce risk of human error (which seems to be the biggest risk here)?

[totten]

We should probably have a 'smoke test' which is basically an array of paths/params -- fetch each and assert a well-formed response. To the effect of (abridged pseudocode):

/** @group e2e */
class PathSmokeTest {
  public function getExamples() {
    return [
       ['civicrm/dashboard', 'reset=1'],
       ['civicrm/contact/view', 'reset=1&cid=10'],
       // Some URLs need an ID# (like "cid" above). E2E tests run on demo data, so static examples would work for a smoke test
       // Repeat for 100-200 examples.
       // Seed the list with a mix of  `civicrm_navigation` and/or crawling a demo site.
    ];
  }

  public function testExamples($path, $params) {
    $http = $this->createGuzzle(); 
    /* use CookieJar and civicrm/authx/login to setup login session for `admin` user. Similar to AllFlowsTest */
    $resp = $http->get($path, ...$params);
    $this->assertEquals(200, $resp->getStatusCode());
    $this->assertTrue(isHtmlWellFormed($resp->getBody()));
    $this->assertTrue(isHtmlTitleSet($resp->getBody()));
  }

(There may be better/more clever/dynamic arrangements. civicrm_navigation entries would be a good data-source. But URLs like civicrm/contact/view?reset=1&cid=10 are more complicated. The KISS/get-started approach could just list examples for demo data.)

[colemanw]

I needed something mindless to do while waking up this morning so I verified that in all 34 classes the object called inherits from CRM_Core_Form.

[mattwire]

Thanks @colemanw