From b39d76be080fb835f7342020e3caa559e8764b7b Mon Sep 17 00:00:00 2001
From: Bryan Davis <bd808@wikimedia.org>
Date: Wed, 20 Apr 2016 23:07:36 -0600
Subject: [PATCH] General cleanup of CSSRenderer

* Add phpdoc comments
* Rename some variables to be a bit more clear for new readers
* Break up render() to make things more readable and reduce cyclomatic
  complexity

Change-Id: Iceeb1f6eb09b61efe6b81f359d28741f54fe88ad
---
 CSSRenderer.php                      | 134 ++++++++++++++++++---------
 tests/phpunit/CSSParseRenderTest.php |  17 +++-
 2 files changed, 104 insertions(+), 47 deletions(-)

diff --git a/CSSRenderer.php b/CSSRenderer.php
index 70d2631..40d7614 100644
--- a/CSSRenderer.php
+++ b/CSSRenderer.php
@@ -1,6 +1,4 @@
 <?php
-
-
 /**
  * @file
  * @ingroup Extensions
@@ -13,85 +11,131 @@
  */
 class CSSRenderer {
 
-	private	$bymedia;
+	/** @var array $byMedia */
+	private $byMedia;
 
 	function __construct() {
-		$this->bymedia = [];
+		$this->byMedia = [];
 	}
 
 	/**
 	 * Adds (and merge) a parsed CSS tree to the render list.
 	 *
 	 * @param array $rules The parsed tree as created by CSSParser::rules()
-	 * @param string $media Forcibly specified @media block selector.  Normally unspecified
-	 *  and defaults to the empty string.
+	 * @param string $media Forcibly specified @media block selector.
 	 */
 	function add( $rules, $media = '' ) {
-		if ( !array_key_exists( $media, $this->bymedia ) ) {
-			$this->bymedia[$media] = [];
+		if ( !array_key_exists( $media, $this->byMedia ) ) {
+			$this->byMedia[$media] = [];
 		}
 
-		foreach ( $rules as $at ) {
-			switch ( strtolower( $at['name'] ) ) {
+		foreach ( $rules as $rule ) {
+			switch ( strtolower( $rule['name'] ) ) {
 				case '@media':
 					if ( $media == '' ) {
-						$this->add( $at['rules'], "@media ".$at['text'] );
+						$this->add(
+							$rule['rules'], "@media {$rule['text']}"
+						);
 					}
 					break;
 				case '':
-					$this->bymedia[$media] = array_merge( $this->bymedia[$media], $at['rules'] );
+					$this->byMedia[$media] = array_merge(
+						$this->byMedia[$media], $rule['rules']
+					);
 					break;
 			}
 		}
 	}
 
 	/**
-	 * Renders the collected CSS trees into a string suitable for inclusion
+	 * Render the collected CSS trees into a string suitable for inclusion
 	 * in a <style> tag.
 	 *
+	 * @param array $functionWhitelist List of functions that are allowed
+	 * @param array $propertyBlacklist List of properties that not allowed
 	 * @return string Rendered CSS
 	 */
-	function render( $functionWhitelist = [], $propertyBlacklist = [] ) {
+	function render(
+		array $functionWhitelist = [],
+		array $propertyBlacklist = []
+	) {
+		// Normalize whitelist and blacklist values to lowercase
+		$functionWhitelist = array_map( 'strtolower', $functionWhitelist );
+		$propertyBlacklist = array_map( 'strtolower', $propertyBlacklist );
 
 		$css = '';
-
-		foreach ( $this->bymedia as $at => $rules ) {
-			if ( $at != '' ) {
-				$css .= "$at {\n";
+		foreach ( $this->byMedia as $media => $rules ) {
+			if ( $media !== '' ) {
+				$css .= "{$media} {\n";
 			}
 			foreach ( $rules as $rule ) {
-				if ( $rule
-					and array_key_exists( 'selectors', $rule )
-					and array_key_exists( 'decls', $rule ) )
-				{
-					$css .= implode( ',', $rule['selectors'] ) . "{";
-					foreach ( $rule['decls'] as $key => $value ) {
-						if ( !in_array( strtolower( $key ), $propertyBlacklist ) ) {
-							$blacklisted = false;
-							foreach ( $value as $prop ) {
-								if ( preg_match( '/^ ([^ \n\t]+) [ \n\t]* \( $/x', $prop, $match ) ) {
-									if ( !in_array( strtolower( $match[1] ), $functionWhitelist ) ) {
-										$blacklisted = true;
-										break;
-									}
-								}
-							}
-							if ( !$blacklisted ) {
-								$css .= "$key:" . implode( '', $value ) . ';';
-							}
-						}
-					}
-					$css .= "} ";
+				if ( $rule !== null ) {
+					$css .= $this->renderRule(
+						$rule, $functionWhitelist, $propertyBlacklist );
 				}
 			}
-			if ( $at != '' ) {
-				$css .= "} ";
+			if ( $media !== '' ) {
+				$css .= '} ';
 			}
 		}
-
 		return $css;
 	}
 
+	/**
+	 * Render a single rule.
+	 *
+	 * @param array $rule Parsed rule
+	 * @param array $functionWhitelist List of functions that are allowed
+	 * @param array $propertyBlacklist List of properties that not allowed
+	 * @return string Rendered CSS
+	 */
+	private function renderRule(
+		array $rule,
+		array $functionWhitelist,
+		array $propertyBlacklist
+	) {
+		$css = '';
+		if ( $rule &&
+			array_key_exists( 'selectors', $rule ) &&
+			array_key_exists( 'decls', $rule )
+		) {
+			$css .= implode( ',', $rule['selectors'] ) . '{';
+			foreach ( $rule['decls'] as $prop => $values ) {
+				$css .= $this->renderDecl(
+					$prop, $values, $functionWhitelist, $propertyBlacklist );
+			}
+			$css .= '} ';
+		}
+		return $css;
+	}
+
+	/**
+	 * Render a property declaration.
+	 *
+	 * @param string $prop Property name
+	 * @param array $values Parsed property values
+	 * @param array $functionWhitelist List of functions that are allowed
+	 * @param array $propertyBlacklist List of properties that not allowed
+	 * @return string Rendered CSS
+	 */
+	private function renderDecl(
+		$prop,
+		array $values,
+		array $functionWhitelist,
+		array $propertyBlacklist
+	) {
+		if ( in_array( strtolower( $prop ), $propertyBlacklist ) ) {
+			// Property is blacklisted
+			return '';
+		}
+		foreach ( $values as $value ) {
+			if ( preg_match( '/^ (\S+) \s* \( $/x', $value, $match ) ) {
+				if ( !in_array( strtolower( $match[1] ), $functionWhitelist ) ) {
+					// Function is blacklisted
+					return '';
+				}
+			}
+		}
+		return $prop . ':' . implode( '', $values ) . ';';
+	}
 }
-
-
diff --git a/tests/phpunit/CSSParseRenderTest.php b/tests/phpunit/CSSParseRenderTest.php
index c6ac683..251d721 100644
--- a/tests/phpunit/CSSParseRenderTest.php
+++ b/tests/phpunit/CSSParseRenderTest.php
@@ -19,8 +19,8 @@ class CSSParseRenderTest extends MediaWikiTestCase {
 		$expect,
 		$source,
 		$baseSelector = '.X ',
-		$functionWhitelist = [ 'whitelisted' ],
-		$propertyBlacklist = [ '-evil' ]
+		array $functionWhitelist = [ 'whitelisted' ],
+		array $propertyBlacklist = [ '-evil' ]
 	) {
 		$tree = new CSSParser( $source );
 		$rules = $tree->rules( $baseSelector );
@@ -239,6 +239,19 @@ prop2/*
 	;prop3		:not/**/whitelisted( val3 );}
 CSS
 			],
+			'Whitelist normalized' => [
+				'expect' => '.foo {bar:whitelisted(1);} ',
+				'css' => '.foo { bar: whitelisted(1); baz: url(1); }',
+				'prefix' => '',
+				'whitelist' => [ 'WHITELISTED' ],
+			],
+			'Blacklist normalized' => [
+				'expect' => '.foo {baz:1;} ',
+				'css' => '.foo { blacklisted: 1; baz: 1; }',
+				'prefix' => '',
+				'whitelist' => [],
+				'blacklist' => [ 'BLACKLISTED' ],
+			],
 		];
 	}
 }