Review: Add validator spec via code generation #100 #221
Conversation
schlessera
force-pushed
the
review/amp-validator-spec
branch
from
21d3908
to
c1dc4b7
Compare
schlessera
force-pushed
the
review/amp-validator-spec
branch
from
c1dc4b7
to
c97b094
Compare
|
Regarding the TODOs for "class/method not found", these are caused by the lack of support for class maps in PhpStorm. The Nette PHP Generator package uses a class map as an autoloader, and PhpStorm doesn't include that in its index. This would be solvable by either generating a complex '.phpstorm.meta.php' file or by generating a stubs file to add to the autoloader, but I'm not sure it is worth the effort. As the TODOs caused PHPCS failures due to their length, I've removed those for now. |
|
As a side-effect of solving one of the code insight TODOs, this PR has now added magic getters for all types of entries, and all of them now include a class doc-block with all relevant properties. |
|
@westonruter I adapted to code and resolved all of your TODOs except for the three that relate to the handling of latest extension versions. I have created a new issue for these, so that we can merge this PR now: #232 |
| @@ -126,7 +126,7 @@ interface Tag | |||
| const NEXTID = 'nextid'; | |||
| const NOBR = 'nobr'; | |||
| const NOSCRIPT = 'noscript'; | |||
| const O_P = 'o:p'; | |||
| const O_P = 'o:p'; // @todo Will this be usable at present given PHP DOM? | |||
There was a problem hiding this comment.
I'd leave this TODO in for now. I haven't tested this yet, and I'm also not sure we'll even encounter that in the WP plugin anywhere.
| SpecRule::MANDATORY_ONEOF => '[\'amp-nested-submenu-close\', \'amp-nested-submenu-open\']', | ||
| SpecRule::MANDATORY_ONEOF => [ | ||
| Attribute::AMP_NESTED_SUBMENU_CLOSE, | ||
| Attribute::AMP_NESTED_SUBMENU_OPEN, | ||
| ], |
| SpecRule::NAME => Attribute::HREF, | ||
| Attribute::ASYNC => [], | ||
| Attribute::CROSSORIGIN => [], | ||
| Attribute::HREF => [ | ||
| SpecRule::MANDATORY => true, | ||
| SpecRule::VALUE_REGEX => 'https://cdn\.materialdesignicons\.com/([0-9]+\.?)+/css/materialdesignicons\.min\.css|https://cloud\.typography\.com/[0-9]*/[0-9]*/css/fonts\.css|https://fast\.fonts\.net/.*|https://fonts\.googleapis\.com/css2?\?.*|https://fonts\.googleapis\.com/icon\?.*|https://fonts\.googleapis\.com/earlyaccess/.*\.css|https://maxcdn\.bootstrapcdn\.com/font-awesome/([0-9]+\.?)+/css/font-awesome\.min\.css(\?.*)?|https://(use|pro|kit)\.fontawesome\.com/releases/v([0-9]+\.?)+/css/[0-9a-zA-Z-]+\.css|https://(use|pro|kit)\.fontawesome\.com/[0-9a-zA-Z-]+\.css|https://use\.typekit\.net/[\w\p{L}\p{N}_]+\.css', |
There was a problem hiding this comment.
Since this regex will be sent to preg_match(), we should escape the '/' delimiter. See ampproject/amp-wp@afd1172
There was a problem hiding this comment.
I wouldn't escape this against a random delimiter.
I would suggest using the chr(0) delimiter in the plugin if you want to avoid preg_quote().
There was a problem hiding this comment.
I'm merging this now to get rid of another huge PR, but we can discuss this particular issue separately.
There was a problem hiding this comment.
Interesting. I wasn't aware you could use a NUL as a delimiter.
There was a problem hiding this comment.
I've opened this as #238.
There was a problem hiding this comment.
Oh, sorry, I've remembered incorrectly, it was chr(1) that we have been using in WP-CLI, not chr(0). The chr(1) character is the "start of header" (SOH) character, intended to control the flow of streams, and shouldn't be in use or cause conflicts in HTML.
This delimiter has been used as the default delimiter in the WP-CLI search-replace command when you provide a regex via the CLI, and as far as I know, it has never caused any issues.
There was a problem hiding this comment.
Excellent. Sounds good to me.
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Follow-up on #100.