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

Added autoloader to reduce complexity; fix phpcs issues #828

Merged
merged 49 commits into from Dec 11, 2017

Conversation

Projects
None yet
3 participants
@mikeschinkel
Copy link
Contributor

commented Dec 8, 2017

@westonruter As I started working on a refactoring as per here I found myself struggling with the complexity of all the classes and making sure they were all loaded in the correct order. So I went out on a limb and implemented a simple, fast classmap autoloader and removed all the now-unnecessary require_once() calls throughout the plugin. I am really hoping you will see this as a plus and accept it, and then I can move forward with the next step of refactor as in the issue above.

While I was at it PhpStorm pointed out several errors and oversights and I created an individual commit for each of those, but the first commit has everything needed for and related to the autoloader.

@mikeschinkel mikeschinkel changed the title Add autoloader to reduce complexity Added autoloader to reduce complexity Dec 8, 2017

@westonruter
Copy link
Member

left a comment

@mikeschinkel Thanks, I think this makes sense. A few changes needed before merging.

.gitmodules Outdated
[submodule "dev-lib"]
path = dev-lib
url = https://github.com/xwp/wp-dev-lib.git
branch = master

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

This file needs to be restored.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 8, 2017

Author Contributor

I deleted it because it was blocking my ability to commit and I thought my pull request would not have included the deletion. I am not exactly sure what I was doing wrong or how to fix it (I gave up on submodules long ago as I always had issues with them.)

Is there any chance that you can accept the pull request after I fix everything else, and then restore manually?

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

The lack of the submodule is causing Travis CI to fail: https://travis-ci.org/Automattic/amp-wp/jobs/313361694#L186

So it needs to be restored before we merge the PR so that the automated tests and checks can be performed.

What is the error you're having?

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 8, 2017

Author Contributor

Let me see if I can figure it out.

I was using SourceTree to allow me to make partial commits of files and it said I needed the submodule but was locked in the dialog and would not let me exit. When I deleted the file SourceTree recognized the change and immediately commit my changes.

I will try reverting this commit and see if I can get things to work from the command line. If I have an issue I will let you know otherwise I will resolve it and we can just move forward.

