Skip to content

Commit de7483c

Browse files
Krinklejdforrester
Krinkle
authored and
jdforrester
committed
build: Enable Phan and remove (possibly slow?) use of by-ref
Phan is smart enough to trace the $patterns strings, even through concatenations, and it even validates the would-be concatenated version as valid regexp syntax knowing it can be passed to preg_match. This is amazing. But, it was not smart enough to realize that buildPatterns() is always called before self::$patterns is used. Thus, in addition to the amazing regex syntax validation, it was also at the same time complaining that preg_match could be passed null, which would be invalid. I suspect the main reason it ges confused is that it isn't able to figure out the state after the is_null() call for one key and deduce that this applies to the other keys as well. Fix this by letting buildPatterns() own the array in its entirety, instead of defining half of it in the class, and half of it at runtime. This simplifies the check to just $patterns as a whole being null, which Phan does understand. As a side-effect, this makes the code easier to understand by removing clever use of by-ref, and it also appears to give the code an ~10% speed up. Before: * mediawiki-legacy-shared 752-797/s * ooui-core 62-68/s After: * mediawiki-legacy-shared 815-886/s * ooui-core 71-83/s Also: * Update Wikimedia library boilerplate. * Resolve file_get_contents() redirect so that there's one less Little Snitch prompt to approve :)
1 parent da8323f commit de7483c

File tree

7 files changed

+80
-67
lines changed

7 files changed

+80
-67
lines changed

‎.gitattributes

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
11
/.* export-ignore
2-
/phpcs.xml export-ignore
3-
/phpunit.xml.dist export-ignore
42
/test/ export-ignore
3+
/phpunit.xml.dist export-ignore

‎.phan/config.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<?php
2+
return require __DIR__ . '/../vendor/mediawiki/mediawiki-phan-config/src/config.php';

‎phpcs.xml renamed to ‎.phpcs.xml

File renamed without changes.

‎composer.json

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
"php": ">=7.2.0"
2323
},
2424
"require-dev": {
25+
"mediawiki/mediawiki-phan-config": "0.10.6",
2526
"php-parallel-lint/php-parallel-lint": "^1.3.0",
2627
"phpunit/phpunit": "^8.5.15",
2728
"squizlabs/php_codesniffer": "^3.6.0"
@@ -30,7 +31,13 @@
3031
"test": [
3132
"parallel-lint . --exclude vendor",
3233
"phpunit",
33-
"phpcs -p"
34-
]
34+
"phpcs -sp",
35+
"@phan"
36+
],
37+
"cover": "phpunit --coverage-html coverage",
38+
"fix": [
39+
"phpcbf"
40+
],
41+
"phan": "phan --allow-polyfill-parser --no-progress-bar"
3542
}
3643
}

‎phpunit.xml.dist

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
<?xml version="1.0" encoding="UTF-8"?>
2-
<phpunit bootstrap="vendor/autoload.php" colors="true">
2+
<phpunit colors="true"
3+
beStrictAboutTestsThatDoNotTestAnything="true"
4+
beStrictAboutOutputDuringTests="true">
35
<testsuites>
4-
<testsuite name="CSSJanus Test Suite">
6+
<testsuite name="Tests">
57
<directory>./test/suites</directory>
68
</testsuite>
79
</testsuites>

‎src/CSSJanus.php

Lines changed: 61 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -30,67 +30,63 @@ class CSSJanus {
3030
private const TOKEN_TMP = '`TMP`';
3131
private const TOKEN_COMMENT = '`COMMENT`';
3232

33-
// Patterns defined as null are built dynamically by buildPatterns()
34-
private static $patterns = array(
35-
'tmpToken' => '`TMP`',
36-
'nonAscii' => '[\200-\377]',
37-
'unicode' => '(?:(?:\\\\[0-9a-f]{1,6})(?:\r\n|\s)?)',
38-
'num' => '(?:[0-9]*\.[0-9]+|[0-9]+)',
39-
'unit' => '(?:em|ex|px|cm|mm|in|pt|pc|deg|rad|grad|ms|s|hz|khz|%)',
40-
'body_selector' => 'body\s*{\s*',
41-
'direction' => 'direction\s*:\s*',
42-
'escape' => null,
43-
'nmstart' => null,
44-
'nmchar' => null,
45-
'ident' => null,
46-
'quantity' => null,
47-
'possibly_negative_quantity' => null,
48-
'color' => null,
49-
'url_special_chars' => '[!#$%&*-~]',
50-
'valid_after_uri_chars' => '[\'\"]?\s*',
51-
'url_chars' => null,
52-
'lookahead_not_open_brace' => null,
53-
'lookahead_not_closing_paren' => null,
54-
'lookahead_for_closing_paren' => null,
55-
'lookahead_not_letter' => '(?![a-zA-Z])',
56-
'lookbehind_not_letter' => '(?<![a-zA-Z])',
57-
'chars_within_selector' => '[^\}]*?',
58-
'noflip_annotation' => '\/\*\!?\s*@noflip\s*\*\/',
59-
'noflip_single' => null,
60-
'noflip_class' => null,
61-
'comment' => '/\/\*[^*]*\*+([^\/*][^*]*\*+)*\//',
62-
'direction_ltr' => null,
63-
'direction_rtl' => null,
64-
'left' => null,
65-
'right' => null,
66-
'left_in_url' => null,
67-
'right_in_url' => null,
68-
'ltr_in_url' => null,
69-
'rtl_in_url' => null,
70-
'cursor_east' => null,
71-
'cursor_west' => null,
72-
'four_notation_quantity' => null,
73-
'four_notation_color' => null,
74-
'border_radius' => null,
75-
'box_shadow' => null,
76-
'text_shadow1' => null,
77-
'text_shadow2' => null,
78-
'bg_horizontal_percentage' => null,
79-
'bg_horizontal_percentage_x' => null,
80-
'suffix' => '(\s*(?:!important\s*)?[;}])'
81-
);
33+
private static $patterns = null;
8234

83-
/**
84-
* Build patterns we can't define above because they depend on other patterns.
85-
*/
8635
private static function buildPatterns() {
87-
if (!is_null(self::$patterns['escape'])) {
88-
// Patterns have already been built
36+
if (self::$patterns !== null) {
8937
return;
9038
}
39+
// Patterns defined as null are built dynamically
40+
$patterns = array(
41+
'tmpToken' => '`TMP`',
42+
'nonAscii' => '[\200-\377]',
43+
'unicode' => '(?:(?:\\\\[0-9a-f]{1,6})(?:\r\n|\s)?)',
44+
'num' => '(?:[0-9]*\.[0-9]+|[0-9]+)',
45+
'unit' => '(?:em|ex|px|cm|mm|in|pt|pc|deg|rad|grad|ms|s|hz|khz|%)',
46+
'body_selector' => 'body\s*{\s*',
47+
'direction' => 'direction\s*:\s*',
48+
'escape' => null,
49+
'nmstart' => null,
50+
'nmchar' => null,
51+
'ident' => null,
52+
'quantity' => null,
53+
'possibly_negative_quantity' => null,
54+
'color' => null,
55+
'url_special_chars' => '[!#$%&*-~]',
56+
'valid_after_uri_chars' => '[\'\"]?\s*',
57+
'url_chars' => null,
58+
'lookahead_not_open_brace' => null,
59+
'lookahead_not_closing_paren' => null,
60+
'lookahead_for_closing_paren' => null,
61+
'lookahead_not_letter' => '(?![a-zA-Z])',
62+
'lookbehind_not_letter' => '(?<![a-zA-Z])',
63+
'chars_within_selector' => '[^\}]*?',
64+
'noflip_annotation' => '\/\*\!?\s*@noflip\s*\*\/',
65+
'noflip_single' => null,
66+
'noflip_class' => null,
67+
'comment' => '/\/\*[^*]*\*+([^\/*][^*]*\*+)*\//',
68+
'direction_ltr' => null,
69+
'direction_rtl' => null,
70+
'left' => null,
71+
'right' => null,
72+
'left_in_url' => null,
73+
'right_in_url' => null,
74+
'ltr_in_url' => null,
75+
'rtl_in_url' => null,
76+
'cursor_east' => null,
77+
'cursor_west' => null,
78+
'four_notation_quantity' => null,
79+
'four_notation_color' => null,
80+
'border_radius' => null,
81+
'box_shadow' => null,
82+
'text_shadow1' => null,
83+
'text_shadow2' => null,
84+
'bg_horizontal_percentage' => null,
85+
'bg_horizontal_percentage_x' => null,
86+
'suffix' => '(\s*(?:!important\s*)?[;}])'
87+
);
9188

9289
// @codingStandardsIgnoreStart Generic.Files.LineLength.TooLong
93-
$patterns =& self::$patterns;
9490
$patterns['escape'] = "(?:{$patterns['unicode']}|\\\\[^\\r\\n\\f0-9a-f])";
9591
$patterns['nmstart'] = "(?:[_a-z]|{$patterns['nonAscii']}|{$patterns['escape']})";
9692
$patterns['nmchar'] = "(?:[_a-z0-9-]|{$patterns['nonAscii']}|{$patterns['escape']})";
@@ -133,14 +129,19 @@ private static function buildPatterns() {
133129
$patterns['translate_x'] = "/(transform\s*:[^;}]*)(translateX\s*\(\s*){$patterns['possibly_negative_quantity']}(\s*\))/i";
134130
$patterns['translate'] = "/(transform\s*:[^;}]*)(translate\s*\(\s*){$patterns['possibly_negative_quantity']}((?:\s*,\s*{$patterns['possibly_negative_quantity']}){0,2}\s*\))/i";
135131
// @codingStandardsIgnoreEnd
132+
133+
self::$patterns = $patterns;
136134
}
137135

136+
138137
/**
139138
* Transform an LTR stylesheet to RTL
139+
*
140140
* @param string $css Stylesheet to transform
141-
* @param array|bool $options Options array or value of transformDirInUrl option (back-compat)
142-
* @param bool $options['transformDirInUrl'] Transform directions in URLs (ltr/rtl). Default: false.
143-
* @param bool $options['transformEdgeInUrl'] Transform edges in URLs (left/right). Default: false.
141+
* @param bool|array{transformDirInUrl?:bool,transformEdgeInUrl?:bool} $options Options array,
142+
* or value of transformDirInUrl option (back-compat)
143+
* - transformDirInUrl: Transform directions in URLs (ltr/rtl). Default: false.
144+
* - transformEdgeInUrl: Transform edges in URLs (left/right). Default: false.
144145
* @param bool $transformEdgeInUrl [optional] For back-compat
145146
* @return string Transformed stylesheet
146147
*/
@@ -158,13 +159,13 @@ public static function transform($css, $options = array(), $transformEdgeInUrl =
158159
'transformEdgeInUrl' => false,
159160
);
160161

162+
self::buildPatterns();
163+
161164
// We wrap tokens in ` , not ~ like the original implementation does.
162165
// This was done because ` is not a legal character in CSS and can only
163166
// occur in URLs, where we escape it to %60 before inserting our tokens.
164167
$css = str_replace('`', '%60', $css);
165168

166-
self::buildPatterns();
167-
168169
// Tokenize single line rules with /* @noflip */
169170
$noFlipSingle = new CSSJanusTokenizer(self::$patterns['noflip_single'], '`NOFLIP_SINGLE`');
170171
$css = $noFlipSingle->tokenize($css);

‎test/suites/CSSJanusTest.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,9 @@ protected static function getSpec() {
6464
$file = "$dir/data-v$version.json";
6565
if (!is_readable($file)) {
6666
array_map('unlink', glob("$dir/data-v*.json"));
67-
$json = file_get_contents("https://github.com/cssjanus/cssjanus/raw/v$version/test/data.json");
67+
$json = file_get_contents(
68+
"https://raw.githubusercontent.com/cssjanus/cssjanus/v$version/test/data.json"
69+
);
6870
if ($json === false) {
6971
throw new Exception('Failed to fetch data');
7072
}

0 commit comments

Comments
 (0)