Incorporate @mo's reviews

ampproject · Jun 21, 2017 · 22e41dc · 22e41dc
1 parent 854da9c
commit 22e41dc
Show file tree

Hide file tree

Showing 9 changed files with 127 additions and 93 deletions.
diff --git a/includes/admin/amp-wp-admin-styles.php b/includes/admin/amp-wp-admin-styles.php
diff --git a/includes/admin/functions.php b/includes/admin/functions.php
@@ -3,7 +3,6 @@
 
 require_once( AMP__DIR__ . '/includes/options/class-amp-options-menu.php' );
 require_once( AMP__DIR__ . '/includes/options/views/class-amp-analytics-options-serializer.php' );
-require_once( AMP__DIR__ . '/includes/admin/amp-wp-admin-styles.php' );
 
 define( 'AMP_CUSTOMIZER_QUERY_VAR', 'customize_amp' );
 
@@ -24,12 +23,6 @@ function amp_init_customizer() {
 
 	// Add a link to the Customizer
 	add_action( 'admin_menu', 'amp_add_customizer_link' );
-
-	// Add a link to Settings
-	add_action( 'admin_menu', 'amp_add_amp_options_link' );
-
-	// Trigger analytics options serializer on analytics option's save
-	add_action( 'admin_post_analytics_options', 'Analytics_Options_Serializer::save' );
 }
 
 function amp_admin_get_preview_permalink() {
@@ -83,43 +76,54 @@ function amp_add_customizer_link() {
 /**
  * Registers a top-level menu for AMP configuration options
  */
-function amp_add_amp_options_link() {
+function amp_add_options_menu() {
 	$amp_options = new AMP_Options_Menu();
 	$amp_options->init();
 }
+add_action('admin_menu','amp_add_options_menu');
+// Action to trigger analytics options serializer on analytics option's save
+add_action( 'admin_post_analytics_options', 'Analytics_Options_Serializer::save' );
+
 
 /**
  * Grab the analytics options from the DB and return $analytics option
  * @return array
  */
-function get_analytics_component_fields($option) {
+function amp_get_analytics_component_fields($option) {
 
-	$id= $option[0];
-	$type = $option[1];
-	$config = $option[2];
+	list( $id, $type, $config ) = $option;
 
 	$fields = array();
 	$component_index = $type . '-' . $id;
 	$fields['id'] =  $component_index;
 	$fields['type'] = $type;
 	$fields['attributes'] = array();
 
-	$analytics_json = json_decode( stripslashes( $config ), true );
+	$analytics_json = json_decode(  $config , true );
 	$fields['config_data'] = $analytics_json;
 
 	return $fields;
 }
 
-function amp_add_custom_analytics( ) {
-	$analytics = array();
-	$analytics_options = get_option( 'analytics', array() );
 
+function amp_add_custom_analytics() {
+	$amp_options = get_option('amp-options');
+	if ( $amp_options ) {
+		$analytics_options = $amp_options[ 'amp-analytics'];
+	}
+
+	if ( ! $analytics_options )
+		return;
+
+	$analytics = array();
 	foreach ( $analytics_options as $option ) {
-		$fields = get_analytics_component_fields($option);
+		$fields = amp_get_analytics_component_fields($option);
 		$analytics[$fields['id']] = $fields;
 	}
 
 	return $analytics;
 }
 add_filter( 'amp_post_template_analytics', 'amp_add_custom_analytics' );
 
+
+
diff --git a/includes/amp-post-template-actions.php b/includes/amp-post-template-actions.php
@@ -75,6 +75,7 @@ function amp_post_template_add_analytics_script( $data ) {
 add_action( 'amp_post_template_footer', 'amp_post_template_add_analytics_data' );
 function amp_post_template_add_analytics_data( $amp_template ) {
 	$analytics_entries = $amp_template->get( 'amp_analytics' );
+	var_dump( $analytics_entries );
 	if ( empty( $analytics_entries ) ) {
 		return;
 	}

diff --git a/includes/options/class-amp-analytics-options-submenu.php b/includes/options/class-amp-analytics-options-submenu.php
@@ -16,16 +16,33 @@ public function __construct( $parent_menu_slug ) {
 
 	public function init() {
 		$this->add_submenu();
+		add_action(
+			'admin_head',
+			'AMP_Analytics_Options_Submenu::amp_options_styles'
+		);
 	}
 
 	private function add_submenu() {
 		add_submenu_page(
 			$this->parent_menu_slug,
-			'AMP Analytics Options',
-			'Analytics',
+			__( 'AMP Analytics Options', 'amp' ),
+			__( 'Analytics', 'amp' ),
 			'manage_options',
 			$this->menu_slug,
 			array($this->menu_page, 'render')
 		);
 	}
-}
+
+	public static function amp_options_styles() {
+		?>
+		<style>
+			.analytics-data-container #delete {
+				background: red;
+				border-color: red;
+				text-shadow: 0 0 0;
+				margin: 0 5px;
+			}
+		</style>;
+		<?php
+	}
+}
diff --git a/includes/options/class-amp-options-menu.php b/includes/options/class-amp-options-menu.php
@@ -2,6 +2,7 @@
 
 require_once( AMP__DIR__ . '/includes/options/class-amp-analytics-options-submenu.php' );
 require_once( AMP__DIR__ . '/includes/options/views/class-amp-options-menu-page.php' );
+require_once( AMP__DIR__ . '/includes/options/views/class-amp-analytics-options-serializer.php' );
 
 class AMP_Options_Menu {
 
@@ -26,8 +27,8 @@ public function init() {
 	 */
 	public function add_amp_options_menu( $submenus ) {
 		add_menu_page(
-			'AMP Plugin Options',
-			'AMP',
+			__( 'AMP Plugin Options', 'amp' ),
+			__( 'AMP', 'amp' ),
 			'manage_options',
 			$this->menu_slug,
 			array( $this->menu_page, 'render' )
@@ -37,4 +38,5 @@ public function add_amp_options_menu( $submenus ) {
 			$submenu->init($this->menu_slug);
 		}
 	}
-}
+}
+
diff --git a/includes/options/views/class-amp-analytics-options-serializer.php b/includes/options/views/class-amp-analytics-options-serializer.php
@@ -2,38 +2,55 @@
 
 class Analytics_Options_Serializer {
 
+	private static function valid_json( $data ) {
+		if (!empty($data)) {
+			@json_decode($data);
+			return (json_last_error() === JSON_ERROR_NONE);
+		}
+		return false;
+	}
+
 	public static function save() {
 
-		$option_name = 'analytics';
+		$option_name = 'amp-analytics';
 
-		if ( !( empty( $_POST['vendor-type'] ) ||
-		     empty( $_POST['config'] ) ) ) {
+		if ( ! ( empty( $_POST['vendor-type'] ) || empty( $_POST['config'] ) ) &&
+				Analytics_Options_Serializer::valid_json( stripslashes($_POST['config'] ) ) ) {
 
 			if ( empty( $_POST['id-value'] ) ) {
 				$_POST['id-value'] = md5( $_POST['config'] );
 			}
 
-			$new_analytics_options = array(
+			// Prepare the data for the new analytics setting
+			$new_analytics_option = array(
 				$_POST['id-value'],
 				$_POST['vendor-type'],
 				stripslashes( $_POST['config'] )
 			);
-
+			// Identifier for analytics option
 			$inner_option_name = $_POST['vendor-type'] . '-' . $_POST['id-value'];
-			$analytics_options = get_option( $option_name );
-			if ( ! $analytics_options ) {
-				$analytics_options = array();
+
+			// Grab the amp_options from the DB
+			$amp_options = get_option( 'amp-options' );
+			if ( ! $amp_options ) {
+				$amp_options = array();
 			}
 
+			// Grab the amp-analytics options
+			$amp_analytics = isset($amp_options[ $option_name ])
+				? $amp_options[ $option_name ]
+				: array();
+
 			if ( isset( $_POST['delete'] ) ) {
-				unset( $analytics_options[ $inner_option_name ] );
+				unset( $amp_analytics[ $inner_option_name ] );
 			} else {
-				$analytics_options[ $inner_option_name ] = $new_analytics_options;
+				$amp_analytics[ $inner_option_name ] = $new_analytics_option;
 			}
-			update_option( $option_name, $analytics_options, false );
+			$amp_options[ $option_name ] = $amp_analytics;
+			update_option( 'amp-options' , $amp_options, false );
 		}
 		// [Redirect] Keep the user in the analytics options page
-		// Wrap in is_admin() to enable phpunit tests to exercise this code
+		// Wrap with is_admin() to enable phpunit tests to exercise this code
 		if ( is_admin() ) {
 			wp_redirect( admin_url( 'admin.php?page=amp-analytics-options' ) );
 		}

diff --git a/includes/options/views/class-amp-analytics-options-submenu-page.php b/includes/options/views/class-amp-analytics-options-submenu-page.php
@@ -9,19 +9,19 @@ private function render_option($id = "", $type = "", $config = "") {
 			?>
 			<div class="analytics-data-container">
 				<form method="post" action="<?php echo esc_html( admin_url( 'admin-post.php' ) ); ?>">
-					<h2>Analytics Component: <?php echo ($type ? $type . ':' : '') . substr($id, -6) ?></h2>
+					<h2><?php echo __( 'Analytics Component', 'amp' ) ?>: <?php echo ($type ? $type . ':' : '') . substr($id, -6) ?></h2>
 					<div class="options">
 						<p>
-							<label>Type: </label>
-							<input class="option-input" type="text" name=vendor-type value="<?php echo $type; ?>" />
-							<label>Id: </label>
-							<input type="text" name=id value="<?php echo substr($id, -6); ?>" text="alberto" readonly />
+							<label><?php echo __( 'Type', 'amp' ) ?>: </label>
+							<input class="option-input" type="text" name=vendor-type value="<?php echo esc_attr( $type ); ?>" />
+							<label><?php echo __( 'Id', 'amp' ) ?>: </label>
+							<input type="text" name=id value="<?php echo substr(esc_attr( $id ), -6); ?>" readonly />
                             <input type="hidden" name=id-value value="<?php echo $id; ?>" />
 						</p>
 						<p>
-							<label>JSON Configuration:</label>
+							<label><?php echo __( 'JSON Configuration', 'amp' ) ?>:</label>
 							<br />
-							<textarea rows="10" cols="100" name="config"><?php echo stripslashes($config); ?></textarea>
+							<textarea rows="10" cols="100" name="config"><?php echo esc_textarea( $config ); ?></textarea>
 						</p>
 						<input type="hidden" name="action" value="analytics_options">
 					</div><!-- #analytics-data-container -->
@@ -42,7 +42,10 @@ public function render() {
 			<h1><?php echo esc_html( get_admin_page_title() ); ?></h1>
 		<?php
 
-		$analytics_options = get_option('analytics');
+		$amp_options = get_option('amp-options');
+		if ( $amp_options ) {
+			$analytics_options = $amp_options[ 'amp-analytics'];
+        }
 
 		if ( $analytics_options ) {
 			foreach ( $analytics_options as $option ) {

diff --git a/includes/options/views/class-amp-options-menu-page.php b/includes/options/views/class-amp-options-menu-page.php
@@ -4,7 +4,7 @@ class AMP_Options_Menu_Page {
 	public function render() {
 		?>
 		<div class="ampoptions-admin-page">
-			<h1>AMP Plugin Options</h1>
+			<h1><?php echo __( 'AMP Plugin Options', 'amp' ) ?></h1>
 				<p>
 					This admin panel menu contains configuration options
 					for the AMP Plugin.