*
* @return AMP_Autoloader
*/
public static function instance() {

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

There aren't any singletons in this plugin, so I think static methods should be used instead for consistency.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 8, 2017

Author Contributor

Funny, I debated using static but assumed you'd prefer instances. :-)

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Normally I'd prefer non-singleton instances, but given most classes are just being used as namespaces with static methods, using static here again seems to be most in keeping with everything else.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 9, 2017

Author Contributor

Updated to use static methods.

public static function register() {
static $registered = false;
if ( ! $registered ) {
spl_autoload_register( [ static::instance(), '_autoload' ] );

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Syntax error in PHP 5.2

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 8, 2017

Author Contributor

5.2? :-o
Fixed.

This comment has been minimized.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 9, 2017

Author Contributor

Noted.

AMP_Autoloader::register();

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Extra linebreaks at end of file.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 8, 2017

Author Contributor

Fixed.

@@ -106,6 +106,11 @@ function amp_add_options_menu() {
}
add_action( 'wp_loaded', 'amp_add_options_menu' );
/**

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Missing function description, param description, and return description. We've also been adding @since tags.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 9, 2017

Author Contributor

I do not understand that function's parameter well enough to fully document so I backed out the partial PHPDoc I had added.

@@ -1,5 +1,10 @@
<?php
/**

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Missing function description, param description, and return description. We've also been adding @since tags.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 9, 2017

Author Contributor

Your requests to document this caused me to go down the rabbit hole for the past (far too many) hours. Every time I would document something it would cause PhpStorm to flag something else as being "off", so I ended up documented all of the sanitizers.

Note I found a few places that would throw errors, e.g. DOMNode $nodes using methods that are only on DOMElement so I added a variable of logic to ensure errors are not thrown during the normal course of usage when WP_DEBUG is set to true. One such example is I added the following in several places:

if ( ! $node instanceof DOMElement ) {
    return;
}

I hope that was okay...

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 10, 2017

Member

The node type checks are probably a good idea, as I can see that clearly some of the situations would result in text nodes being used as elements. It looks like some of the instances you added would never result in a non-element being iterated (e.g. $this->dom->getElementsByTagName( self::$tag );) but it doesn't hurt.

@@ -73,6 +71,9 @@ public function sanitize() {
}
}
/**

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 8, 2017

Member

Missing function description, param description. We've also been adding @since tags.

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 9, 2017

Author Contributor

Documented.

@westonruter

This comment has been minimized.

Copy link
Member

commented Dec 10, 2017

We kinda opened a can of worms here because now there are many PHPCS violations being reported, because of the various files being touched, and wp-dev-lib will only report PHPCS errors surrounding actual patches. The PHPCS issues need to be fixed at some point anyway, but I don't know if now is the time.

I am noticing, however, one PHPUnit failure that must be fixed:

There was 1 failure:

1) AMP_Iframe_Converter_Test::test__args__placeholder
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<amp-iframe src="https://example.com/video/132886713" width="500" height="281" sandbox="allow-scripts allow-same-origin" sizes="(min-width: 500px) 500px, 100vw" class="amp-wp-enforced-sizes"><div placeholder="" class="amp-wp-iframe-placeholder"></div></amp-iframe>'
+'<amp-iframe src="https://example.com/video/132886713" width="500" height="281" sandbox="allow-scripts allow-same-origin" sizes="(min-width: 500px) 500px, 100vw" class="amp-wp-enforced-sizes"><div placeholder="" class="amp-wp-enforced-sizes" src="https://example.com/video/132886713" width="500" height="281" sandbox="allow-scripts allow-same-origin" sizes="(min-width: 500px) 500px, 100vw"></div></amp-iframe>'

.../amp/tests/test-amp-iframe-sanitizer.php:167
@mikeschinkel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2017

@westonruter I have been trying to recreate a local PHPUnit setup to run the tests locally and while I've gotten past numerous issues I've found a roadblock on this error message:

"Unable to locate wordpress-tests-lib"

It appears to be requiring an includes/functions.php if it can find the appropriate directory but I don't know what functions.php is suppose to contain.

Google is failing me to find the right solution to this. I'm sure I could power through it given enough time but I am hoping you can make me more productive and help me know how to solve this one.

Help?

@westonruter

This comment has been minimized.

Copy link
Member

commented Dec 10, 2017

@mikeschinkel Yeah, there's a couple options here.

For one, you can add the following to your .bash_profile:

export WP_TESTS_DIR=~/path/to/wordpress-develop/tests/phpunit/

That assumes you have a fully up-and-running wordpress-develop core unit tests on the machine.

Otherwise, you can just run the unit tests inside of VVV. You can see that it has this out of the box: https://github.com/Varying-Vagrant-Vagrants/VVV/blob/5b4ee4b/config/bash_profile#L29-L33

@mikeschinkel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

@westonruter Thanks. That got me past that roadblock.

However the next roadblock is trying to use either PHPUnit 4.8 or 5.7. With PHPUnit 4.8.9 and PHP 5.3 I get failures at:

Parse error: syntax error, unexpected '[' in /.../plugins/amp-wp/vendor/doctrine/instantiator/src/Doctrine/Instantiator/Instantiator.php on line 45

Clearly I have a later version of doctrine/instantiator but since my composer.json does not reference it directly I have no clue how to correct this.

With PHPUnit 5.7.9 and PHP 5.6 I get failures at:

PHP Warning: Unsupported declare 'strict_types' in /.../plugins/amp-wp/vendor/sebastian/global-state/src/Snapshot.php on line 11

When I switch to PHPUnit 6.5.4 and PHP 7.0.17 I can get tests to run, but there are more than one tests failed. Should I work through all the failures using PHP 7.x?

@mikeschinkel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

@westonruter Good news! I was about to get all 501 tests to pass, at least with PHPUnit 6.5.4 and PHP 7.0. I'll have a commit very soon.

mikeschinkel added some commits Dec 11, 2017

@mikeschinkel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

@westonruter Yes, because I was trying to use PhpStorm to run the tests (as it provides such a nice interface) and for some reason PhpStorm would not recognize the .phar; it would only recognize when I pointed to the Composer-generated autoload.php.

I added one without "dist" — for obvious reasons — and I omitted a version number because I have found adding one makes it difficult for the person wanting to use a specific version when the Git tag does not exactly match the version in the composer.json file; kind of a "man with two watches" problem.

mikeschinkel added some commits Dec 11, 2017

Remove "repositories" from composer.json because it is not merged in …
…yet and thus is causing the build to fail.
Had to remove require: php>=5.2 because it appears to be breaking the…
… build. (Methinks we would need to update the composer.json file for each Travis build to support the proper version of PHP?)
@mikeschinkel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

@westonruter Finally! What a rabbit hole that was. :-)

Let me know if you have an issues otherwise I will start working on how to implement a full theme in AMP and run the ideas by you either in issue #827 or via a new pull request.

public function get_styles() {
if ( ! $this->did_convert_elements ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 11, 2017

Member

Why this if $this->styles is already an array()?

This comment has been minimized.

Copy link
@mikeschinkel

mikeschinkel Dec 11, 2017

Author Contributor

Consistency. But to your point I guess it is not needed.

@westonruter westonruter changed the title Added autoloader to reduce complexity Added autoloader to reduce complexity; clean phpcs issues Dec 11, 2017

@westonruter westonruter changed the title Added autoloader to reduce complexity; clean phpcs issues Added autoloader to reduce complexity; fix phpcs issues Dec 11, 2017

@westonruter westonruter merged commit b94b10b into ampproject:develop Dec 11, 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
@mikeschinkel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

Woohoo! :-D

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

westonruter added a commit that referenced this pull request Jan 13, 2018

Revert "Merge pull request #725 from Automattic/amedina/add-amp-actio…
…ns-class-hierarchy"

This reverts commit c5237e6, reversing
changes made to 41af096.

Fixes merge conflicts introduced by #810 and #828.

westonruter added a commit that referenced this pull request Jan 13, 2018

Revert "Merge pull request #725 from Automattic/amedina/add-amp-actio…
…ns-class-hierarchy"

This reverts commit c5237e6, reversing
changes made to 41af096.

Fixes merge conflicts introduced by #810 and #828.

westonruter added a commit that referenced this pull request Jan 19, 2018

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.