Don't call session_start() before CMS bootstrap (PHP 7.2 compat)

[mfb]

Overview

CiviCRM's extern/rest.php calls session_start() before bootstrapping the CMS. However, configuring the session is one of the bootstrap steps for Drupal (and perhaps other CMS), and on PHP ≥ 7.2, you cannot configure the session after starting it. In addition, generally speaking, Drupal does not want anything besides itself to start the session. This is presumably why there is some logic in CRM_Core_Session::initialize() for starting the session differently on Drupal.

Before

When sending a request to extern/rest.php on PHP ≥ 7.2, the following error is logged: Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in drupal_environment_initialize() (line 695 of includes/bootstrap.inc). In addition, other errors/notices in Drupal modules may be logged.

After

We no longer call session_start() in extern/rest.php. I believe this should work because commit f320e5c should result in a session being initialized during CRM_Utils_REST::loadCMSBootstrap(). If not.. then more work is needed here :)

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

[seamuslee001]

paging @totten this looks good to me @mfb

[totten]

On general policy grounds, 👍 -- it definitely sounds better to let the normal bootstrap process handle the session start.

In theory, the test-coverage in E2E_Extern_RestTest should show if the REST endpoint works. It'd be smart to check the results of the test on each CMS (although, in fairness, I think some CMS's have been failing for quite a while 🙁 - a fact which became visible when we added CiviCRM-E2E-Matrix a few weeks ago... fixing those is in the backlog...). However, I'm not sure the test would reveal issues in sessions, and we do want userID to work correctly... so I'd lean toward expanding RestTest to check that userID is accurate (i.e. call Contact.get id=user_contact_id and assert that it returns the expected ID).

Aside: Actually... creating any kind of session during execution of extern/rest.php is probably extraneous. In the current REST protocol, there's no such thing as a login -- you just resubmit the authentication token on every request, which makes it a stateless affair. The only real reason I can see to have any session is happenstance (the conventional representation of "current user ID" is stored in the CRM_Core_Session)... but the session-storage could ideally be ephemeral/thread-local (so that we don't waste resources on unused sessions). Alas, that doesn't make this job any easier. So you don't need to think about the performance optimization now. But I'm just saying... if there were an easy way to mostly avoid persistent sessions for these requests... that would be neat.

[mfb]

Side note: CiviCRM has some other front controllers that call session_start(), like soap.php - but these should be ok if they don't also bootstrap the CMS. These do result in some "weird" behavior though: Whatever session configuration is defined in the PHP config files will be started up, where otherwise you would expect this configuration to be overridden by Drupal. E.g. session files will show up in the /tmp directory rather than using Drupal's session store.

[seamuslee001]

@totten @mfb added a unit test as per TIm's comments #14076

[xurizaemon]

Meta sidenote: As long as we use extern/*php, we'll have issues with the different bootstrapping in place in those scripts. There's a meta issue at dev/cloud-native#16

If possible, consider using a CMS-bootstrapped endpoint; for extern/rest.php, you may find that civicrm/ajax/rest is a suitable replacement.

(I may be out of date on this front ...)

[mfb]

My understanding was that civicrm/ajax/rest is designed for requests from browser JS. Whereas we're using extern/rest.php to have other apps talk to CiviCRM. Presumably we could have these other apps simulate a browser if we had to, but it's a bit clunky.

[mfb]

I did a deep dive on why we were getting a 500 error (in addition to logging warnings to the error log), because I could not reproduce the 500 error on a barebones Civi+Drupal install.

Turns out that there is an incompatibility with some of the Drupal modules on our site, which I am able to resolve to some extent. So now we just have numerous warnings and notices being logged by this bug, but not the fatal 500 error page. E.g. as a result on this bug, the global $user object does not exist, which Drupal modules are not happy with.

I removed some other superflous comments here because the warnings being thrown in early bootstrap led to weird problems re: loading classes in our error handling code, but I don't think that was another issue in Civi itself.

[seamuslee001]

Jenkins re test this please

[seamuslee001]

@totten are you good with this PR if the tests pass now that #14076 has been merged thanks to Coleman

[mfb]

It's likely that the 3 lines above could also be removed, if we're not actually starting a session here?

if (defined('PANTHEON_ENVIRONMENT')) {
  ini_set('session.save_handler', 'files');
}

[totten]

Seamus's additional E2E test has been merged. Combining this with #14186 shows an almost green E2E matrix.

Let's do it!