Add WordPress Playground plugin

[seanmorris]

What?

This adds the collector plugin for plugin-preview functionality and renames the plugin to WordPress Playground.

Requires:
WordPress/wordpress-playground#745 .
WordPress/blueprints#49

Why?

Users want to be able to preview plugins on their own sites before actually committing to installing them live.

Testing Instructions

• Checkout the project and copy the plugin to your local WordPress install:

git checkout sm-collector-plugin
cp packages/collector/ [PATH_TO_YOUR_WORDPRESS]/wp-content/plugins/

• Open your local WordPress install and activate the WordPress Playground plugin
• Start Playground by clicking on Tools > Sandbox Site in wp-admin
• After the site loads it should have the same content as your site (files and database)

Testing Previews

• Open the file at [PATH_TO_YOUR_WORDPRESS]/wp-content/plugins/collector/Collector.php. Replace line 81 in line Collector.php with:

const blueprintUrl = (query.get('blueprintUrl') ?? '').replace('https://wordpress.org', 'http://localhost:8010/proxy');

• Start the CORS proxy:

npm install -g local-cors-proxy
npx lcp --proxyUrl https://wordpress.org/

• Navigate to plugins > add new
• Search for "Akismet"
• You should now see a Preview Now button next to the Install Now button.
• Click Preview now, and you should see Playground boot your site with the plugin activated.

[bgrgicak]

@seanmorris I tried testing your PR for a Woo prototype, but couldn't get it to work.
This is what I did:

• In the Playground project checked out your runSQL branch
• Merged trunk into it to get HTTP requests to work
• Started a local Playground
• Installed the collector plugin from your branch (using Plugin upload in the admin UI)
• Plugin installs and activates successfully
• After installing it any page I try to open fails with ERR_FAILED

[seanmorris]

@adamziel Is there a way to perform this pattern using $wpdb? I don't see a way to fetch records from a query sequentially. I've only found a way to load the entire result set into memory, or a single row and that's it. Is there a fetch-like method that I'm missing? I checked the docs, but didn't see one.

Unless $wpdb exposes this functionality, then I'd either have to run the same query multiple times, or load the entire result set into memory at once, neither of which is an optimal solution.

[dawidurbanski]

@seanmorris Maybe this is not greatly documented, but using for example $wpdb->get_results("query"), under the hood, inside the class, it calls $this->query($query) and this one later calls $this->_do_query($query).

Looking at the source code of the last one, you can see that it sets the mysqli_result object (return value of mysqli_query()) in the $wpdb->result field:

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wpdb.php#L2348-L2350

