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

fix: PHPCS errors and warnings #2273

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

fix: PHPCS errors and warnings #2273

wants to merge 8 commits into from

Conversation

mrabbani
Copy link
Member

@mrabbani mrabbani commented May 13, 2024

All Submissions:

  • My code follow the WordPress' coding standards
  • My code satisfies feature requirements
  • My code is tested
  • My code passes the PHPCS tests
  • My code has proper inline documentation
  • I've included related pull request(s) (optional)
  • I've included developer documentation (optional)
  • I've added proper labels to this pull request

Changes proposed in this Pull Request:

Related Pull Request(s)

  • Full PR Link

Closes

  • Closes #

How to test the changes in this Pull Request:

  • Steps or issue link

Changelog entry

Title

Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.

Before Changes

Describe the issue before changes with screenshots(s).

After Changes

Describe the issue after changes with screenshot(s).

Feature Video (optional)

Link of detailed video if this PR is for a feature.

PR Self Review Checklist:

  • Code is not following code style guidelines
  • Bad naming: make sure you would understand your code if you read it a few months from now.
  • KISS: Keep it simple, Sweetie (not stupid!).
  • DRY: Don't Repeat Yourself.
  • Code that is not readable: too many nested 'if's are a bad sign.
  • Performance issues
  • Complicated constructions that need refactoring or comments: code should almost always be self-explanatory.
  • Grammar errors.

FOR PR REVIEWER ONLY:

As a reviewer, your feedback should be focused on the idea, not the person. Seek to understand, be respectful, and focus on constructive dialog.

As a contributor, your responsibility is to learn from suggestions and iterate your pull request should it be needed based on feedback. Seek to collaborate and produce the best possible contribution to the greater whole.

  • Correct — Does the change do what it’s supposed to? ie: code 100% fulfilling the requirements?
  • Secure — Would a nefarious party find some way to exploit this change? ie: everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities?
  • Readable — Will your future self be able to understand this change months down the road?
  • Elegant — Does the change fit aesthetically within the overall style and architecture?

Summary by CodeRabbit

  • New Features
    • Improved security and clarity in user input and output handling across various templates.
  • Bug Fixes
    • Enhanced sanitization practices to prevent potential XSS vulnerabilities in displayed content.
  • Refactor
    • Streamlined database query preparation and execution for better performance and security.
  • Style
    • Updated output functions across templates to improve formatting and security.
  • Documentation
    • Added comments for clarity and suppression of coding standard warnings in several files.

Copy link
Contributor

coderabbitai bot commented Aug 21, 2024

Walkthrough

This update enhances security, readability, and maintainability across various parts of the codebase. Key changes include improved sanitization of user inputs and outputs, clearer parameter naming, and better handling of SQL queries. These modifications collectively strengthen the code's integrity while ensuring its functionality remains intact.

Changes

File Change Summary
deprecated/deprecated-functions.php Renamed parameter $function to $function_name in dokan_doing_it_wrong for clarity; no functional changes made.
includes/functions.php Added comments to suppress PHPCS warnings for direct database queries, enhancing documentation without altering functionality; improved SQL query preparations for security.
templates/account/update-customer-to-vendor.php Improved security by sanitizing phone number input using dokan_sanitize_phone_number.
templates/account/vendor-registration.php Enhanced security by replacing translation functions with escaped equivalents to prevent XSS vulnerabilities.
templates/global/seller-registration-form.php Replaced __() with esc_html__() for security in terms and conditions message.
templates/store-lists-loop.php Enhanced security in seller rating display by improving output formatting and escaping.
templates/widgets/store-open-close.php Updated output functions for store hours to improve performance and security.
templates/orders/listing.php Improved sanitization for order-related strings and utilized safer output functions; enhanced security against XSS.
templates/withdraw/withdraw-dashboard.php Enhanced security and sanitization of displayed values related to withdrawal limits.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant System
    participant Database

    User->>System: Request to view seller rating
    System->>Database: Fetch seller rating data
    Database-->>System: Return seller rating
    System->>User: Display sanitized seller rating
Loading
sequenceDiagram
    participant User
    participant System

    User->>System: Initiate withdrawal process
    System->>User: Display withdrawal dashboard
    User->>System: Request withdrawal
    System->>User: Show withdrawal limits and thresholds
    System->>User: Display formatted monetary values
Loading

Poem

🐇✨
In code we hop, with changes bright,
To make our functions safe and right.
From queries deep to output clear,
We dance with joy, no bugs near.
With every line, our spirits soar,
A cleaner path, we all adore!
🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a636976 and cf27bff.

Files selected for processing (15)
  • deprecated/deprecated-functions.php (1 hunks)
  • includes/Order/Hooks.php (6 hunks)
  • includes/Order/functions.php (5 hunks)
  • includes/functions.php (11 hunks)
  • includes/wc-functions.php (5 hunks)
  • templates/account/update-customer-to-vendor.php (2 hunks)
  • templates/account/vendor-registration.php (2 hunks)
  • templates/global/seller-registration-form.php (1 hunks)
  • templates/orders/listing.php (4 hunks)
  • templates/orders/order-download-permission-html.php (1 hunks)
  • templates/reverse-withdrawal/reverse-balance.php (1 hunks)
  • templates/store-lists-loop.php (1 hunks)
  • templates/widgets/store-open-close.php (2 hunks)
  • templates/widgets/widget-content-product.php (1 hunks)
  • templates/withdraw/withdraw-dashboard.php (1 hunks)
Files skipped from review due to trivial changes (5)
  • deprecated/deprecated-functions.php
  • includes/Order/Hooks.php
  • includes/Order/functions.php
  • templates/orders/order-download-permission-html.php
  • templates/widgets/widget-content-product.php
Additional comments not posted (36)
templates/reverse-withdrawal/reverse-balance.php (4)

16-16: Centralize formatting logic for monetary values.

The introduction of $formatted_price centralizes the formatting logic, improving readability and maintainability. This change is beneficial for consistent formatting across the code.


21-23: Ensure safe output with wp_kses_post().

Using wp_kses_post() to sanitize the output ensures that the displayed content is safe from XSS vulnerabilities. This is a good practice for handling user-generated content.


30-30: Sanitize threshold value with wp_kses_post().

Using wp_kses_post() for the threshold value ensures that the output is properly sanitized, enhancing security.


32-37: Sanitize payable amount with wp_kses_post().

The introduction of $formatted_payable_amount and its sanitization with wp_kses_post() ensures that the displayed amount is secure and properly formatted.

templates/widgets/store-open-close.php (3)

14-18: Use printf() for direct output.

Replacing echo sprintf() with printf() reduces the need for additional output operations, potentially improving performance.


18-18: Enhance security with esc_html__().

Using esc_html__() instead of __() ensures that the output is properly escaped for HTML, mitigating XSS vulnerabilities.


46-54: Use printf() for formatted output.

The use of printf() for directly outputting formatted strings is consistent with best practices for performance and readability.

templates/global/seller-registration-form.php (1)

67-67: Enhance security with esc_html__().

Replacing __() with esc_html__() ensures that the output is properly escaped for HTML, enhancing security by preventing XSS vulnerabilities.

templates/account/update-customer-to-vendor.php (2)

43-44: Sanitize store URL with esc_url().

Introducing $store_url and sanitizing it with esc_url() before outputting ensures that the URL is safe from XSS vulnerabilities.


73-73: Secure output with esc_html__() and wp_kses_post().

Using esc_html__() and wp_kses_post() in printf() ensures that the output is both translated and sanitized, preventing XSS vulnerabilities.

templates/account/vendor-registration.php (2)

31-31: Enhance security with esc_html__().

Replacing __() with esc_html__() ensures that the output is properly escaped for HTML, enhancing security by preventing XSS vulnerabilities.


92-92: Secure output with esc_html__().

Using esc_html__() ensures that the output is properly escaped for HTML, enhancing security by preventing XSS vulnerabilities.

templates/withdraw/withdraw-dashboard.php (2)

26-26: Good use of wp_kses_post() for sanitizing HTML output.

Wrapping wc_price() with wp_kses_post() ensures that any HTML is properly sanitized, preventing XSS vulnerabilities. This is a good practice for displaying dynamic content.


34-38: Proper use of esc_html() and sprintf() for secure output.

The use of esc_html() around sprintf() ensures that the formatted string is safely escaped for HTML output. This change enhances security by preventing XSS vulnerabilities.

templates/store-lists-loop.php (1)

56-58: Improved security with printf() and esc_html().

Replacing echo sprintf() with printf() and wrapping number_format_i18n() with esc_html() ensures that the output is properly escaped, enhancing security by preventing XSS vulnerabilities.

templates/orders/listing.php (7)

64-64: Secure output with esc_html__() and esc_attr().

Using esc_html__() for translation and esc_attr() for escaping the order number ensures that the output is safe from XSS vulnerabilities. This is a good practice for handling dynamic content.


70-70: Consistent use of esc_html__() and esc_attr().

The use of esc_html__() for translation and esc_attr() for escaping the order number maintains consistency and security in the code, preventing XSS vulnerabilities.


78-78: Proper use of wp_kses_post() for safe HTML output.

Wrapping the order total with wp_kses_post() allows for safe HTML output while stripping potentially harmful tags, enhancing security.


84-84: Secure rendering of order status with esc_html().

Using esc_html() to escape the order status ensures that the output is safe from XSS vulnerabilities, which is a good security practice.


117-117: Safe HTML output with wp_kses_post().

Using wp_kses_post() for the shipment status ensures that only safe HTML is rendered, enhancing security by preventing XSS vulnerabilities.


159-159: Secure use of printf() with esc_url() and esc_attr().

Using printf() with esc_url() and esc_attr() ensures that URLs and attributes are properly escaped, preventing XSS vulnerabilities. This is a good practice for handling dynamic content.


182-182: Safe pagination output with wp_kses_post().

Using wp_kses_post() for pagination links ensures that only safe HTML is rendered, enhancing security by preventing XSS vulnerabilities.

includes/wc-functions.php (6)

442-443: Improved readability with separate query preparation.

Storing the SQL statement in a separate $permission_query variable before executing it enhances readability and maintains a clearer separation of concerns in the code.


463-463: PHPCS comment for direct database query.

The PHPCS comment acknowledges the direct database query and suppresses warnings. Ensure that this is necessary and that the query is safe from SQL injection.


484-484: Consistent return type declaration syntax.

Removing the space before the colon in the return type declaration aligns with PHP's type declaration standards, improving code consistency.


488-488: PHPCS comment for direct database query.

The PHPCS comment acknowledges the direct database query and suppresses warnings. Ensure that this is necessary and that the query is safe from SQL injection.


669-669: Consistent return type declaration syntax.

Removing the space before the colon in the return type declaration aligns with PHP's type declaration standards, improving code consistency.


453-453: PHPCS comment for direct database query.

The PHPCS comment acknowledges the direct database query and suppresses warnings. Ensure that this is necessary and that the query is safe from SQL injection.

Run the following script to verify the safety of the query:

Verification successful

Direct Database Query Safety Verification

The function dokan_process_product_file_download_paths includes direct database queries that are suppressed with PHPCS comments. Here are the key points regarding the safety of these queries:

  • The function uses $wpdb->prepare() for query preparation, which is a good practice to prevent SQL injection.
  • The inputs to the queries ($product_id, dokan_get_prop($order, 'id'), and $download_id) are expected to be integers or strings that are sanitized elsewhere in the code.

However, ensure that:

  • The variables used in the queries are properly validated and sanitized before being passed to $wpdb->prepare().
  • The function is not deprecated, as indicated by wc_deprecated_function.

Code Locations to Review:

  • includes/wc-functions.php: Lines involving $wpdb->prepare() and $wpdb->query() within dokan_process_product_file_download_paths.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the safety of direct database queries in dokan_process_product_file_download_paths.

# Test: Search for the function implementation. Expect: Proper sanitization and validation of inputs.
ast-grep --lang php --pattern $'function dokan_process_product_file_download_paths($_, $_, $_) {
  $$$
}'

Length of output: 106


Script:

#!/bin/bash
# Search for any function with a similar name to dokan_process_product_file_download_paths
rg --php --pcre2 --multiline --multiline-dotall 'function\s+dokan_process_product_file_download_paths\s*\(.*\)\s*\{' -A 20

Length of output: 532


Script:

#!/bin/bash
# Corrected search for any function with a similar name to dokan_process_product_file_download_paths
rg --pcre2 --multiline --multiline-dotall 'function\s+dokan_process_product_file_download_paths\s*\(.*\)\s*\{' -A 20

Length of output: 37088

includes/functions.php (8)

Line range hint 288-310: LGTM! Enhanced SQL query preparation.

The use of placeholders for excluded product types improves security by preventing SQL injection. The changes are well-implemented.


351-351: Direct SQL query with PHPCS ignore.

The direct SQL query is accompanied by a PHPCS ignore comment, indicating a conscious decision to bypass coding standards for this query. The query uses placeholders, which is good practice.


411-411: Direct SQL query with PHPCS ignore.

The direct SQL query is accompanied by a PHPCS ignore comment, indicating a conscious decision to bypass coding standards for this query. The query uses placeholders, which is good practice.


1105-1105: Use of list() for destructuring.

The use of list() for destructuring aligns with coding standards and improves clarity. This change is consistent with best practices.


1204-1204: Direct use of $_SERVER with PHPCS ignore.

The use of $_SERVER['QUERY_STRING'] is accompanied by a PHPCS ignore comment, indicating a conscious decision to bypass coding standards for this line. This is acceptable in this context.


1486-1486: Correct logic for percentage calculation.

The logic for calculating percentage changes is correct, and the use of round() ensures the percentage is formatted to two decimal places.


Line range hint 2318-2330: Direct SQL query with PHPCS ignore.

The direct SQL query is accompanied by a PHPCS ignore comment, indicating a conscious decision to bypass coding standards for this query. The query uses placeholders, and input is sanitized using wc_clean and wp_unslash, which is good practice.


3992-3992: Correct email masking logic.

The logic for masking the email address is correct, using explode and str_replace to replace characters with asterisks. This approach effectively hides sensitive parts of the email.

@mrabbani mrabbani self-assigned this Aug 21, 2024
@mrabbani mrabbani added Upcoming Release Needs: Testing This requires further testing Needs: Dev Review It requires a developer review and approval labels Aug 21, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cf27bff and 78af483.

Files selected for processing (6)
  • deprecated/deprecated-functions.php (1 hunks)
  • includes/functions.php (4 hunks)
  • templates/account/update-customer-to-vendor.php (1 hunks)
  • templates/account/vendor-registration.php (2 hunks)
  • templates/global/seller-registration-form.php (1 hunks)
  • templates/store-lists-loop.php (1 hunks)
Files skipped from review due to trivial changes (1)
  • deprecated/deprecated-functions.php
Files skipped from review as they are similar to previous changes (5)
  • includes/functions.php
  • templates/account/update-customer-to-vendor.php
  • templates/account/vendor-registration.php
  • templates/global/seller-registration-form.php
  • templates/store-lists-loop.php

@shohag121 shohag121 added 👍 Dev Review Done and removed Needs: Dev Review It requires a developer review and approval labels Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants