-
Notifications
You must be signed in to change notification settings - Fork 381
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
Changes from 7 commits
8346524
5c1fa20
33c0ebf
08bac6b
e5c1a8c
8b267ef
8b34e24
b54243b
38cccab
f13c385
03c126b
fdd1c74
48c492e
1a0b53c
fb10624
19b6f04
b1ee243
010dad4
8fad986
241e866
38992ed
5fcbc2b
2105af0
f85c366
564a9d9
f90c16d
b62132a
5417aeb
19658c9
a66c992
23bf64f
bfa54bc
129bda7
b1feee0
00bea9b
b74ee8f
6000b50
6e0c13b
bd97046
215a27b
85a46ba
de7f617
5d1157f
42c0600
47edc2f
3e15941
f5b2d2f
8b416f9
575ac66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
<?php | ||
|
||
/** | ||
* Autoload the classes used by the AMP plugin. | ||
* | ||
* Class AMP_Autoloader | ||
*/ | ||
class AMP_Autoloader { | ||
|
||
/** | ||
* Map of Classname to relative filepath sans extension. | ||
* | ||
* @example Format: | ||
* | ||
* array( | ||
* 'Class_Name1' => 'subdir-of-includes/filename1', | ||
* 'Class_Name2' => '2nd-subdir-of-includes/filename2', | ||
* 'Class_Name3' => '!/full/path/to/filename3.php', | ||
* ); | ||
* | ||
* @var string[] | ||
*/ | ||
private $_classmap; | ||
|
||
/** | ||
* Provide access to the singlton instance | ||
* | ||
* @var AMP_Autoloader | ||
*/ | ||
private static $_instance; | ||
|
||
/** | ||
* Perform the autoload. | ||
* | ||
* Run as few lines of code as possible. | ||
* | ||
* If $class_name has first char as '!' strip it and assume a full path w/extension. | ||
* Otherwise assume subdir or /includes/ w/o extension. | ||
* | ||
* @param string $class_name | ||
*/ | ||
function _autoload( $class_name ) { | ||
if ( ! isset( $this->_classmap[ $class_name ] ) ) { | ||
return; | ||
} | ||
$filepath = $this->_classmap[ $class_name ]; | ||
require( '!' !== $filepath[ 0 ] ? __DIR__ . "/includes/{$filepath}.php" : substr( $filepath, 1 ) ); | ||
} | ||
|
||
/** | ||
* Trigger creation of AMP_Autoloader singleton instance and return it. | ||
* | ||
* @return AMP_Autoloader | ||
*/ | ||
public static function instance() { | ||
if ( ! isset( static::$_instance ) ) { | ||
static::$_instance = new static(); | ||
} | ||
return static::$_instance; | ||
} | ||
|
||
/** | ||
* Registers to autoloader to PHP. | ||
* | ||
* Called at the end of this file; calling a second time has no effect. | ||
*/ | ||
public static function register() { | ||
static $registered = false; | ||
if ( ! $registered ) { | ||
spl_autoload_register( [ static::instance(), '_autoload' ] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Syntax error in PHP 5.2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 5.2? :-o There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. Currently the plugin supports PHP 5.2: https://github.com/Automattic/amp-wp/blob/eeb502b28807c1be80e59937fab6112e406b204b/.travis.yml#L45-L47 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted. |
||
$registered = true; | ||
} | ||
} | ||
|
||
/** | ||
* Allows an extensions plugin to register a class and its file for autoloading | ||
* | ||
* @param string $class_name Full classname (include namespace if applicable) | ||
* @param string $filepath Absolute filepath to class | ||
*/ | ||
public static function register_autoload_class( $class_name, $filepath ) { | ||
self::$_instance->_classmap[ $class_name ] = '!' . $filepath; | ||
} | ||
|
||
/** | ||
* Creates instance and initializes classmap. | ||
* | ||
* AMP_Autoloader constructor. | ||
*/ | ||
private function __construct() { | ||
$this->_classmap = array( | ||
'AMP_Actions' => 'actions/class-amp-actions', | ||
'AMP_Frontend_Actions' => 'actions/class-amp-frontend-actions', | ||
'AMP_Paired_Post_Actions' => 'actions/class-amp-paired-post-actions', | ||
'AMP_Template_Customizer' => 'admin/class-amp-customizer', | ||
'AMP_Post_Meta_Box' => 'admin/class-amp-post-meta-box', | ||
'AMP_Post_Type_Support' => 'class-amp-post-type-support', | ||
'AMP_Base_Embed_Handler' => 'embeds/class-amp-base-embed-handler', | ||
'AMP_DailyMotion_Embed_Handler' => 'embeds/class-amp-dailymotion-embed', | ||
'AMP_Facebook_Embed_Handler' => 'embeds/class-amp-facebook-embed', | ||
'AMP_Gallery_Embed_Handler' => 'embeds/class-amp-gallery-embed', | ||
'AMP_Instagram_Embed_Handler' => 'embeds/class-amp-instagram-embed', | ||
'AMP_Pinterest_Embed_Handler' => 'embeds/class-amp-pinterest-embed', | ||
'AMP_SoundCloud_Embed_Handler' => 'embeds/class-amp-soundcloud-embed', | ||
'AMP_Twitter_Embed_Handler' => 'embeds/class-amp-twitter-embed', | ||
'AMP_Vimeo_Embed_Handler' => 'embeds/class-amp-vimeo-embed', | ||
'AMP_Vine_Embed_Handler' => 'embeds/class-amp-vine-embed', | ||
'AMP_YouTube_Embed_Handler' => 'embeds/class-amp-youtube-embed', | ||
'FastImage' => '/lib/fastimage/class-fastimage.php', | ||
'WillWashburn\\Stream\\Exception\\StreamBufferTooSmallException' => 'lib/fasterimage/Stream/Exception/StreamBufferTooSmallException', | ||
'WillWashburn\\Stream\\StreamableInterface' => 'lib/fasterimage/Stream/StreamableInterface', | ||
'WillWashburn\\Stream\\Stream' => 'lib/fasterimage/Stream/Stream', | ||
'FasterImage\\Exception\\InvalidImageException' => 'lib/fasterimage/Exception/InvalidImageException', | ||
'FasterImage\\ExifParser' => 'lib/fasterimage/ExifParser', | ||
'FasterImage\\ImageParser' => 'lib/fasterimage/ImageParser', | ||
'FasterImage\\FasterImage' => 'lib/fasterimage/FasterImage', | ||
'AMP_Analytics_Options_Submenu' => 'options/class-amp-analytics-options-submenu', | ||
'AMP_Options_Menu' => 'options/class-amp-options-menu', | ||
'AMP_Options_Manager' => 'options/class-amp-options-manager', | ||
'AMP_Analytics_Options_Submenu_Page' => 'options/views/class-amp-analytics-options-submenu-page', | ||
'AMP_Options_Menu_Page' => 'options/views/class-amp-options-menu-page', | ||
'AMP_Allowed_Tags_Generated' => 'sanitizers/class-amp-allowed-tags-generated', | ||
'AMP_Audio_Sanitizer' => 'sanitizers/class-amp-audio-sanitizer', | ||
'AMP_Base_Sanitizer' => 'sanitizers/class-amp-base-sanitizer', | ||
'AMP_Blacklist_Sanitizer' => 'sanitizers/class-amp-blacklist-sanitizer', | ||
'AMP_Iframe_Sanitizer' => 'sanitizers/class-amp-iframe-sanitizer', | ||
'AMP_Img_Sanitizer' => 'sanitizers/class-amp-img-sanitizer', | ||
'AMP_Playbuzz_Sanitizer' => 'sanitizers/class-amp-playbuzz-sanitizer', | ||
'AMP_Style_Sanitizer' => 'sanitizers/class-amp-style-sanitizer', | ||
'AMP_Tag_And_Attribute_Sanitizer' => 'sanitizers/class-amp-tag-and-attribute-sanitizer', | ||
'AMP_Video_Sanitizer' => 'sanitizers/class-amp-video-sanitizer', | ||
'AMP_Customizer_Design_Settings' => 'settings/class-amp-customizer-design-settings', | ||
'AMP_Customizer_Settings' => 'settings/class-amp-customizer-settings', | ||
'AMP_Content' => 'templates/class-amp-content', | ||
'AMP_Content_Sanitizer' => 'templates/class-amp-content-sanitizer', | ||
'AMP_Post_Template' => 'templates/class-amp-post-template', | ||
'AMP_DOM_Utils' => 'utils/class-amp-dom-utils', | ||
'AMP_HTML_Utils' => 'utils/class-amp-html-utils', | ||
'AMP_Image_Dimension_Extractor' => 'utils/class-amp-image-dimension-extractor', | ||
'AMP_String_Utils' => 'utils/class-amp-string-utils', | ||
'AMP_WP_Utils' => 'utils/class-amp-wp-utils', | ||
'WPCOM_AMP_Polldaddy_Embed' => 'wpcom/class-amp-polldaddy-embed.php', | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Call method to register this autoloader with PHP. | ||
*/ | ||
AMP_Autoloader::register(); | ||
|
||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra linebreaks at end of file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,6 @@ | ||
<?php | ||
// Callbacks for adding AMP-related things to the admin. | ||
|
||
require_once AMP__DIR__ . '/includes/options/class-amp-options-menu.php'; | ||
require_once AMP__DIR__ . '/includes/options/views/class-amp-options-manager.php'; | ||
|
||
define( 'AMP_CUSTOMIZER_QUERY_VAR', 'customize_amp' ); | ||
|
||
add_action( 'admin_init', 'AMP_Options_Manager::register_settings' ); | ||
|
@@ -33,6 +30,9 @@ function amp_maybe_init_customizer() { | |
add_action( 'admin_menu', 'amp_add_customizer_link' ); | ||
} | ||
|
||
/** | ||
* @return string|null | ||
*/ | ||
function amp_admin_get_preview_permalink() { | ||
/** | ||
* Filter the post type to retrieve the latest for use in the AMP template customizer. | ||
|
@@ -42,7 +42,7 @@ function amp_admin_get_preview_permalink() { | |
$post_type = (string) apply_filters( 'amp_customizer_post_type', 'post' ); | ||
|
||
if ( ! post_type_supports( $post_type, 'amp' ) ) { | ||
return; | ||
return null; | ||
} | ||
|
||
$post_ids = get_posts( array( | ||
|
@@ -106,6 +106,11 @@ function amp_add_options_menu() { | |
} | ||
add_action( 'wp_loaded', 'amp_add_options_menu' ); | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing function description, param description, and return description. We've also been adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not understand that function's parameter well enough to fully document so I backed out the partial PHPDoc I had added. |
||
* @param array $analytics | ||
* | ||
* @return array | ||
*/ | ||
function amp_add_custom_analytics( $analytics ) { | ||
$analytics_entries = AMP_Options_Manager::get_option( 'analytics', array() ); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,10 @@ | ||
<?php | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing function description, param description, and return description. We've also been adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
I hope that was okay... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
* @param int $post_id | ||
* | ||
* @return string | ||
*/ | ||
function amp_get_permalink( $post_id ) { | ||
$pre_url = apply_filters( 'amp_pre_get_permalink', false, $post_id ); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,9 @@ | ||
<?php | ||
|
||
function amp_load_fasterimage_classes() { | ||
// We're not using composer to pull in FasterImage so we need to load the files manually | ||
$fasterimage__DIR__ = dirname( __FILE__ ); | ||
|
||
// Stream files | ||
require_once( $fasterimage__DIR__ . '/Stream/Exception/StreamBufferTooSmallException.php' ); | ||
require_once( $fasterimage__DIR__ . '/Stream/StreamableInterface.php' ); | ||
require_once( $fasterimage__DIR__ . '/Stream/Stream.php' ); | ||
|
||
// FasterImage files | ||
require_once( $fasterimage__DIR__ . '/Exception/InvalidImageException.php' ); | ||
require_once( $fasterimage__DIR__ . '/ExifParser.php' ); | ||
require_once( $fasterimage__DIR__ . '/ImageParser.php' ); | ||
require_once( $fasterimage__DIR__ . '/FasterImage.php' ); | ||
_deprecated_function( __FUNCTION__, '0.6.0' ); | ||
} | ||
|
||
function amp_get_fasterimage_client( $user_agent ) { | ||
if ( ! class_exists( 'FasterImage\FasterImage' ) ) { | ||
amp_load_fasterimage_classes(); | ||
} | ||
|
||
return new FasterImage\FasterImage( $user_agent ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There aren't any singletons in this plugin, so I think static methods should be used instead for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny, I debated using static but assumed you'd prefer instances. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use static methods.