Web-stories-wp: Storing Story content in the DB (AMP HTML)

[miina]

Feature description

The AMP HTML for rendering in the FE should be kept in post_content based on the JSON saved to post_meta (see ampproject/amp-wp#3794)

This would be updated only when JSON is updated.

---

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

Acceptance criteria

• AMP Story HTML is saved to post_content based on JSON from editor.
• The HTML renders in the FE as a valid AMP Story.

Implementation brief

Every time when post_meta is updated in the DB to save the data from the editor then we should also update AMP HTML in the post_content.

For portability reasons, putting together the AMP HTML will be done on the client-side within the React app. The content will be saved to the DB as-is and it will leverage the AMP Plugin's sanitization logic for always rendering valid AMP. (To clarify: this effectively means that we could potentially save invalid AMP to the post_content even though the rendered output will always be valid AMP. The validation is purposefully not making any changes to the DB directly, different behavior could be discussed within a separate Validation issue)

The following will be considered:

• The amp-story tag will be part of the post_content to make it easier to port, everything outside of the amp-story tag will be part of the WordPress template and not saved to post_content.
• Each Page (amp-story-page) will have one amp-story-grid-layer child element with vertical layout which will include all the common elements with absolute positioning, including shapes, images, Text, etc.
• Additionally, amp-story-cta-layer and amp-story-page-attachment will be handled separately and always added as the last element after amp-story-grid-layer. It has to be ensured that there is only one CTA layer or one Attachment on each Page. Also, there can be no CTA layer on the first Page. This should already be done before starting putting together the AMP HTML output.
• Depending on the approach chosen within ampproject/amp-wp#3802 for full-bleed components, then those might potentially have their own amp-story-grid-layer with template fill.

We will need mapping for the attributes from JSON to attributes added to the tags, for example:
x in JSON will be converted to left in style attribute (likely with some kind of logic for converting from pixels, TBD later)
animationType => animate-in, etc.

We will then loop through the Pages, create the content and save the result AMP HTML to post_content via API request.

Additionally, we need to filter the allowed tags to make sure that all the AMP Story tags and attributes are whitelisted for post_content. We can do this via the filter_kses_allowed_html filter. See the old CPT for reference: https://github.com/ampproject/amp-wp/blob/d007c7749b96141f1d076427ed06062d7d38f8d9/includes/class-amp-story-post-type.php#L684L722

• One of the features of the AMP Plugin is ensuring the rendered output from post_content is valid. We should make sure to take advantage of that existing feature. See also the legacy post type setup for that. For example this and also the related parts in CPT file.

Things to potentially follow up with after this PR

• Content validation logic (block validation in Gutenberg) to ensure that the logic for saving is not broken
• Deprecation and not breaking the previously existing stories (related to AMP HTML but more to JSON) ( ampproject/amp-wp#3806 )
• FE stylesheet
• Revisions ( ampproject/amp-wp#3786 )
• Previewing without saving
• RSS feed and other meta that might be necessary.
• Removing the_content filter ( ampproject/amp-wp#3788 )
• Validation: what to do with invalid AMP, e.g. due to third party plugins, filters, bugs in the tool, embeds?

QA testing instructions

Demo

• 
  Editor:
  

Frontend:

Changelog entry

[miina]

Splitting this part of storing in DB out of ampproject/amp-wp#3794 and moving it to IB column. cc @pbakaus

[miina]

Note: looking into this.

[miina]

Things to consider:

• AMP Validation (ensure that the HTML goes through AMP Validation process and only valid AMP is displayed in the FE)
• FE styling (add stylesheet for FE only)
• RSS Feed?
• Filter allowed tags -- ensure that AMP HTML tags and attributes are saved into post_content.
• Previewing -- needs perhaps UX / Product confirmation first.

[spacedmonkey]

We are going to have to do some work to make sure that AMP html tags save in post_content. Some of this work already, this work will need to extended to accept all the attributes and tags in AMP spec.

[miina]

Yep, a lot of it is already in the legacy CPT. Was (or will be) removed within ampproject/amp-wp#3839 but will add the relevant parts back.

[miina]

Just updated https://github.com/ampproject/amp-wp/issues/3838#issuecomment-559534738 as well to clarify the "things to consider" in more detail.

[pbakaus]

Things to consider:

• AMP Validation (ensure that the HTML goes through AMP Validation process and only valid AMP is displayed in the FE)
• FE styling (add stylesheet for FE only)
• RSS Feed?
• Filter allowed tags -- ensure that AMP HTML tags and attributes are saved into post_content.
• Previewing -- needs perhaps UX / Product confirmation first.

I hope we can mostly skip validation, as we wouldn't allow arbitrary HTML content. That might of course change once we allow some sort of Gutenberg block connector, but for now we're in full control over our elements (and we don't allow users to paste arbitrary HTML).

[miina]

I hope we can mostly skip validation, as we wouldn't allow arbitrary HTML content.

Makes sense that ideally, we should never save invalid AMP anyway.

Q: Would amp-story-page-attachment allow embedding? Or would any other embeds be allowed?

Validating the HTML of the post_content before rendering is something that the plugin already does so perhaps it doesn't hurt to go through it? It helps catching bugs as well: misspelling attributes, ensuring the CTA tag is always the last etc. Thoughts?

Or do you mean that we shouldn't display the validation errors in the editor as we used to do before and just do it in the FE silently?

[pbakaus]

If it's something that we currently get "for free", then yes, let's continue doing so! I haven't thought about amp-story-page-attachment too much, it's an interesting question. Paging @jasti for his product knowledge..

[spacedmonkey]

I imagined that we valid when saving post_content and don't allow third parties to filter or modify post_content. There is already a filter on the old board removing all filters from the_content see ampproject/amp-wp#3788

We may also want to do this anyway, as a lot of the filters that run on the content are not useful to use at all, like automatically completing tags, rendering blocks, shortcode and embeds.

[miina]

@pbakaus The IB description has been updated based on what we discussed over the call (creating the AMP HTML in the client-side and saving via API request), should be ready for review. As suggested, pinging @westonruter here, too.

[jasti]

The plan is to allow arbitrary remote URLs (non-AMP) as part of amp-story-page-attachment. @gmajoulet for planned launch date.

[gmajoulet]

Q: Would amp-story-page-attachment allow embedding? Or would any other embeds be allowed?

By embedding, do you mean loading an external web page (like Instagram does), or writing some HTML that embeds components like amp-youtube or amp-twitter?
The latter already works today! The former requires some work on the runtime end, with no clear launch date planned for now :(

[pbakaus]

@jasti @gmajoulet so for now, it sounds like it's safe to assume that we need to ship with the ability to inline embed page attachment AMP HTML code when we launch end of Q1 then?

[gmajoulet]

Correct, this is already launched and working

[dvoytenko]

(Moving this over from the other thread).

To clarify for posterity, the idea we discussed is to have the editor be able to produce serialized JSON (post_meta) and AMP doc (post_content) at the saving time. The php server side can accept both forms and store them. Depending on the post's state (draft vs published) we could additionally run validator on the published post. Is this right?

[westonruter]

The AMP plugin performs validation by issuing a loopback request to the frontend for the permalink of the post. It does this with the user's credentials so it can access the frontend content even for posts in a draft state as a preview, even prior to it being published.

[miina]

Here's a very raw PoC PR for the logic which adds validation to Stories + creates markup (without most of the attributes) for an image element, PR based on the try/save-content branch.

As soon as the IB gets confirmed, will continue work on this.

(Any validation logic changes would be good to separate into a different issue.)

[miina]

@pbakaus Let me know if there is anything pending for allowing this issue to move forward, considering that potential validation changes will be addressed in ampproject/web-stories-wp#40.

[spacedmonkey]

One more acceptance criteria seems to be that both the HTML amp and the story data are in sync to stop breakages.

After a discussion with @miina it seems to make sense to tweak the REST API to make sure that content and story_data required. We have two options.

1. Make both fields required to be supplied on every update request.
2. Add a validation error when either field is submitted, if one field is present and the other is not.

---

1. is against the values of REST API and would this API different from all WP REST API. It would stop delta updates in the future, as all fields would have to be supplied.
2. Should be pretty simple to implement with something like this

protected function prepare_item_for_database( $request ) {
        $prepared_story = parent::prepare_item_for_database( $request );
        if ( is_wp_error( $prepared_story ) ) {
            return $prepared_story;
        }

    if ( ! empty( $request['story_data'] ) && empty( $request['content'] ) ||   ! empty( $request['content'] ) && empty( $request['story_data'] )   ) {
    return new WP_Error( 'rest_empty_content', __( 'content and story_data should always be updated together.' ), array( 'status' => 412 ) );
    }

I am personally not a fan of changing APIs like this later down the road, as it has unintended consequences.

[miina]

@spacedmonkey Note that nr 4. is already implemented: https://github.com/ampproject/amp-wp/pull/3876/commits/deba09a40f3db2d12340c6b42a04cac8063077d3

[spacedmonkey]

Regarding AMP validation.

We have a nicely written PHP implementation of validation, why can't we use that via a custom REST API. How about something like this.

• Create a REST API that takes AMP markup as a param, runs it through PHP validation and returns a success message or an array of errors.
• Build the AMP markup in JS
• Submit markup API.
• Show users errors returned from API.

We make this API validation optional, so other backend do not have to implement it. Thoughts?

[miina]

@spacedmonkey There's a separate issue for AMP Validation issues: ampproject/web-stories-wp#40 , let's discuss this there.

This PR is explicitly not addressing AMP Validation changes.

[spacedmonkey]

Okay, then, I approve the IBR 👍

[miina]

@pbakaus Moving this to Approval for you, added screenshots under Demo section in the description.

[pbakaus]

Looks good, thanks @miina!