Bug Fix to Preserve Encoded HTML Characters in Post Body Between Edits

November 2nd, 2019

A bug was introduced in the previous mp-wp patch that caused encoded ("escaped") html characters to be converted back to their symbols after being loaded into the post editor and re-saved.

#ossasepia 2019-10-31 18:33 whaack: lol til after hitting 'save draft' on a mpwp post with html characters escaped, the escaped characters are evaluated to their html characters, thus hitting 'save draft' twice in succession can alter the content of a post.

It was a bug I had run into myself, and apparently others as well. Diana Coman encouraged further investigation and helped to isolate the problem.

#ossasepia 2019-11-01 20:26 billymg: diana_coman: steps to reproduce: 1) create a new post, include some encoded html character, e.g. &lt; 2) save post, 3) reload post edit page, 4) the &lt; you previously saved will render as a < in the editor text area, so a subsequent save inserts the < into the db, not the &lt; you had before
#ossasepia 2019-11-01 20:26 diana_coman: billymg: did that and...nope?

[...]

#ossasepia 2019-11-01 20:32 diana_coman: billymg: I start suspecting it's something you broke with the latest /trimming patches really; iirc on ossasepia I have the original mpwp, ie not pressed to last vpatch

And indeed it was!

#trilema 2019-11-01 20:49 billymg: yeah, when originally ripping it out, my brain parsed a "if (! $richedit ) ..." as "if ( $richedit ) ..." and so removed the line

So let's see exactly the lines of the patch that caused this bug and also what may have caused my original confusion.


diff -uNr a/mp-wp/wp-includes/formatting.php b/mp-wp/wp-includes/formatting.php
--- a/mp-wp/wp-includes/formatting.php 10526d3d84bbe766511af25287f01bbe1125a06d18babb93e1b7c33488c1f1df15ea3e706848a67d36c30edac25d3a4dd360bc563ac21093f6bfe0183f9c4388
+++ b/mp-wp/wp-includes/formatting.php 96c9b9b0b8da6f255e10fd47fd4053958417127524a5b8be961e126cce7fde0476bd5c2b20c4580517a75b01e9751636c8dc0c6f250ab8f94dbbe8097057cf67
@@ -775,20 +775,16 @@
 /**
  * Acts on text which is about to be edited.
  *
- * Unless $richedit is set, it is simply a holder for the 'format_to_edit'
- * filter. If $richedit is set true htmlspecialchars() will be run on the
- * content, converting special characters to HTMl entities.
+ * Holder for the 'format_to_edit'
+ * filter.
  *
  * @since 0.71
  *
  * @param string $content The text about to be edited.
- * @param bool $richedit Whether or not the $content should pass through htmlspecialchars(). Default false.
  * @return string The text after the filter (and possibly htmlspecialchars()) has been run.
  */
