Fix various PHPCS issues

[paulschreiber]

No description provided.

[westonruter]

@paulschreiber if you re-base you recent pull requests against develop the build errors related to PHP 5.3 should go away.

Also, feel free to combine your commits into a single PR that fixes multiple phpcs issues in one go. No need for separate PRs for each commit.

[paulschreiber]

• I created separate PRs to make reviewing the diffs easier. I also wasn't sure which ones you would accept.
• This was branched off develop. What sort of rebasing is needed?

[westonruter]

I created separate PRs to make reviewing the diffs easier. I also wasn't sure which ones you would accept.

Normally that's good, but these PRs are almost all PHPCS fixes which can be easily reviewed together. Having a separate PR for each just creates too much noise.

This was branched off develop. What sort of rebasing is needed?

A fix just landed in develop which fixes the PHP 5.3 issue: a04782a

So just git pull --rebase https://github.com/Automattic/amp-wp.git develop; git push -f and that should fix the build.

[westonruter]

Please then feel free to git cherry-pick each commit from your other PHPCS-related pull requests to amend to this PR, and then close the other PRs.

[paulschreiber]

I've rebased and cherry-picked the commits (except for #1000, as it's not clear what do there).

[paulschreiber]

@westonruter What's causing Travis CI to fail here?

[westonruter]

PHPCS errors: https://travis-ci.org/Automattic/amp-wp/jobs/351608818#L315

[paulschreiber]

This commit (be6a78a) doesn't touch class-amp-base-embed-handler.php, which generates the error.

[westonruter]

@paulschreiber there are multiple errors: https://travis-ci.org/Automattic/amp-wp/jobs/351608818#L315-L337

PHPCS via wp-dev-lib is looking at changes made to the entire diff in the PR, not individual commits in the PR. Because you added public to a method that lacks the necessary phpdoc, now that PHPCS error is being surfaced because it is related to a line you touched.

[paulschreiber]

Odd. We saw a green checkmark after the previous commit (df77083). Do you want me to revert 78e64f4 or add docs?

[westonruter]

Adding the doc comments would be great.

[paulschreiber]

I've added many doc comments. PHPCS is still finding a few errors:

class-amp-base-embed-handler.php:59:32: error - Object property "DEFAULT_WIDTH" is not in valid snake_case format
class-amp-youtube-embed.php:161:13: warning - This comment is 50% valid code; is this commented out code?
class-amp-youtube-embed.php:170:13: warning - This comment is 47% valid code; is this commented out code?
test-amp-image-dimension-extractor.php:66:1: warning - Best practice suggestion: Declare only one class in a file.
test-amp-image-dimension-extractor.php:130:1: warning - Best practice suggestion: Declare only one class in a file.

Should we leave those for now?

[westonruter]

This actually isn't necessary. The PHPCS ruleset does not include the AdminBarRemoval WordPress.com VIP sniff. You should make sure to use the project's own ruleset when developing.

[westonruter]

Aside: things like this are more elegant as compact( 'url', 'video_id' )

[paulschreiber]

Agreed that compact is better. phpcbf reformatted it as above (expanded).