The mysqli_result implements IteratorAggregate so you should be able to call $wpdb->query("query") directly (this doesn't create or return results array, opposite to get_results(), so no memory is being used), and then use foreach loop to traverse the buffer:

global $wpdb;

$wpdb->query(
    $wpdb->prepare( "SELECT * FROM %1s", $wpdb->prefix . 'posts' )
);

foreach ( $wpdb->result as $row ) {
    var_dump( $row );
}

[seanmorris]

@dawidurbanski I did notice that but it seems that the $result property is protected, and not actually part of the public api of the class. Can I expect that to remain constant as the class is updated and refactored in the future?

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wpdb.php#L154

[dawidurbanski]

@since 0.71

Looks like it's been untouched since WordPress 0.71. I would bet all my money on it being stable :D

[dawidurbanski]

Can I expect that to remain constant as the class is updated and refactored in the future?

If that's about to happen, half of the internet is going down :)

[seanmorris]

One thing I'd be concerned about is if the underlying datatype were to change, but I did see the old file where the line originated, and it HAS been around a while, so I'll go with it.

[seanmorris]

playground-tools/packages/collector/Collector_Db.php

Lines 39 to 59 in c8e6a99

	foreach($wpdb->result as $record)
	{
	if($table === 'wp_users' && (int) $record['ID'] === (int) wp_get_current_user()->ID)
	{
	$record['user_pass'] = wp_hash_password(collector_use_fakepass());
	}
	$insert = sprintf(
	'INSERT INTO `%s` (%s) VALUES (%s);',
	esc_sql($table),
	implode(', ', array_map(fn($f) => "`" . esc_sql($f) . "`", array_keys($record))),
	implode(', ', array_map(fn($f) => "'" . esc_sql($f) . "'", array_values($record))),
	);
	file_put_contents($sqlFile, $insert . "\n", FILE_APPEND);
	if(--$remaining <= 0)
	{
	break;
	}
	}

[adamziel]

What's the difference between esc_sql and $wpdb->prepare()? Would it make sense to stick to one of these?

[bgrgicak]

$wpdb->prepare() will add quotes around table name, and it's a practice in WP to not add table names with $wpdb->prepare().

[adamziel]

Aha, gotcha! Is sprintf() typically used in WordPress core in cases like that? If yes, we're good. If not, is it possible do here what core does?

[bgrgicak]

You would usuall see "SELECT * FROM $wpdb->posts" but this would require us to save the escaped value in a variable, or contact strings 'DROP TABLE IF EXISTS ' . esc_sql($table) . ';' I think that sprintf is a ok solution here.

[adamziel]

The doc page for esc_sql says this:

Be careful in using this function correctly. It will only escape values to be used in strings in the query. That is, it only provides escaping for values that will be within quotes in the SQL (as in field = '{$escaped_value}'). If your value is not going to be within quotes, your code will still be vulnerable to SQL injection. For example, this is vulnerable, because the escaped value is not surrounded by quotes in the SQL query: ORDER BY {$escaped_value}. As such, this function does not escape unquoted numeric values, field names, or SQL keywords.

Here's how it handles unsanitized inputs:

<?php
echo "`" . esc_sql("A backtick ` and a backslash " . chr(92)) . "`";
// `A backtick ` and a backslash \\`

Here's what $wpdb->quote_identifier does:

<?php
echo $wpdb->quote_identifier("A backtick ` and a backslash " . chr(92))
// `A backtick `` and a backslash \`

The latter seems to be correct:

create table `te``st\` (id int);
show tables;

Tables_in_test
--
te`st\

[adamziel]

Well, in the (highly unlikely) case I listed above, "SELECT * FROM $table" would not work, as it would become...

SELECT * FROM te`st\

However, sprintf("SELECT * FROM %s", $wpdb->quote_identifier($table)) would work as it would become...

SELECT * FROM `te``st\`

which is valid MySQL SQL.

[bgrgicak]

I see now. What about esc_sql?
It seems like it's not adding any security so we can remove it.

[bgrgicak]

Sorry about all the questions I would just like to ensure we are going in the same direction.

[adamziel]

Agreed about removing esc_sql. sprintf("SELECT * FROM %s", esc_sql($table)) would return invalid SQL:

SELECT * FROM `te`st\\`

$wpdb->quote_identifier looks like a good replacement for esc_sql

[bgrgicak]

I updated this in c02934d.
Thank you for your help on this. It took a while, but I think that we landed on a solid implementation.

[adamziel]

Let's document what that regex is doing, I can't tell without stopping here for a few minutes

[bgrgicak]

Done in e0cb7c5

[adamziel]

Aha, thank you! This will exclude the .htaccess files, the .ht.sqlite database file, and other potentially desirable files. I'm not sure what's the right thing to do here, but we could start by allowing those two.

[bgrgicak]

Are these files used by Playground in any way?
If not we may not actually need them now.

[adamziel]

.htaccess isn't and a @TODO note would be good enough for now. The .ht.sqlite file would normally be used if available, but in this case we're moving the data as SQL dump so I guess that isn't used either. A comment or an issue should do the trick.

[bgrgicak]

Done in c02934d

[bgrgicak]

@adamziel I moved the code to a PR on this repo.
Let's move the conversation there. All your current comments in this PR should have already been addressed.