Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use coding standards consistently and switch to PSR across all repos #16

Open
5 tasks
Tracked by #14
SteelWagstaff opened this issue May 19, 2022 · 2 comments
Open
5 tasks
Tracked by #14

Comments

@SteelWagstaff
Copy link
Member

SteelWagstaff commented May 19, 2022

  • Tech team reviews Ricardo's coding-standards proposal: https://docs.google.com/document/d/1TcltiK7mamenA1lbahyFmiEKDt_pBTcn2ZovnFZ-Ro8/edit#heading=h.f3twoiy72t97 and finalizes any desired changes to the rule set in pressbooks/coding standards
  • Make changes to coding standards rule set and release new version of coding-standards (if needed)
  • Bump coding standards dependency in all repos (if needed) and remove all unneeded repo-level rule exceptions
  • Fix or document fixes needed to pass standards in each repo
  • Gradually refactor our plugins to be PSR compatible instead of WP/HumanMade

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

@SteelWagstaff SteelWagstaff changed the title Make changes to the ruleset in each of our repos, as needed Use coding standards consistently across all repos Jun 9, 2022
@fdalcin
Copy link

fdalcin commented Jul 26, 2022

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 1050

Digging 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.

@fdalcin
Copy link

fdalcin commented Nov 21, 2022

Question

Now that we're using php 8 could we replace Coding Standards with Laravel Pint?

@arzola arzola changed the title Use coding standards consistently across all repos Use coding standards consistently and switch to PSR across all repos Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants