4.11.0-beta1 PHP 8.1 - Embed media button broken

[emteknetnz]

Server error when attempting to embed media

I saw this in logs for an unrelated issue, though likely it's the reason why inserting media wasn't working

error-log.ERROR: Uncaught Exception Error: "Call to private Embed\Http\CurlDispatcher::__construct() from scope SilverStripe\Core\Injector\InjectionCreator" at /sites/<mysite>/releases/20220506003258/vendor/silverstripe/framework/src/Core/Injector/InjectionCreator.php line 17 {"exception":"[object] (Error(code: 0): Call to private Embed\\Http\\CurlDispatcher::__construct() from scope SilverStripe\\Core\\Injector\\InjectionCreator at /sites/<mysite>/releases/20220506003258/vendor/silverstripe/framework/src/Core/Injector/InjectionCreator.php:17)"} []

This happened because we recently changed - https://github.com/silverstripe/silverstripe-framework/blob/4/src/Core/Injector/InjectionCreator.php to use new $class(...$values) rather than Reflection. CurlDispatcher has a private constructor, which is allowed, because you're expected to use it via public static function fetch which calls new static() which does have access to the private constructor

Either revert the change, or update calls to CurlDispatcher to use public static fetch()

Update

• Reflection does not work, and it's not PHP related either - I got the following error in PHP 7.4 using reflection: [Emergency] Uncaught ReflectionException: Access to non-public constructor of class Embed\Http\CurlDispatcher

ACs

• Confirm if reverting the change to InjectorCreator fixes issue i.e. Reflection allows private constructors
• (NA) If Reflection does allow private constructors then add a try catch to fallback to Reflection
• If this isn't the case, and it's related to PHP 8.1, then update calls to CurlDispacter to use the public static method

PRs

• ENH Allow multiple backtick variables in a single value #10312
• ENH Use guzzle for oembed #10311
• FIX Update oembed config cwp-core#107

[emteknetnz]

Can replicate locally with a modified oembed.yml file. Issue related to environments with an SS_OUTBOUND_PROXY environment variable defined - https://github.com/silverstripe/cwp-core/blob/2/_config/oembed.yml#L7 - Only in that scenario will an Embed\Http\CurlDispatcher attempt to get instantiated by Injector.

Embed v3 CurlDispatcher had a public constructor, v4 has a private constructor - you're not supposed to instantiate this class, instead you're supposed to call CurlDispatcher::fetch()

The constructors are also fundamentally different

We need to set the CURLOPT_PROXY and CURLOPT_PROXYPORT curl options, however the CurlDispatcher constructor no longer allows this. And it's a final class so it cannot be subclassed, so we're not able to create a custom MyCurlDispatcher within framework to work around this, instead we'd need to resort to copy-pasting the whole class.

[emteknetnz]

I think the solution here is to swap out the Client from Curl to Guzzle, which is allowed via the use of PSR standards, and then configure Guzzle to set the proxy variables

The oembed.yml in cwp-core <=2.10 is incompatible with embed v4 which is introduced in framework 4.11, so we'll need to add a composer conflict in

The oembed.yml in framework defines a 'Crawler' which, with its empty constuctor, will default to creating a CurlClient which itself calls CurlDispatcher::fetch()

So we should create a Guzzle Client service class and ideally configure it with proxy variables in cwp-core 2.11 yml. We can should delete the old curl config from cwp-core

[maxime-rainville]

Great investigation!

Just blinding looking at the Crawler constructor, it looks like we could instantiate our own CurlClient object and call setSettings on it to define a CURLOPT_PROXY value.

All things being equal, my preference would be to stick with CURL if only for simplicity.

We'll need to document this in the changelog as it might trip other people.

[GuySartorelli]

@emteknetnz This is in peer review but the PRs are in draft - can you please set them to ready if they're ready, or else indicate what it is that should be reviewed at this stage?

[emteknetnz]

I've set them to ready for review

[GuySartorelli]

Linked PRs are all merged.