dev/core#3172 Update phpleague/csv from 9.2 to 9.6 (supports php 8)

[eileenmcnaughton]

Overview

Update phpleague/csv to php8-compatible version https://github.com/thephpleague/csv

https://lab.civicrm.org/dev/core/-/issues/3172

Before

phpleague/csv = 9.2

After

phpleague/csv = 9.6

Technical Details

Minimum php version is 7.2.5 - which is a VERY old variant of 7.2

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

[totten]

From a quick grep, primary use-cases appear to be core unit-tests (esp tests/phpunit/CRM/Export/BAO/ExportTest.php) and ext/search_kit/Civi/Api4/Action/SearchDisplay/Download.php.

@eileenmcnaughton @colemanw Does ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDownloadTest.php provide suitable coverage for that functionality? Or does someone need to r-run that?

[eileenmcnaughton]

@totten I think the tests provide enough - it's not got any breaking changes - however, I'm hoping to convert import to use it so will be messing around in related code

[colemanw]

@totten there is a test that would cover this, but I can't get it working - something to do with phpunit processes. I would really love your help getting it to run:

civicrm-core/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDownloadTest.php

Lines 88 to 93 in 8fd58f5

	public function testDownloadCSV() {
	$this->markTestIncomplete('Unable to get this test working in separate process, probably due to being in an extension');
	// Re-enable because this test has to run in a separate process
	\CRM_Extension_System::singleton()->getManager()->install('org.civicrm.search_kit');

[totten]

Oh, I see the test doesn't actually run then. 🙃 I'll take a look.

In the mean-time, I don't want this to be blocked on that - so if anyone's done some r-run, then go ahead and merge.

[eileenmcnaughton]

@totten BAO/ExportTest tests do r-run

[colemanw]

I just did an r-run and SK .csv export continues to work fine. However I noticed that our PR demo sites still do not have SK enabled automatically. I thought we had fixed that in BuildKit.

[totten]

@colemanw OK, so regarding that test-case for SearchDisplay.download...

Problem statement

The root+biggest problem is that SearchDisplay.download is at-odds with the architecture around it -- it uses header() (via League\Csv's sendHeaders()); it uses exit(); and it outputs a typed-string-blob. (Obviously, the contract for APIv4 is to always output using PHP-array-data in the $result object. Internal events and external bindings are both built around this contract.)

It seems like @runInSeparateProcess is an attempt to work-around this (eg asking phpunit to guard against header()/exit()). But it leads to a different error about missing CIVICRM_DSN -- ie the subprocess hasn't booted Civi. The challenge there is that phpunit-listeners don't fire the same way with @runInSeparateProcess (n.b. as it stands, the subprocess doesn't run CiviTestListener::bootHeadless()).

There is a secondary problem as well - the test is tagged with TransactionalInterface, but this only makes sense for single-processing (not compatible with @runInSeparateProcess or e2e). But we have other patterns for cleaning test-data (eg 'TRUNCATE' or 'DELETE'). Let's figure the unusual issue first.

Brainstorm

1. We could normalize the contract, either:
   • Convert this action from APIv4 to a new route like civicrm/ajax/search-display-csv. (There's a probably a reason why you didn't do this from the start, though.)
   • Expand the definition of Civi\Api4\Generic\Result to explicitly allow headers/streams. Update all the bindings that depend on Result. (That'd be a painful adaptation IMHO. The only upshot is that it'd fit well within headless and TransactionalInterface.)
2. Keep the contract. Convert the test from headless style to e2e style. This would be my goto if you need to cover things like header() and exit(), and it also gives better coverage over the CMS fiddly-bits (which sometimes complicate things when you need precise HTTP responses).
3. Keep the contract and keep the headless+@runInSeparateProcess. The subprocess needs to boot Civi. Here's a partial-patch just for testDownloadCSV(): https://gist.github.com/totten/21c050dd14b2f2f1082059c567f42ab0. (Note "FIXME".) If you really want to support @runInSeparateProcess for any-old headless test, that would require a longer think...
4. Use runkit to overload header() and exit(). (But runkit is a bit of a drag on portability b/c it's not a very common PECL.)

I may take a stab tonight at converting to e2e (#2) - but if that doesn't work out, #3 is probably the smallest change.