[CMSP-353] Add pantheon-wp-coding-standards #400
Conversation
|
Interesting to note that behat fell down where phpunit did not. Is that a case we should cover with unit testing? |
jazzsequence
changed the title
@pwtyler I think there are legit issues that were introduced by some of the fixes for PHPCS sniffs. I'm trying to put stuff back the way it was to see if it can get through Behat and then be more intentional about what I'm changing. But I do think that if things are failing, probably unit tests should cover those unless they are legitimately not able to be covered by functional tests. |
jazzsequence
force-pushed
the
add-pantheon-wp-coding-standards
branch
from
05de5ef to
1fc2b66
Compare
|
Nevermind, it's the upstream tests that are failing, not the Behat tests running from the plugin. 🤦 |
so we aren't potentially waiting for other things later
jazzsequence
changed the title
|
Will hold review until #406 is updated with same behat fixes and this PR is rebased. |
|
@pwtyler This PR removes the upstream behat tests (although they're still loaded as a repository, we just aren't running them). I think while pantheon-systems/pantheon-wordpress-upstream-tests#62 is still WIP we can go ahead and merge this and move forward and re-add the upstream tests as those are fixed. (actually those were removed in @jspellman814's update) |
jazzsequence
changed the title
* add our coding standards * use php 8 for linting * phpcbf fixes * ignore tests for linting * adjust and fix sniffs for object-cache.php drop-in * check if all the server global values * fix sniffs for WP CLI commands * do a composer update before install for 7.4 * make the $cache property public * fix composer update & install in both 7.4 tests * set $cache_hits property to true * set redis_calls property to public * set is_redis_connected property to public * revert wp_redis_ignore_global_groups const value * make all the private and protected properties public * use short arrays again * remove parenthesis when we instantiate the class * set object-cache back to what it was * remove extra isset checks * add the issets back to the database param * add original object-cache file back * use the version of the upstream tests that maybe have the issues fixed * update the upstream tests * Revert "add original object-cache file back" This reverts commit 1fc2b66. * restore cli.php and wp-redis.php back to what they were * remove the upstream behat tests these are failing because they're old and need to be updated. they have nothing to do with the plugin itself * add some echos * wordpress isn't actually getting installed break the brackets * one line, brackets and remove the & * guess we can't do this on one line * don't need that semicolon i don't think * okay fine, put it back the way it was * enable redis before flushing cache * enable redis on the entire site (not env) and add a wait command * enable redis early so we aren't potentially waiting for other things later * don't need to wait * don't need to enable because it's already enabled * don't need an isset, we already know it's set * use strip all tags instead of sanitize_text_field * don't activate redis twice and flush cache after wp-redis is active * re-add composer.lock * re-add composer update for 7.4
* add our coding standards * use php 8 for linting * phpcbf fixes * ignore tests for linting * adjust and fix sniffs for object-cache.php drop-in * check if all the server global values * fix sniffs for WP CLI commands * do a composer update before install for 7.4 * make the $cache property public * fix composer update & install in both 7.4 tests * set $cache_hits property to true * set redis_calls property to public * set is_redis_connected property to public * revert wp_redis_ignore_global_groups const value * make all the private and protected properties public * use short arrays again * remove parenthesis when we instantiate the class * set object-cache back to what it was * remove extra isset checks * add the issets back to the database param * add original object-cache file back * use the version of the upstream tests that maybe have the issues fixed * update the upstream tests * Revert "add original object-cache file back" This reverts commit 1fc2b66. * restore cli.php and wp-redis.php back to what they were * remove the upstream behat tests these are failing because they're old and need to be updated. they have nothing to do with the plugin itself * add some echos * wordpress isn't actually getting installed break the brackets * one line, brackets and remove the & * guess we can't do this on one line * don't need that semicolon i don't think * okay fine, put it back the way it was * enable redis before flushing cache * enable redis on the entire site (not env) and add a wait command * enable redis early so we aren't potentially waiting for other things later * don't need to wait * don't need to enable because it's already enabled * don't need an isset, we already know it's set * use strip all tags instead of sanitize_text_field * don't activate redis twice and flush cache after wp-redis is active * re-add composer.lock * re-add composer update for 7.4
| $redis_server = [ | ||
| 'host' => wp_strip_all_tags( $_SERVER['CACHE_HOST'] ), | ||
| 'port' => isset( $_SERVER['CACHE_PORT'] ) ? wp_strip_all_tags( $_SERVER['CACHE_PORT'] ) : 0, | ||
| 'auth' => isset( $_SERVER['CACHE_PASSWORD'] ) ? wp_strip_all_tags( $_SERVER['CACHE_PASSWORD'] ) : '', |
There was a problem hiding this comment.
@jazzsequence FYI, just noting that this line is incorrect and the default value should have been null as I had indicated in my PR to fixed this. Also, the default port should not be 0 as that is completely invalid for Redis and should be 6379.
There was a problem hiding this comment.
@jazzsequence noting also that changes should have fixed the issues called out in #359 but in fact broke our patch which we will now have to redo.
There was a problem hiding this comment.
These have been updated in #428. Thanks for digging this up.
| 'host' => sanitize_text_field( $_SERVER['CACHE_HOST'] ), | ||
| 'port' => sanitize_text_field( $_SERVER['CACHE_PORT'] ), | ||
| 'auth' => sanitize_text_field( $_SERVER['CACHE_PASSWORD'] ), | ||
| 'database' => sanitize_text_field( $_SERVER['CACHE_DB'] ), |
There was a problem hiding this comment.
@jazzsequence this was also incorrectly changed to no longer be providing a default value.
There was a problem hiding this comment.
The CACHE_DB value is checked on line 41 -- which checks if ALL of these values are set. If any of them are not set, it uses the default values on lines 49-53
Lines 49 to 53 in 1541959
* Fixes pantheon-systems#359 * Falls back on port 6379 if the CACHE_PORT is not configured. * Doesn't require a CACHE_PASSWORD to be set when it isn't used, or can't be. * Improves code quality by reducing Redis default port & databasei duplicate values to a share variables. * Fixes invalid code changes made in PR [pantheon-systems#400 ](pantheon-systems#400) to the core plugin connectivity testing that prevent connectivty checks if the port/password/database aren't explicitly defined.
* fix: Fixes assumption that CACHE_PORT & CACHE_PASSWORD are Set. * Fixes #359 * Falls back on port 6379 if the CACHE_PORT is not configured. * Doesn't require a CACHE_PASSWORD to be set when it isn't used, or can't be. * Improves code quality by reducing Redis default port & databasei duplicate values to a share variables. * Fixes invalid code changes made in PR [#400 ](#400) to the core plugin connectivity testing that prevent connectivty checks if the port/password/database aren't explicitly defined. * update changelog --------- Co-authored-by: Chris Reynolds <chris@jazzsequence.com>
* fix: Fixes assumption that CACHE_PORT & CACHE_PASSWORD are Set. * Fixes #359 * Falls back on port 6379 if the CACHE_PORT is not configured. * Doesn't require a CACHE_PASSWORD to be set when it isn't used, or can't be. * Improves code quality by reducing Redis default port & databasei duplicate values to a share variables. * Fixes invalid code changes made in PR [#400 ](#400) to the core plugin connectivity testing that prevent connectivty checks if the port/password/database aren't explicitly defined. * update changelog --------- Co-authored-by: Chris Reynolds <chris@jazzsequence.com>
Adds our coding standards and fixes sniffs.
Also updates the circle lint workflow to use PHP 8