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

Networking access: Fix wp_http_supports() to work without the kitchen-sink extension bundle. #1504

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Jun 11, 2024

Fixes #1494

In #1048 I introduced a regression of #819, in that wp_http_supports( [ 'ssl' ] ) no longer returned truthful without the kitchen-sink extension bundle loaded.

This partially reverts #1048 by keeping the filters for Requests, but reinstating the deprecated WP_HTTP filters.

It's worth noting, that this is not a direct revert, as it forces the Fetch/Dummy handlers instead of simply adding them as an option. This is to match the Requests filter behaviour.

To test this, the following blueprint should land you on a plugin install page without any errors visible:
https://playground.wordpress.net/#{%22phpExtensionBundles%22:[%22light%22],%22features%22:{%22networking%22:true},%22landingPage%22:%22/wp-admin/plugin-install.php%22,%22steps%22:[{%22step%22:%22login%22,%22username%22:%22admin%22,%22password%22:%22password%22}]}

{
  "phpExtensionBundles": [
    "light"
  ],
  "features": {
    "networking": true
  },
  "landingPage": "/wp-admin/plugin-install.php",
  "steps": [
    {
      "step": "login",
      "username": "admin",
      "password": "password"
    }
  ]
}

@dd32 dd32 added [Type] Bug An existing feature does not function as intended [Aspect] Networking labels Jun 11, 2024
@dd32 dd32 self-assigned this Jun 11, 2024
Copy link
Collaborator

@bgrgicak bgrgicak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Let's just check in with @adamziel to make sure this won't have any unwanted consequences.

@adamziel
Copy link
Collaborator

adamziel commented Jun 11, 2024

These filters are tricky! Lovely fix, thank you @dd32!

Would you mind also adding an E2E test around here to make sure this regression won't come back in the future?

it('should enable networking when requested AND the kitchen sink extension bundle is enabled', () => {

(The existing one is a duplicate of the one above it and the label is confusing. Kitchen sink is the default now so that test can be rewritten to use ?php-extension-bundle=light)

@dd32 dd32 force-pushed the fix/wp_http_supports branch from 794b03e to 761f594 Compare June 12, 2024 03:20
@dd32
Copy link
Member Author

dd32 commented Jun 12, 2024

I've tried to e2e it, and the test change looks correct to me, but should enable networking when requested AND the kitchen sink extension bundle is NOT enabled still passed when I expected it to fail, and should return true from wp_http_supports(array( "ssl" )) (which I switched to run without the bundle) timed out twice in a row.

I wasn't able to get the tests to run locally, I've only guessed what I'm supposed to do for that (npm test, also tried using npx nx e2e playground-website) as I can't find any related documentation.

@brandonpayton
Copy link
Member

the test change looks correct to me, but should enable networking when requested AND the kitchen sink extension bundle is NOT enabled still passed when I expected it to fail

@dd32 Why were you expecting it to fail?

I wasn't able to get the tests to run locally, I've only guessed what I'm supposed to do for that (npm test, also tried using npx nx e2e playground-website) as I can't find any related documentation.

I struggled with this too and ended up running the following CI e2e testing commands locally:

- run: sudo ./node_modules/.bin/cypress install --force
- run: sudo CYPRESS_CI=1 npx nx e2e playground-website --configuration=ci --verbose

(probably shouldn't prefix with sudo locally though :)

Then I discovered local e2e instructions in the website README:

To run the end to end tests locally, use the following command:
```bash
npx nx run playground-website:e2e:dev:cypress
```

@dd32
Copy link
Member Author

dd32 commented Jun 13, 2024

the test change looks correct to me, but should enable networking when requested AND the kitchen sink extension bundle is NOT enabled still passed when I expected it to fail

@dd32 Why were you expecting it to fail?

Good question; and that might have been the misunderstanding :)
The test run was without this change, which I thought would mean it would use fetch() which would be a CORS request which would then hit a HTTP remote, which CORS would block as it would be cross-origin cross-protocol; but now I realise that the tests are probably not running with a SSL viewport, which means that whether it supported SSL requests or not is mostly irrelevant, the API endpoint would return via HTTP or HTTPS.

What this means, is that the test is likely working as expected, even though it's now without OpenSSL, it probably still passes just fine (Just requests via HTTP rather than HTTPS, both of which are totally acceptable).

@dd32
Copy link
Member Author

dd32 commented Jun 13, 2024

Then I discovered local e2e instructions in the website README:

Ah! Thank you! Developer instructions being in nested readme's gets me all the time when I have no idea which things are distinct components :)

@brandonpayton
Copy link
Member

Then I discovered local e2e instructions in the website README:

Ah! Thank you! Developer instructions being in nested readme's gets me all the time when I have no idea which things are distinct components :)

Same here. :) I logged an issue to add better instructions here since documenting e2e isn't as quick as mentioning a couple of commands at the top level.

@brandonpayton brandonpayton merged commit 95e15a2 into WordPress:trunk Jun 17, 2024
5 checks passed
bgrgicak pushed a commit that referenced this pull request Jun 20, 2024
…-sink extension bundle. (#1504)

Fixes #1494 

In #1048 I introduced a regression of #819, in that `wp_http_supports( [
'ssl' ] )` no longer returned truthful without the kitchen-sink
extension bundle loaded.

This partially reverts #1048 by keeping the filters for Requests, but
reinstating the deprecated WP_HTTP filters.

It's worth noting, that this is not a direct revert, as it forces the
Fetch/Dummy handlers instead of simply adding them as an option. This is
to match the Requests filter behaviour.

To test this, the following blueprint should land you on a plugin
install page without any errors visible:

https://playground.wordpress.net/#{%22phpExtensionBundles%22:[%22light%22],%22features%22:{%22networking%22:true},%22landingPage%22:%22/wp-admin/plugin-install.php%22,%22steps%22:[{%22step%22:%22login%22,%22username%22:%22admin%22,%22password%22:%22password%22}]}
```
{
  "phpExtensionBundles": [
    "light"
  ],
  "features": {
    "networking": true
  },
  "landingPage": "/wp-admin/plugin-install.php",
  "steps": [
    {
      "step": "login",
      "username": "admin",
      "password": "password"
    }
  ]
}
```
adamziel added a commit that referenced this pull request Jul 31, 2024
Suppresses the following deprecation warning:

> PHP Deprecated: Hook http_api_transports is deprecated since version 6.4.0 with no alternative available. in /wordpress/wp-includes/functions.php on line 135

We're forced to use this deprecated hook to ensure SSL operations work without
the kitchen-sink bundled. See #1504
for more context.

 ## Testing instructions

Open devtools. Then open Playground:

* With and without this PR
* With and without networking enabled

Then go to wp-admin. Confirm that with this PR the above deprecation
notice is not logged.

cc @dd32 @bgrgicak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Aspect] Networking [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WP_HTTP reports that it doesn't support SSL requests without the kitchen-sink bundle loaded
4 participants