-function format_to_edit($content, $richedit = false) {
+function format_to_edit($content) {
 	$content = apply_filters('format_to_edit', $content);
-	if (! $richedit )
-		$content = htmlspecialchars($content);
 	return $content;
 }

Notice the contradiction in the comment:


If $richedit is set true htmlspecialchars() will be run on the content, converting special characters to HTMl entities.

And the code it describes:


if (! $richedit )
	$content = htmlspecialchars($content);

Which does precisely the opposite of what the comment describes. Now, the introduction of the bug was still 100% my fault, and I honestly can't say if I even bothered reading the comment at the time. Perhaps it wasn't the comment that misled me but the "!" wedged in there, which, after reading so many lines after however many days, simply slipped past me. Either way, having a test in place would've caught the bug before it went into production and so along with a patch to fix the bug I also added a test to assert the correct behavior.

MP-WP Patch and Signature

mp-wp_apply-htmlspecialchars-to-post-edit-content.vpatch
mp-wp_apply-htmlspecialchars-to-post-edit-content.vpatch.billymg.sig

MP-WP-Tests Patch and Signature

mp-wp-tests_add-test-for-encoded-html-characters-in-edit-ui.vpatch
mp-wp-tests_add-test-for-encoded-html-characters-in-edit-ui.vpatch.billymg.sig

Additional Tests for MP-WP, Now According to Spec

September 2nd, 2019

After the first round of tests and some discussion in the logs, there is now a spec in place for a minimum viable blogging platform. The tests included in this patch evaluate against a portion of the requirements in the spec, with remaining requirements to be covered in future patches.

Together with the first round of tests, mp-wp-tests now provides coverage for most1 of the Write section of the spec.

The full tree can now be found here: MP-WP-Tests VTree.

Patch and Signature

mp-wp-tests_add-media-and-additional-post-coverage.vpatch
mp-wp-tests_add-media-and-additional-post-coverage.vpatch.billymg.sig

  1. The tests to do not yet cover: multi-file upload, programmable tag presets, reviewable/diffable list of drafts, and time-deferred publishing []

MP-WP Automated Testing Proposal and VPatch

August 9th, 2019

The following is presented for the consideration of the lords and ladies of tmsr.

Automated End-To-End Testing

Since mp-wp is a web application consisting of a frontend and a backend, my proposal is to handle testing via end-to-end tests run in the browser itself. This will unfortunately require the operator to run the tests on a toilet box as the browser automation tool, Selenium WebDriver runs on Java and will not work with earlier vintages of Firefox (I initially tested with version 54 of Firefox and found myself having to install version 60 to get anything working). However, I believe this to be an acceptable temporary trade-off for the following reasons:

  • MP-WP is a gnarly mix of PHP, JavaScript, HTML, and CSS code. Unit tests can assert correctness of individual PHP and Javascript functions but one careless snip in a CSS file could result in a broken UI that no unit test would catch. End-to-end tests will not only insure against breakage from HTML/CSS changes, but will also allow for test coverage of critical code paths without first fully unwinding the codebase and fitting it into one's head.
  • These e2e tests are intended to be temporary, and only remain while mp-wp sweats away the pounds. They can be abandoned completely once the codebase is in its desired minimal form.
  • Only the tests need to be run on a polluted box, the requirements for running mp-wp are not affected.

I've put together a genesis patch that contains a small POC spec for the basic posting functionality in mp-wp. The spec is written as:


const { LoginPage, DashboardPage, PostsPage, EditPostPage } = require('../pages');

describe('Posts', () => {
  beforeAll(() => {
    const loginPage = new LoginPage();
    loginPage.navigate();
    loginPage.logIn();
  });

  afterAll(() => {
    const dashboardPage = new DashboardPage();
    dashboardPage.navigate();
    dashboardPage.logOut();
  });

  it('Can navigate to and display the Posts page', () => {
    const dashboardPage = new DashboardPage();
    dashboardPage.followNavLinkTo('Posts', 'Edit Posts');

    const postsPage = new PostsPage();

    expect(postsPage.pageTitle.isExisting()).toBe(true);
    expect(postsPage.postsTable.isExisting()).toBe(true);
  });

  it('Can create and publish a post', () => {
    const dashboardPage = new DashboardPage();
    dashboardPage.followNavSubMenuLinkTo('Posts', 'Add New', 'Add New Post');

    const postTitle = 'Bitcoin Declaration Of Sovereignty';
    const postBody = '<p>When in the course of human events, it becomes necessary for one person to dissolve the political bands which have connected them with the human herd, and to assume among the powers of the world the separate and equal station to which the laws of nature entitle them, a modicum of respect to their own intelligence requires that they should declare the causes which impel them to the separation.</p>';

    const editPostPage = new EditPostPage();
    editPostPage.editAndPublishPost({ postTitle, postBody });

    expect(editPostPage.page.$('#message*=Post published').isExisting()).toBe(true);

    dashboardPage.navigateToBlog();

    expect($(`a=${postTitle}`).isExisting()).toBe(true);
    expect($('.entry').getHTML(false).trim()).toEqual(postBody);
  });
});

Run the tests from the root of the project with:


./node_modules/\@wdio/cli/bin/wdio.js

This will execute the runner which will look for test files in tests/specs/ and run any it finds. This will look something like this, along with output in your console on the status of the test.

After the initial setup, writing the test didn't take very long. If this seems like a worthwhile enough approach I'll follow up with another post outlining the full set of tests to be written. After receiving input/feedback on the list, I'll proceed to write and publish the remaining tests as additional patches on this tree.

Patch and Signature

mp-wp-tests_genesis.vpatch
mp-wp-tests_genesis.vpatch.billymg.sig

Note: After pressing you will need to run npm install from the project directory in order to pull down just under 100mb of depshits in order for this to work. Comments welcome here or in #trilema.