fdalcin
commented
2 years ago Update
There's a draft PR where we're defining new rules we want to use/ignore. However, there is a rule we disabled temporarily because it's breaking on PHP 8.
<!-- Disabled rule because it's breaking on PHP 8 -->
<exclude name="Pressbooks.Security.EscapeOutput.OutputNotEscaped" />When this rule is applied we get the following error when running coding standards
Fatal error: Uncaught TypeError: vsprintf(): Argument #2 ($values) must be of type array, string given in /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Files/File.php:1050
Stack trace:
#0 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Files/File.php(1050): vsprintf('All output shou...', '$display')
#1 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Files/File.php(670): PHP_CodeSniffer\Files\File->addMessage(true, 'All output shou...', 260, 56, 'OutputNotEscape...', '$display', 5, false)
#2 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/Security/EscapeOutputSniff.php(456): PHP_CodeSniffer\Files\File->addError('All output shou...', 1919, 'OutputNotEscape...', '$display')
#3 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/wp-coding-standards/wpcs/WordPress/Sniff.php(910): WordPressCS\WordPress\Sniffs\Security\EscapeOutputSniff->process_token(1914)
#4 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Files/File.php(496): WordPressCS\WordPress\Sniff->process(Object(PHP_CodeSniffer\Files\LocalFile), 1913)
#5 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(91): PHP_CodeSniffer\Files\File->process()
#6 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Runner.php(630): PHP_CodeSniffer\Files\LocalFile->process()
#7 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Runner.php(434): PHP_CodeSniffer\Runner->processFile(Object(PHP_CodeSniffer\Files\LocalFile))
#8 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run()
#9 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS()
#10 /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/bin/phpcs(120): include('/srv/www/pressb...')
#11 {main}
thrown in /srv/www/pressbooks.test/current/web/app/plugins/pressbooks-vip/vendor/squizlabs/php_codesniffer/src/Files/File.php on line 1050Digging a bit further I noticed it happens when we output using the HEREDOC syntax
// This breaks
echo <<<HTML
<style>div#privacy { display: none; }</style>
HTML;
// This works
echo '<style>div { display: none; }</style>';Replacing simple HEREDOC would be a solution in this case, however when we output complex content it fails for everything:
// Replacing content with printf or sprintf functions
$display = 'none';
echo sprintf( '<style>div { display: %s; }</style>', $display );
printf( '<style>div { display: %s; }</style>', $display );
// Using double quotes with variables
echo "<style>div { display: $display; </style>";
// Concatenation
echo '<style>div { display: ' . $display . '; }</style>';It looks lik the issue was solved upstream here (not sure it's released though), but even if this is already released we don't have access to updates because humanmade/coding-standards has a fixed version for this dependency, see https://github.com/humanmade/coding-standards/blob/master/composer.json#L8.
Goal: Review all repos. Make sure that they inherit our coding standards and general ruleset to the greatest degree possible. Only override ruleset at individual repo when absolutely needed, and with clear plan to remove/replace over time. For example, these overrides should happen in coding-standards instead of pressbooks/pressbooks if they're overrides that the team agrees upon: https://github.com/pressbooks/pressbooks/blob/e341b3a92838f061bc8e77c1a2c690d98b1e84b5/phpcs.ruleset.xml#L3-L10