Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Curl for the FPM health check requests if it's available #9916

Open
3 tasks
techanvil opened this issue Dec 18, 2024 · 3 comments
Open
3 tasks

Use Curl for the FPM health check requests if it's available #9916

techanvil opened this issue Dec 18, 2024 · 3 comments
Assignees
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Dec 18, 2024

Feature Description

At the moment we're unconditionally using file_get_contents() to request the FPM health check URLs.

As discussed on Slack, there can be scenarios where file_get_contents() doesn't work, but using Curl does.

It should also be noted that the measurement.php script itself uses Curl in preference to file_get_contents() if it's installed:

public function sendRequest($url): array
{
if ($this->isCurlInstalled()) {
$response = $this->sendCurlRequest($url);
} else {
$response = $this->sendFileGetContents($url);
}
return $response;
}

We should therefore apply the same logic to the health check requests, using Curl to make the request if it's installed on the server.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • If the Curl PHP extension is installed on the Site Kit host, the FPM health check requests should be made using Curl.

Implementation Brief

Note that the implementation should use code from measurement.php to make the requests, in order to keep the health check logic as close to the proxy behaviour as possible. This will involve reusing the RequestHelper class, as well as possibly routing a request through measurement.php itself.

  • Update includes/Core/Tags/First_Party_Mode/First_Party_Mode.php:

    • Use Google\FirstPartyLibrary\RequestHelper;.
    • Update the is_endpoint_healthy method:
      • Create a new instance of RequestHelper.
      • Get $response by calling the sendRequest method on RequestHelper.
  • Update composer.json adding a files key to the autoload key with the value [ 'fpm/measurement.php' ] to autoload the classes in this file.

  • Update fpm/measurement.php:

    • Update the following conditional block:
      if (!defined('IS_FIRST_PARTY_MODE_TEST')) {
    • Add the condition !defined( 'WPINC' ), this prevents the code from being executed when the classes are included via autoload within the plugin rather than being called directly.
      • Add code comments here to make the changes clear and to ensure we retain these changes in future updates.

Test Coverage

  • No additional test coverage required.

QA Brief

  • Set up Site Kit with the firstPartyMode feature flag enabled and Analytics connected (and/or test with the Ads module).
  • On a site where the FPM health checks are known to be passing, navigate to the Analytics settings edit page and verify the "Your server’s current settings prevent first-party mode from working" warning notice does not appear.
  • Using a site where the FPM health checks are known to be failing for a reason other than the allow_url_fopen PHP setting being disabled on the server, and where Curl is not installed, verify the warning notice still does appear.
  • For sites where the health check has failed because allow_url_fopen is disabled on the server and the Curl extension is installed, the warning should no longer appear because Curl doesn't rely on the allow_url_fopen setting.

If Curl is installed, it will be listed in the Server section of Site Health:
Image

To see if the health checks are failing due to the allow_url_fopen setting, either examine your site's php.ini settings directly to see if the setting is enabled, or install the build for PR #9914 and examine the response of a request to fpm-server-requirement-status:

await googlesitekit.api.get('core','site','fpm-server-requirement-status', undefined, { useCache: false });

image

See this Asana task for more details, and ask @jamesozzie for access to his sites with health check failures which meet the criteria listed above if necessary.

Changelog entry

@techanvil techanvil added P0 High priority Type: Enhancement Improvement of an existing feature labels Dec 18, 2024
@benbowler benbowler self-assigned this Dec 19, 2024
@benbowler
Copy link
Collaborator

If we have more control over measurement.php we could extract the RequestHandler class and import it later within the plugin and measurements.php.

With the above approach we could contribute back the additional conditional logic so that this change is included in future updates.

@benbowler benbowler removed their assignment Dec 19, 2024
@techanvil techanvil self-assigned this Dec 19, 2024
@techanvil
Copy link
Collaborator Author

techanvil commented Dec 19, 2024

Thanks @benbowler, SGTM.

It would be good to feed this back to the FPM library developers... Tagging @aaemnnosttv, who's had a hand in the development.

IB ✅

@techanvil techanvil assigned techanvil and unassigned techanvil Dec 19, 2024
@techanvil
Copy link
Collaborator Author

Actually, further on the above I've noticed this comment in measurement.php, which suggests the RequestHelper class is available as an independent file in the source library that we could consider copying over as well. This would allow us to remove one of our modifications, although it would add ~1.5KB to the build size.

// REQUEST_HELPER_START
/**
* NOTE: DO NOT edit RequestHelper directly nor remove the start and end tags.
*
* This class is copied over from src/RequestHelper.php. If any changes are
* needed, change that file and run the command `npm run copy:RequestHelper`.
*/
/**
* Isolates network requests and other methods like exit to inject into classes.
*/
class RequestHelper

Of course even if it wasn't a separate file we could extract it ourselves, but the fact that it is would make the exercise more within the spirit of just copying things over without modifying them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants