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

PHPCS warning and error with refactoring #158

Merged
merged 8 commits into from Oct 19, 2018

Conversation

Projects
None yet
3 participants
@vaishaliagola27
Copy link
Collaborator

commented Oct 12, 2018

This PR contains below changes.

  • Refactor code changes
  • PHPCS issues and warnings resolving
  • Missing code from master added

Below are some issues which need debugging.

  • Redis cache flush on post update/ deletes not working.
  • Timestamp shows latest time - Needs to check it's functionality or issue.
  • Cache not found while deleting home page cache
    Purging homepage http://nhtest.h1.rtdemo.in/ 2018-10-15 07:43:32 | ERROR | - Cache Not Found | http://nhtest.h1.rtdemo.in/

@vaishaliagola27 vaishaliagola27 requested a review from rahulsprajapati Oct 12, 2018

@vaishaliagola27 vaishaliagola27 changed the title PHPCS warning and error with refactoring WIP: PHPCS warning and error with refactoring Oct 15, 2018

@abhijitrakas

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Found following two issues in current stable version.

  • Redis cache flush, on post update/ deletes not working.
  • Timestamp shows latest time - Needs to check it's functionality or issue.

@rahulsprajapati rahulsprajapati changed the title WIP: PHPCS warning and error with refactoring PHPCS warning and error with refactoring Oct 16, 2018

$purge_urls = isset( $nginx_helper_admin->options['purge_url'] ) && ! empty( $nginx_helper_admin->options['purge_url'] ) ?
explode( "\r\n", $nginx_helper_admin->options['purge_url'] ) : array();
// Allow plugins/themes to modify/extend urls. Pass urls array in first parameter, second says if wildcards are allowed
// Allow plugins/themes to modify/extend urls. Pass urls array in first parameter, second says if wildcards are allowed.

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Update this comment formate.

Also check where this 2nd value is useful because it seems we are always adding wildcard as false and apply_filter is not going to update it as it's 2nd paramter. Maybe we need to detect from somewhere that wildcard in url is supported or not.

Suggested change
// Allow plugins/themes to modify/extend urls. Pass urls array in first parameter, second says if wildcards are allowed.
/**
* Allow plugins/themes to modify/extend urls.
*
* @param array $purge_urls urls which needs to be purged.
* @param bool $wildcard If wildcard in url is allowed or not. default false.
*/

This comment has been minimized.

Copy link
@vaishaliagola27

vaishaliagola27 Oct 18, 2018

Author Collaborator

This filter is there in other places also, where wildcard is true.

}
/**
* Register the stylesheets for the admin area.
*
* @since 2.0.0
*
* @param string $hook Hook.

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Please check the variable what its used for, and then add comment don't just put a name like this. It's not hook.

Suggested change
* @param string $hook Hook.
* @param string $hook The current admin page.

ref: https://github.com/WordPress/WordPress/blob/8efab48edeb348db9399410b5ede28785ad25357/wp-admin/admin-header.php#L97

}
/**
* Register the JavaScript for the admin area.
*
* @since 2.0.0
*
* @param string $hook Hook.

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Same for this as above.

foreach ( $rss_items as $item ) {
?>
<li role="listitem">
<a href='<?php echo esc_url( $item->get_permalink() ); ?>' title='<?php echo esc_attr_e( 'Posted ', 'nginx-helper' ) . esc_attr( $item->get_date( 'j F Y | g:i a' ) ); ?>'>

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

why not use printf or sprintf ?

Show resolved Hide resolved admin/class-nginx-helper-admin.php
esc_url( $nginx_setting_link )
);
printf(
wp_kses( '%1$s (<a target="_blank" href="%2$s" title="%3$s">%4$s</a>)', array( 'strong' => array(), 'a' => array( 'href' => array(), 'title' => array() ) ) ),

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

I think escaping o/p of html code is make more sense her, not it's template/skeleton. Use wp_kses on sprintf o/p .

@@ -126,12 +156,18 @@
<input type="radio" value="get_request" id="purge_method_get_request" name="purge_method" <?php checked( $nginx_helper_settings['purge_method'], 'get_request' ); ?>>
&nbsp;
<?php
_e( 'Using a GET request to <strong>PURGE/url</strong> (Default option)', 'nginx-helper' );
printf(
wp_kses( '%1$s <strong>PURGE/url</strong> %2$s', array( 'strong' => array() ) ),

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Same for this as above. Escape final o/p.

?>
<br />
<small>
<?php
_e( 'Uses the <strong><a href="https://github.com/FRiCKLE/ngx_cache_purge">ngx_cache_purge</a></strong> module. ', 'nginx-helper' );
printf(
wp_kses( '%1$s <strong><a href="https://github.com/FRiCKLE/ngx_cache_purge">ngx_cache_purge</a></strong> %2$s.', array( 'strong' => array(), 'a' => array( 'href' => array() ) ) ),

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Same for this as above. Escape final o/p.

@@ -145,7 +181,10 @@
<br />
<small>
<?php
_e( 'Checks for matching cache file in <strong>RT_WP_NGINX_HELPER_CACHE_PATH</strong>. Does not require any other modules. Requires that the cache be stored on the same server as WordPress. You must also be using the default nginx cache options (levels=1:2) and (fastcgi_cache_key "$scheme$request_method$host$request_uri"). ', 'nginx-helper' );
printf(
wp_kses( '%1$s<strong>RT_WP_NGINX_HELPER_CACHE_PATH</strong>. %2$s', array( 'strong' => array(), ) ),

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Same for this as above. Escape final o/p.

@@ -206,7 +272,10 @@
<input type="checkbox" value="1" id="purge_homepage_on_edit" name="purge_homepage_on_edit" <?php checked( $nginx_helper_settings['purge_homepage_on_edit'], 1 ); ?> />
&nbsp;
<?php
_e( 'when a <strong>post</strong> (or page/custom post) is <strong>modified</strong> or <strong>added</strong>.', 'nginx-helper' );
printf(
wp_kses( '%1$s<strong>%2$s</strong>%3$s<strong>%4$s</strong>%5$s<strong>%6$s</strong>.', array( 'strong' => array(), ) ),

This comment has been minimized.

Copy link
@rahulsprajapati

rahulsprajapati Oct 17, 2018

Member

Same for this as above. Escape final o/p.

Check for other places and update it.

@rahulsprajapati

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@vaishaliagola27 Why did you resolve all comments before even pushing the changes?

@rahulsprajapati rahulsprajapati merged commit 4442fc6 into rtCamp:v2 Oct 19, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
WIP ready for review
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.