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.

Sign up for GitHub

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

Jump to bottom

Abstracting AMP validators and sanitizers into AMP Library #4

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

Abstracting AMP validators and sanitizers into AMP Library #4

wants to merge 18 commits into from

Conversation

marcelovani
Copy link
Owner

@marcelovani marcelovani commented Dec 5, 2018 •
edited
Loading

On this pull request I did the following:

  • Removed the submodule https://github.com/xwp/wp-dev-lib.git and requested it as composer package, which will be checked out inside the vendor folder
  • Added a require for autoloader.php in the vendor folder
  • Moved the bash script and python script to re-generate the validator classes into the AMP-Library, see this pull request https://github.com/dennisinteractive/amp-library/pull/6/files
  • Fixed the bash script that installs WP to delete the table before creating it
  • Removed the generated class-amp-allowed-tags-generated.php, this will now live in the AMP-Library
  • I have done a quick hack to replace the AMP_Content class with \Lullabot\AMP\AMP(). This needs to be done properly buy my WordPress skills are limited.
private function build_post_content() {
    $amp = new \Lullabot\AMP\AMP();
     $content = apply_filters('the_content', $this->post->post_content);
     $options = array(
      'scope' => 'body',
      'add_stats_html_comment' => false
    );
    $amp->loadHtml($content, $options);
    $this->add_data_by_key('post_amp_content', $amp->convertToAmpHtml());
    $this->merge_data_for_key('amp_component_scripts', $amp->getComponentJs());
    $this->add_data_by_key('post_amp_debug', $amp->warningsHumanHtml());
    return;

marcelovani
marcelovani commented Dec 5, 2018
View reviewed changes
Gruntfile.js
@@ -34,7 +34,7 @@ module.exports = function( grunt ) {
stderr: true
},
readme: {
command: './dev-lib/generate-markdown-readme' // Generate the readme.md.
command: './vendor/xwp/wp-dev-lib/generate-markdown-readme' // Generate the readme.md.
},
Copy link
Owner Author

Choose a reason for hiding this comment

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

The submodule is now pulled via composer and goes to the vendor folder

marcelovani
marcelovani commented Dec 5, 2018
View reviewed changes
amp.php Outdated
@@ -96,6 +96,7 @@ function _amp_incorrect_plugin_slug_admin_notice() {
add_action( 'admin_notices', '_amp_incorrect_plugin_slug_admin_notice' );
}

require_once AMP__DIR__ . '/vendor/autoload.php';
Copy link
Owner Author

Choose a reason for hiding this comment

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

Include autoloader

marcelovani
marcelovani commented Dec 5, 2018
View reviewed changes
bin/install-wp-tests.sh
@@ -104,7 +104,8 @@ install_db() {
fi

# create database
mysqladmin create $DB_NAME --user="$DB_USER" --password="$DB_PASS"$EXTRA
mysql --user="$DB_USER" --password="$DB_PASS"$EXTRA -e"DROP DATABASE IF EXISTS $DB_NAME"
mysql --user="$DB_USER" --password="$DB_PASS"$EXTRA -e"create DATABASE $DB_NAME"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Deletes the database if it already exists then re-creates it

marcelovani
marcelovani commented Dec 5, 2018
View reviewed changes
composer.json
},
{
"type": "vcs",
"url": "[email protected]:dennisinteractive/amp-library.git"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Using our fork of amp-library, see pr dennisinteractive/amp-library#6

marcelovani
marcelovani commented Dec 6, 2018
View reviewed changes
composer.json Outdated
"sabberworm/php-css-parser": "dev-master"
"sabberworm/php-css-parser": "dev-master as 8.0.0",
"xwp/wp-dev-lib": "dev-master",
"lullabot/amp": "dev-amphtml-generator"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Xp-dev-lib is now being cloned with composer
Use fork of lullabot library

marcelovani
marcelovani commented Dec 6, 2018
View reviewed changes
includes/class-amp-autoloader.php
@@ -71,7 +71,7 @@ class AMP_Autoloader {
'AMP_Analytics_Options_Submenu_Page' => 'includes/options/views/class-amp-analytics-options-submenu-page',
'AMP_Options_Menu_Page' => 'includes/options/views/class-amp-options-menu-page',
'AMP_Rule_Spec' => 'includes/sanitizers/class-amp-rule-spec',
'AMP_Allowed_Tags_Generated' => 'includes/sanitizers/class-amp-allowed-tags-generated',
// 'AMP_Allowed_Tags_Generated' => 'includes/sanitizers/class-amp-allowed-tags-generated',
Copy link
Owner Author

Choose a reason for hiding this comment

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

This file is being moved to the amp-library https://github.com/dennisinteractive/amp-library/tree/amphtml-generator/src/Spec

@marcelovani marcelovani changed the title Amp library upgrade Abstract AMP validators and sanitizers into AMP Library Dec 6, 2018
@marcelovani marcelovani changed the title Abstract AMP validators and sanitizers into AMP Library Abstracting AMP validators and sanitizers into AMP Library Dec 6, 2018
@marcelovani marcelovani mentioned this pull request Dec 7, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@marcelovani