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.

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

#804 Update Allowed Tags And Attributes #823

Merged
merged 27 commits into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@kienstra
Copy link
Collaborator

commented Dec 7, 2017

This PR fixes and improves the AMP HTML tag and attributes updater, refer to the ACs for more info...


Steps To Manually Run Script

In order for amp_wp_build.py to generate class-amp-allowed-tags-generated.php, run these commands, using a Linux environment like VVV:

  1. cd /path/to/your/vvv/wp-content/plugins/amp-wp
  2. bash bin/amphtml-update.sh

Note: these steps were edited, as most of these steps are now in the file bin/amphtml-update.sh. That file also includes the step:
7. git apply this commit to re-add validator_gen_md.py, which this commit deleted

See #689 (Only the requirement "Minor: The script should do the git clone...")
Fixes #761
Fixes #772
Fixes #804
Closes #749
Closes #805

kienstra added some commits Dec 5, 2017

804 : Update allowed AMP tags in sanitizer file.
Generate class-amp-allowed-tags-generated.php,
Using amp_wp_build.py.
Followed the instructions at the top of the file.
This is spec file revision 527.
804 : Prevent errors on amp_wp_build.py, update extensions dir.
Before, there was an error in:
tag_spec.also_requires_tag
This didn't always have the property.
Also, update the directory of the extensions.
The .protoascii file is no longer in the directory '0.1.'
804 : Modify PHPUnit tests for tag and attribute validation.
Since the allowed tags and attributes are updated,
Several assertions failed.
Update them to reflect the new AMP spec.
Since there is no longer a protocol whitelist,
Remove the assertions for protocols.
804 : Remove the requirement that 'data-multi-size' be empty.
This file was generated from a build script,
In the repo https://github.com/ampproject/amphtml
The spec file seems to require an empty value:
extensions/amp-ad/validator-amp-ad.protoascii
But examples of <amp-ad> seem to have the value.
Like <amp-ad data-multi-size=320x50.
@see examples/a4a-multisize.amp.html.
So remove the line in the PHP file that requires it to be empty.
804 : PHPUnit tests for newly-allowed tags.
Having generated a new file for the allowed tags and attributes,
Add these to the data for test_sanitizer().
Also, add unit test for is_missing_mandatory_attribute().
And rename that function from:
is_mandatory_attribute_missing().
804 : Move logic for missing attributes.
Before, this was inside an 'else' block.
And it should be outside of this,
So it applies more broadly.
If there is a list of attributes, and one is missing,
Remove the node.
This was the previous behavior.
804 : Remove the unused $spec_value variable.
This was assigned a value that's used later.
There's no need to store it in a variable.
804 : Improve variable alignment and add PHPDoc tags.
There was some missing documentation.
Add this for test_sanitizer and validate_tag_spec_for_node.
804 : Exclude the generated allowed tags file from PHPCS checks.
This has hundreds of errors and warnings.
It's generated from a Python script.
804 : Align 'array' keywords vertically.
In response to the warnings in Travis.
And align an equal sign in test_sanitizer().
804 : Change the conditional to simply compare to true.
Before, there was a failed Travis build.
This used boolval(), which caused a fatal error.
Instead, simply compare the value to true.
804 : Array and equal sign alignment fixes.
In response to a failed Travis build.
Fix an error in an array not being aligned.
An move an = sign 2 spaces to the left.
804 : Prevent accessing an object as an array.
There was an error in the Travis build.
A function accessed a DOMElement property as an array.
Instead, use the setAttribute() method.
$attr_spec_list = array_merge( $attr_spec_list, $rule_spec_list_to_validate[ $id ][AMP_Rule_Spec::ATTR_SPEC_LIST] );
foreach ( $spec_ids_sorted as $id ) {
$spec_list = isset( $rule_spec_list_to_validate[ $id ][ AMP_Rule_Spec::ATTR_SPEC_LIST ] ) ? $rule_spec_list_to_validate[ $id ][ AMP_Rule_Spec::ATTR_SPEC_LIST ] : null;
if ( ! $this->is_missing_mandatory_attribute( $spec_list, $node ) ) {

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 7, 2017

Author Collaborator

This addresses an edge case where there are more than one spec lists, and one of them requires a value. If one of them doesn't require a value, use that instead.

This detects that with $this->is_missing_mandatory_attribute(). If the node satisfies one of the specs, this doesn't remove it.

if ( $is_mandatory && ! $attribute_exists ) {
$this->remove_node( $node );
return;
}

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 7, 2017

Author Collaborator

This is mainly decomposed into the function below, is_missing_mandatory_attribute().

@@ -430,8 +449,7 @@ private function _sanitize_disallowed_attribute_values_in_node( $node, $attr_spe
foreach ( $attrs_to_remove as $attr_name ) {
if ( isset( $attr_spec_list[$attr_name][AMP_Rule_Spec::ALLOW_EMPTY] ) &&
( true == $attr_spec_list[$attr_name][AMP_Rule_Spec::ALLOW_EMPTY] ) ) {
$attr = $node->attributes;
$attr[ $attr_name ]->value = '';
$node->setAttribute( $attr_name, '' );

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 7, 2017

Author Collaborator

This raised an error, so it uses the setAttribute() method instead.

@kienstra kienstra requested a review from ThierryA Dec 7, 2017

@@ -19,4 +19,5 @@
<exclude-pattern>*/dev-lib/*</exclude-pattern>
<exclude-pattern>*/node_modules/*</exclude-pattern>
<exclude-pattern>*/vendor/*</exclude-pattern>
<exclude-pattern>*/includes/sanitizers/class-amp-allowed-tags-generated.php</exclude-pattern>

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 7, 2017

Author Collaborator

This file is generated by the Python file, and created hundreds of warnings and errors in PHPCS.

@@ -246,136 +246,6 @@ public function get_attr_spec_rule_data() {
),
'expected' => AMP_Rule_Spec::NOT_APPLICABLE,
),

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 7, 2017

Author Collaborator

The allowed protocol values are removed from the spec file, so this removes the tests for them.

@kienstra kienstra requested a review from westonruter Dec 7, 2017

@westonruter

This comment has been minimized.

Copy link
Member

commented Dec 8, 2017

Double-arrow PHPCS errors are suppressed for now via 2b1443e.

@westonruter

This comment has been minimized.

Copy link
Member

commented Dec 8, 2017

@kienstra I think the next thing here is to look over gitHub issues that seem to be related to the spec being out of date, and confirm that the issues are now resolved with your latest changes.

Thanks, @westonruter. I'll look at the other issues surrounding the specification needing to be updated, and confirm if this pull request resolves them. -Ryan

ThierryA added some commits Dec 8, 2017

@ThierryA
Copy link
Collaborator

left a comment

Great job on the PR @kienstra. Below is a bash script draft which is still in WIP.

@@ -0,0 +1,40 @@
#!/bin/bash

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 8, 2017

Collaborator

Here is a draft the bash script to update AMP HTML. This script is meant to run in Linux environment (ex. VVV). To run the script, simply cd to the plugin root and run bash bin/amphtml-update.sh.

# Go to the right location.
if [[ '.' != $(dirname "$0") ]]; then
cd bin
fi

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

What I usually do instead of this conditional is:

cd "$(dirname "$0")"
sudo apt-get install git
sudo apt-get install python
sudo apt-get install protobuf-compiler
sudo apt-get install python-protobuf

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

You can combine all of the dependencies into one call:

sudo apt-get install git python protobuf-compiler python-protobuf
# Clone amphtml repo.
if [[ ! -e $VENDOR_PATH/amphtml ]]; then
git clone https://github.com/ampproject/amphtml amphtml
fi

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Maybe add an else here which goes into $VENDOR_PATH/amphtml and does git pull?


# Install dependencies.
sudo apt-get install git
sudo apt-get install python

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Maybe we should make git and python just dependencies that we assume are installed, and exit if the command doesn't exist?

if ! command -v git >/dev/null 2>&1; then
    echo "Git is required"
    exit 1
fi
if ! command -v python >/dev/null 2>&1; then
    echo "Python is required"
    exit 1
fi

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 8, 2017

Collaborator

If you are ok with it, I think it is fine to automatically install git or python if not installed, the check is pretty quick if it is already installed.

PROJECT_PATH=$(dirname $PWD)
VENDOR_PATH=$PROJECT_PATH/vendor

if ! apt-get > /dev/null; then

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Better to check if the command exists via: if ! command -v apt-get >/dev/null 2>&1; then


if ! apt-get > /dev/null; then
echo -e "The AMP HTML uses the apt-get, make sure to run this script in a Linux environment"
exit

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Do exit 1 so a failure error code is sent. An exit means just exit 0 which means success.

VENDOR_PATH=$PROJECT_PATH/vendor

if ! apt-get > /dev/null; then
echo -e "The AMP HTML uses the apt-get, make sure to run this script in a Linux environment"

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

No need for -e

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Dec 8, 2017

@kienstra now that the bash script to update the HTML AMP tags is completed, you may update the PR description with the steps to run the script:

  1. cd to the AMP plugin root
  2. run bash bin/amphtml-update.sh

Note that the amphtml-update.sh bash script is meant to run in a Linux environment (ex. VVV).

Checklist

  • update the PR description with the steps above
  • add a contributing.md file in the root with the instruction to update the HTML AMP tags and attributes as described above
@westonruter

This comment has been minimized.

Copy link
Member

commented Dec 9, 2017

@kienstra This PR will close PR #749 as obsolete, correct?

Thanks, @westonruter. You're right, PR #749 is obsolete now. -Ryan

804 : Add a contributing.md file, with steps to use build script.
This describes how to use the new script amphtml-update.sh.
This is mainly modified from another open-source file.
It could probably use more information.
Including the workflow for submitting pull requests.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2017

Created contributing.md, Updated Script Steps,

Hi @westonruter and @ThierryA,
Thanks a lot for your work on this. The commit above adds a contributing.md file, with instructions to use your new script amphtml-update.sh. At some point, that file should probably have more details about contributing.

Still, I need to look at the remaining issues for AMP tag support, and see if this PR addresses them.

@kienstra kienstra changed the title #804 : Update Allowed Tags And Attributes Issue 804 : Update Allowed Tags And Attributes Dec 9, 2017

kienstra and others added some commits Dec 9, 2017

804 : Test that 'amp-playbuzz' is sanitized properly.
As harrisdavid mentioned, before this PR, the 'src' was mandatory.
But with the updated AMP spec, it's not.
804 : Add allowed protocols to sanitization file.
Before, there was an issue in the file that created:
class-amp-allowed-tags-generated.php.
amphtml-update.sh had a conditional:
if isinstance(value, list)
But the allowed_protocol attribute has a type of:
google.protobuf.internal.containers.RepeatedScalarFieldContainer
So that conditional failed.
And allowed_protocol wasn't added.
So instead, make that an 'else' statement.
That change only resulted in 'allowed_tags' being added.
@@ -0,0 +1,30 @@
# AMP Contributing Guide

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 11, 2017

Collaborator

Perhaps add a welcoming sentence (ex. "Thanks for taking the time to contribute!")

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 11, 2017

Author Collaborator

Thanks, @ThierryA. This commit adds your sentence to contributing.md:

Thanks for taking the time to contribute!
2. run `bash bin/amphtml-update.sh`
That script is intended for a Linux environment like [VVV](https://github.com/Varying-Vagrant-Vagrants/VVV).

### PHPUnit Testing

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 11, 2017

Collaborator

The way the dev lib runs PHPUnit (run_phpunit_local) needs to run in an environment which has WordPress Unit Test installed, for example VVV. I would advise to add that as a note.

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 11, 2017

Member

Yes, this came up in #828,.along with questions about how to get pre-commit hook working from wp-dev-lib.

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 11, 2017

Author Collaborator

Thanks, @ThierryA. This commit adds your point to contributing.md:

Please run these tests in an environment with WordPress unit tests installed, like [VVV](https://github.com/Varying-Vagrant-Vagrants/VVV).

@ThierryA ThierryA changed the title Issue 804 : Update Allowed Tags And Attributes #804 : Update Allowed Tags And Attributes Dec 11, 2017

@ThierryA ThierryA changed the title #804 : Update Allowed Tags And Attributes #804 Update Allowed Tags And Attributes Dec 11, 2017

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

@kienstra as you go through the PRs and issues that this PR address, could you add the Fixes #{number} and/or Closes #{number} to the description (I have already added a few). Thanks,

@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

Sure, @ThierryA. I'll add Fixes #{number} and or Closes #{number} to related tickets, as they apply.

kienstra added some commits Dec 11, 2017

804 : Add to contributing.md, including unit test information.
Add a point that this requires WP unit tests.
Also, begin with the welcome that Thierry suggested.
Props @ThierryA.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

Added Issues That Are Fixed

Hi @ThierryA and @westonruter,
Having looked at all of the issues with the labels validation-errors and sanitizers, I added these issues to the PR description as fixed by this PR:

#689 (Only the requirement "Minor: The script should do the git clone...")
#761
#772

The following issues also look to have been resolved, though not with this PR. I made comments to that effect on the tickets, requesting that the openers verify them:
#237
#253
#781

@westonruter westonruter added this to the v0.6 milestone Dec 11, 2017

804 : Merge in develop, resolve conflicts.
In phpcs.xml, retain edits from both branches.
There were several conflicts in:
class-amp-tag-and-attribute-sanitizer.php
These were mainly conflicts with 03c12.
Some were only DocBlocks, but some were logic.
I mainly resovled them in favor of this branch:
feature/804-allowed-tags.

@mjangda mjangda referenced this pull request Dec 12, 2017

Closed

Whitelist Sanitizer Improvements #689

4 of 6 tasks complete

kienstra added some commits Dec 12, 2017

804 : Correct PHPUnit test for <script> validation.
When this has a type of 'application/json,'
There is a required ancestor.
So create an ancestor of <amp-analytics>.
804 : Add spaces to correct PHPCS errors.
In response to a failed Travis build.
Also, change == to ===.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Resolved Merge Conflicts

Hi @westonruter,
Thanks for waiting. This commit merges in develop, and resolves the merge conflicts. The commits that follow fix a PHPUnit test and PHPCS issues.

@westonruter
Copy link
Member

left a comment

Impressive work here! You wrestled with dragons. Let's get these changes in wider testing.

@westonruter westonruter merged commit 7f79a50 into develop Dec 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the feature/804-allowed-tags branch Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.