Extend allowable resources available via WordPress/WordPress. #1721

peterwilsoncc · 2024-08-31T23:17:47Z

Motivation for the change, related issues

This extends the allowable resources available via the wordpress-branch query string parameter to include all available resources.

I've also improved the robustness of the code by getting the location header from the official URL rather than the destination URL.

Follow up to #1705.

Implementation details

Which resource to download is determined by the the form the wordpress-branch parameter:

the HEAD of any branch in the form x.x or x.x-branch,
a specific tag in the form x.x.x, normalizing the value for major releases,
a specific commit in the form [a-z0-9]{7,40}

To allow for the querying of the location header, I've needed to make the PluginDownloader::gitHubRequest() method public. If there is an alternative approach, please let me know.

Testing Instructions (or ideally a Blueprint)

In the command line run the following commands:

PLAYGROUND_URL=http://localhost:9999 npx nx run playground-website:build:wasm-wordpress-net
php -S localhost:9999 -t dist/packages/playground/wasm-wordpress-net

Blueprint configs using the localhost:

This extends the allowable resources available via the `wordpress-branch` query string parameter to include all available resources: * the HEAD of any branch in the form x.x or x.x-branch, * a specific tag in the form x.x.x, normalizing the value for major releases, * a specific commit in the form [a-z0-9]{7,40} Determining the location of the zip file is improved to allow for the dynamic nature of the final URLs. A request is first made to the offical URL to retrieve the location header for the final zip file.

peterwilsoncc · 2024-09-01T21:49:19Z

packages/playground/website/public/plugin-proxy.php


        $downloader->streamFromGithubReleases($_GET['repo'], $_GET['name']);
    } else if ( isset( $_GET['wordpress-branch'] ) ) {
        $branch = strtolower( $_GET['wordpress-branch'] );


While this is undocumented, should it be renamed to wordpress-artifact or similar given it now allows for more than just branches? Naming things suggestions are welcome.

I'd love to see this all documented, ideally before the previous PR merged.
git talks about refs and shas, where a branch and tag are both a kind of ref.

perhaps the query arg could be something like &git-ref=wordpress/wordpress-develop#0ac256d5fb or &git-ref=wordpress/gutenberg#trunk

we could allow only specific repositories, but this syntax would be flexible enough to allow more than initially planned without changing the public interface

There's already repo and name but it assumes a plugin release rather than a WordPress install.

wordpress-playground/packages/playground/website/public/plugin-proxy.php

Lines 330 to 336 in eb0985e

} else if (isset($_GET['repo']) && isset($_GET['name'])) {

// Only allow downloads from the block-interactivity-experiments repo for now.

if ($_GET['repo'] !== 'WordPress/block-interactivity-experiments') {

throw new ApiException('Invalid repo. Only "WordPress/block-interactivity-experiments" is allowed.');

}

$downloader->streamFromGithubReleases($_GET['repo'], $_GET['name']);

I could use WordPress/WordPress=artifact although it's potentially confusing as parameter names are case sensitive.

if we have repo and name then what about ref as the third one?

if we have repo and name then what about ref as the third one?

We could do that as there's already some magic around the two

wordpress-playground/packages/playground/website/public/plugin-proxy.php

Lines 279 to 280 in eb0985e

$downloader->streamFromDirectory($_GET['theme'], PluginDownloader::THEMES);

} else if (isset($_GET['org']) && isset($_GET['repo']) && isset($_GET['workflow']) && isset($_GET['pr']) && isset($_GET['artifact'])) {

So check for isset($_GET['org']) && isset($_GET['repo']) && isset($_GET['ref'])?

Although I'd like to understand the purpose better, it's unlikely we'll accept other WordPress forks as wordpress.zip. https://github.com/Woofpress/Woofpress will need to set up their own playground, for example.

Why wouldn't that work for Woofpress/Woofpress, though?

Related – eventually I want to try hosting a generic CORS proxy and forks may be able to use that. That is, unless GitHub rate limits these requests. But even then, it may still work via an in-browser git clone.

@peterwilsoncc this is unimportant and tangential to the discussion, but passing multiple arguments to isset() is effectively the same as calling it in an && chain.

} else if ( isset( $_GET['org'], $_GET['repo'], $_GET['ref'], $_GET['workflow'], $_GET['pr'], $_GET['artifact'] ) )

https://www.php.net/manual/en/function.isset.php

So check for isset($_GET['org']) && isset($_GET['repo']) && isset($_GET['ref'])?

We can probably default $_GET['ref'] so it's not necessary. Specifying a repo should return the latest HEAD from its main branch, no?

Would the $_GET['ref'] and $_GET['pr'] arguments be mutually exclusive?

dmsnell · 2024-09-02T19:44:17Z

packages/playground/website/public/plugin-proxy.php

+            }
+            $prefix = 'refs/tags/';
+            // If the branch is in the form [a-f0-9]{7,40} it's a commit hash.
+        } elseif ( preg_match( '/^[a-f0-9]{7,40}$/', $branch ) ) {


is there a compelling reason to arbitrarily constrain this to seven characters?
git itself imposes a minimum length of 4 characters for short ids. core.abbrev

https://git-scm.com/book/en/v2/Git-Internals-Git-References

sometimes I think we could eliminate some confusion by using refs as a term so that we don't have to create multiple new terms to describe branches, tags, commit SHAs, etc…

@dmsnell GitHub displays short hashes as seven characters. I can change it to six if you wish but anything shorter will throw a 404.

Five characters: WordPress/WordPress@eac35
Six characters: WordPress/WordPress@eac356

@peterwilsoncc Hm. well it looks like Github is the one pushing arbitrary limits. Still, it's not a guarantee that seven characters will return a real SHA if there's a seven-character clash, as is the case for these:

WordPress/WordPress@733e06315a (works)

WordPress/WordPress@733e06360c (works)

WordPress/WordPress@733e063 (404)

We can't resolve that though since the problem is their system. git will display as many characters as it needs to resolve clashes, with a minimum of four characters. Github apparently displays seven even if that's ambiguous.

Github apparently displays seven even if that's ambiguous.

Hilarious

In my WordPress/WordPress checkout git rev-parse --short HEAD shows ten characters, b428e1b5be, so I guess that's the minimum required to avoid conflicts.

What's your recommendation here, leave as is, increase to 10 or require the full hash?

@peterwilsoncc I think the main point I was trying to make was that short refs are just a display thing and not inherent. what works today might not work tomorrow. if you set core.abbrev = 4 and re-run git rev-parse --short HEAD it should show four characters.

this is all fine in the PR as suggested: I didn't realize Github imposed the restrictions it does.

adamziel

This looks good to me, thank you for this work @peterwilsoncc! Would you like to get this merged, or is there still something you wanted to change?

peterwilsoncc · 2024-09-06T22:21:33Z

Why wouldn't that work for Woofpress/Woofpress, though?

We could set it up to work as an allowed repo, but I don't think it would be wise. I chose Woofpress because it's a satirical fork but I think we'd need to limit repos so we don't end up with one that includes wp_script_enqueue( 'bitcoin-mining' ) running on the wordpress.net domain.

Would the $_GET['ref'] and $_GET['pr'] arguments be mutually exclusive?

Yes, they are as PR pulls from WordPress/WordPress-Develop whereas the new argument pulls from WordPress/WordPress

Would you like to get this merged, or is there still something you wanted to change?

I decided to rename the parameter to build-ref as it better represents the wider nature of the parameter. I've also pushed some documentation in the file packages/docs/site/docs/blueprints/08-examples.md.

I'm now happy for merge if you are.

These are the revised blueprints against localhost:9999

peterwilsoncc added 2 commits August 29, 2024 15:59

Accept WordPress tag and commit references.

84c94bf

peterwilsoncc added the [Type] Enhancement New feature or request label Aug 31, 2024

peterwilsoncc requested a review from adamziel August 31, 2024 23:17

Merge branch 'trunk' into wordpress-commit-and-tags

bd03e98

peterwilsoncc commented Sep 1, 2024

View reviewed changes

peterwilsoncc self-assigned this Sep 1, 2024

dmsnell reviewed Sep 2, 2024

View reviewed changes

adamziel approved these changes Sep 6, 2024

View reviewed changes

peterwilsoncc added 2 commits September 7, 2024 07:58

Rename query string parameter.

c261d78

Document build-ref parameter.

ed8b126

bgrgicak merged commit 7f5720e into trunk Sep 12, 2024
5 checks passed

bgrgicak deleted the wordpress-commit-and-tags branch September 12, 2024 07:21

peterwilsoncc mentioned this pull request Sep 13, 2024

Replace nightly option with trunk. #1763

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extend allowable resources available via WordPress/WordPress. #1721

Extend allowable resources available via WordPress/WordPress. #1721

Uh oh!

peterwilsoncc commented Aug 31, 2024

Uh oh!

peterwilsoncc Sep 1, 2024

Uh oh!

dmsnell Sep 2, 2024

Uh oh!

peterwilsoncc Sep 2, 2024

Uh oh!

dmsnell Sep 2, 2024

Uh oh!

peterwilsoncc Sep 2, 2024

Uh oh!

adamziel Sep 6, 2024

Uh oh!

dmsnell Sep 6, 2024

Uh oh!

dmsnell Sep 2, 2024 •

edited

Loading

Uh oh!

peterwilsoncc Sep 2, 2024

Uh oh!

dmsnell Sep 2, 2024

Uh oh!

peterwilsoncc Sep 2, 2024

Uh oh!

dmsnell Sep 3, 2024

Uh oh!

adamziel left a comment

Uh oh!

peterwilsoncc commented Sep 6, 2024

Uh oh!

Uh oh!

Uh oh!

	} else if (isset($_GET['repo']) && isset($_GET['name'])) {
	// Only allow downloads from the block-interactivity-experiments repo for now.
	if ($_GET['repo'] !== 'WordPress/block-interactivity-experiments') {
	throw new ApiException('Invalid repo. Only "WordPress/block-interactivity-experiments" is allowed.');
	}

	$downloader->streamFromGithubReleases($_GET['repo'], $_GET['name']);

	$downloader->streamFromDirectory($_GET['theme'], PluginDownloader::THEMES);
	} else if (isset($_GET['org']) && isset($_GET['repo']) && isset($_GET['workflow']) && isset($_GET['pr']) && isset($_GET['artifact'])) {

Extend allowable resources available via WordPress/WordPress. #1721

Extend allowable resources available via WordPress/WordPress. #1721

Uh oh!

Conversation

peterwilsoncc commented Aug 31, 2024

Motivation for the change, related issues

Implementation details

Testing Instructions (or ideally a Blueprint)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmsnell Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamziel left a comment

Choose a reason for hiding this comment

Uh oh!

peterwilsoncc commented Sep 6, 2024

Uh oh!

Uh oh!

Uh oh!

dmsnell Sep 2, 2024 •

edited

Loading