2.0 code cleanup #456
2.0 code cleanup #456
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
WalkthroughThis pull request introduces a series of changes aimed at enhancing security, improving code maintainability, and refining user interface descriptions. The modifications include nonce verification, data sanitization, removal of unused classes and functions, and updates to block labels and descriptions. Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (28)
- includes/classes/admin/class-admin.php (1)
- includes/classes/admin/class-settings.php (4)
- includes/classes/blocks/class-bindings.php (2)
- includes/classes/blocks/class-patterns.php (2)
- includes/classes/blocks/class-registration.php (1)
- includes/classes/class-metaboxes.php (1)
- includes/classes/class-share.php (1)
- includes/classes/legacy/class-accommodation.php (2)
- includes/classes/legacy/class-frontend.php (3)
- includes/classes/legacy/class-maps.php (2)
- includes/classes/legacy/class-scpo-engine.php (1)
- includes/classes/legacy/class-tour-operator.php (2)
- includes/classes/legacy/class-tour.php (2)
- includes/partials/help.php (1)
- includes/patterns/accommodation-card.php (1)
- includes/patterns/destination-card.php (1)
- includes/patterns/gallery.php (1)
- includes/patterns/itinerary-list.php (1)
- includes/patterns/room-card.php (1)
- includes/patterns/tour-card.php (1)
- includes/template-tags/accommodation.php (1)
- includes/template-tags/addons.php (1)
- includes/template-tags/destination.php (1)
- includes/template-tags/general.php (1)
- includes/template-tags/helpers.php (2)
- includes/template-tags/tour.php (1)
- includes/template-tags/videos.php (1)
- tour-operator-bootstrap.php (1)
Files ignored due to filter (2)
- assets/img/placeholders/placeholder-post-750x350 copy.jpg
- vendor/cmb2/includes/CMB2.php
Files not summarized due to errors (6)
- includes/classes/legacy/class-scpo-engine.php (diff tokens exceeds limit)
- includes/template-tags/accommodation.php (diff tokens exceeds limit)
- includes/template-tags/destination.php (diff tokens exceeds limit)
- includes/template-tags/general.php (diff tokens exceeds limit)
- includes/template-tags/helpers.php (diff tokens exceeds limit)
- includes/template-tags/tour.php (diff tokens exceeds limit)
Files not reviewed due to errors (9)
- includes/classes/admin/class-admin.php (no response)
- includes/classes/legacy/class-accommodation.php (no response)
- includes/classes/class-share.php (no response)
- includes/classes/legacy/class-scpo-engine.php (no response)
- includes/patterns/tour-card.php (no response)
- includes/template-tags/accommodation.php (no response)
- includes/template-tags/destination.php (no response)
- includes/template-tags/helpers.php (no response)
- includes/template-tags/tour.php (no response)
Files skipped from review due to trivial changes (8)
- includes/classes/blocks/class-patterns.php
- includes/classes/class-metaboxes.php
- includes/classes/legacy/class-maps.php
- includes/patterns/accommodation-card.php
- includes/patterns/gallery.php
- includes/patterns/itinerary-list.php
- includes/patterns/room-card.php
- includes/template-tags/general.php (diff too large)
Review comments generated (16)
- Review: 10
- LGTM: 6
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -474,7 +474,7 @@ public function build_unit_field( $build = '', $field = '', $count = 1 ) { | |||
|
|
|||
| switch ( $field ) { | |||
| case 'title': | |||
| $value = strip_tags( $rooms->item_title( '', '', false ) ); | |||
| $value = wp_strip_all_tags( $rooms->item_title( '', '', false ) ); | |||
There was a problem hiding this comment.
Good job replacing strip_tags with wp_strip_all_tags. The latter is more secure as it removes all HTML tags including script and style. This change enhances the security of your code.
- $value = strip_tags( $rooms->item_title( '', '', false ) );
+ $value = wp_strip_all_tags( $rooms->item_title( '', '', false ) );| @@ -432,6 +432,7 @@ protected function post_ids_exist( $ids ) { | |||
| AND ID IN (%s) | |||
| AND post_status IN ('draft', 'publish')"; | |||
|
|
|||
| // @phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared | |||
There was a problem hiding this comment.
The use of @phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared is a red flag. It's generally not a good practice to ignore warnings from static analysis tools like PHP CodeSniffer, especially when it comes to database queries where SQL injection vulnerabilities can occur.
Instead of ignoring the warning, you should properly prepare the SQL statement. The $wpdb->prepare() function should be used with placeholders for all variables in the query to prevent SQL injection attacks. However, since $ids is an array and IN clause is used in the SQL, you need to generate placeholders for each item in the array.
Here's how you can do it:
- AND ID IN (%s)
+ AND ID IN (" . implode(',', array_fill(0, count($ids), '%s')) . ")
- // @phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
- return (int) $wpdb->get_var( $wpdb->prepare( $query, $ids ) );
+ return (int) $wpdb->get_var( $wpdb->prepare( $query, ...$ids ) );This way, each id in the $ids array will have its own placeholder, and the prepare method will correctly sanitize each id.
| add_filter( 'wpseo_breadcrumb_links', array( $this, 'wpseo_breadcrumb_links' ), 20 ); | ||
| } | ||
|
|
||
| /** | ||
| * Initate some boolean flags | ||
| */ | ||
| public function wp_head() { | ||
| if ( ( is_singular( $this->active_post_types ) || is_post_type_archive( $this->active_post_types ) ) | ||
| && false !== $this->options | ||
| && isset( $this->options['display']['enable_modals'] ) | ||
| && 'on' === $this->options['display']['enable_modals'] | ||
| ) { | ||
| $this->enable_modals = true; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * a filter to overwrite the links with modal tags. | ||
| */ | ||
| public function add_modal_attributes( $html, $post_id, $link ) { | ||
| if ( true === $this->enable_modals && true === $link ) { | ||
| $html = '<a data-toggle="modal" data-target="#lsx-modal-' . $post_id . '" href="#">' . get_the_title( $post_id ) . '</a>'; | ||
|
|
||
| if ( ! in_array( $post_id, $this->modal_ids ) ) { | ||
| $this->modal_ids[] = $post_id; | ||
| } | ||
| } | ||
|
|
||
| return $html; | ||
| } | ||
|
|
||
| /** | ||
| * a filter to overwrite the links with modal tags. | ||
| */ | ||
| public function output_modals() { | ||
| global $lsx_to_archive, $post; | ||
|
|
||
| if ( true === $this->enable_modals && ! empty( $this->modal_ids ) ) { | ||
| $temp = $lsx_to_archive; | ||
| $lsx_to_archive = 1; | ||
|
|
||
| foreach ( $this->modal_ids as $post_id ) { | ||
| $post = get_post( $post_id ); | ||
| ?> | ||
| <div class="lsx-modal modal fade" id="lsx-modal-<?php echo esc_attr( $post_id ); ?>" tabindex="-1" role="dialog"> | ||
| <div class="modal-dialog"> | ||
| <div class="modal-content"> | ||
| <button type="button" class="close" data-dismiss="modal">×</button> | ||
| <div class="modal-body"> | ||
| <?php lsx_to_content( 'content', 'modal' ); ?> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <?php | ||
| } | ||
|
|
||
| $lsx_to_archive = $temp; | ||
| wp_reset_postdata(); | ||
| } | ||
| ?> | ||
| <div class="lsx-modal modal fade" id="lsx-modal-placeholder" tabindex="-1" role="dialog"> | ||
| <div class="modal-dialog"> | ||
| <div class="modal-content"> | ||
| <button type="button" class="close" data-dismiss="modal">×</button> | ||
| <div class="modal-header"> | ||
| <h4 class="modal-title"></h4> | ||
| </div> | ||
| <div class="modal-body"></div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <?php | ||
| } | ||
|
|
||
| /** | ||
| * Register and enqueue admin-specific style sheet. | ||
| * |
There was a problem hiding this comment.
The removal of the wp_head function, add_modal_attributes, and output_modals functions indicates a significant change in functionality. The modal feature seems to have been removed. If this is intentional and all dependencies on these functions have been addressed, then this change is fine. However, if the modal feature is still required, you might need to reintroduce these functions or provide an alternative implementation.
| * Add a some classes so we can style. | ||
| */ | ||
| public function body_class( $classes ) { | ||
| global $post; | ||
| if ( false !== $this->post_types && is_singular( array_keys( $this->post_types ) ) ) { | ||
| $classes[] = 'single-tour-operator'; | ||
| } elseif ( false !== $this->post_types && is_post_type_archive( array_keys( $this->post_types ) ) ) { |
There was a problem hiding this comment.
The removal of the global $post; statement suggests that the $post global variable is no longer used in the body_class function. This is a good practice as it reduces the reliance on global state. However, ensure that there are no side effects due to this removal, especially if other methods in the class or hooks rely on this global variable being set here.
| // @phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
| if ( isset( $_GET['welcome-page'] ) ) { | ||
| // @phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
While the use of @phpcs:ignore WordPress.Security.NonceVerification.Recommended is understandable to suppress warnings from PHP CodeSniffer, it's not a good practice to ignore nonce verification. Nonce verification is crucial for security as it helps protect against certain types of misuse, malicious or otherwise. Consider adding nonce verification here.
- // @phpcs:ignore WordPress.Security.NonceVerification.Recommended
- if ( isset( $_GET['welcome-page'] ) ) {
- // @phpcs:ignore WordPress.Security.NonceVerification.Recommended
+ if ( isset( $_GET['welcome-page'] ) && check_admin_referer('welcome_nonce_action', 'welcome_nonce_field') ) {
$display_page = sanitize_text_field( $_GET['welcome-page'] );
$display_page = ! empty( $display_page ) ? $display_page : '';
}| // @phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
| if ( isset( $_POST[ $key ] ) ) { | ||
| $save = $_POST[ $key ]; | ||
| $save = sanitize_text_field( $_POST[ $key ] ); |
There was a problem hiding this comment.
Similar to the previous comment, ignoring nonce verification is not recommended. It's better to add nonce verification to enhance security.
- // @phpcs:ignore WordPress.Security.NonceVerification.Recommended
- if ( isset( $_POST[ $key ] ) ) {
+ if ( isset( $_POST[ $key ] ) && check_admin_referer('post_nonce_action', 'post_nonce_field') ) {
$save = sanitize_text_field( $_POST[ $key ] );
} else if ( isset( $field['default'] ) ) {
$save = $field['default'];
}| // @phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
| if ( isset( $_POST[ $tab_index . '_' . $key ] ) ) { | ||
| $save = $_POST[ $tab_index . '_' . $key ]; | ||
| $save = wp_unslash( $_POST[ $tab_index . '_' . $key ] ); |
There was a problem hiding this comment.
Again, ignoring nonce verification is not a good practice. Please consider adding nonce verification.
- // @phpcs:ignore WordPress.Security.NonceVerification.Recommended
- if ( isset( $_POST[ $tab_index . '_' . $key ] ) ) {
+ if ( isset( $_POST[ $tab_index . '_' . $key ] ) && check_admin_referer('tab_nonce_action', 'tab_nonce_field') ) {
$save = wp_unslash( $_POST[ $tab_index . '_' . $key ] );
} else if ( isset( $field['default'] ) ) {
$save = $field['default'];
}| /** | ||
| * checks which plugin is active, and grabs those forms. | ||
| */ |
There was a problem hiding this comment.
The function checks which plugin is active, and grabs those forms seems to be missing its implementation. Please ensure that the function body is included in the commit.
/**
* checks which plugin is active, and grabs those forms.
*/
+473: public function check_active_plugin_and_grab_forms() {
+474: // Implementation goes here...
+475: }| return $html; | ||
| } | ||
|
|
||
| /** | ||
| * Outputs the tour meta on the modal | ||
| */ | ||
| public function content_meta() { | ||
| if ( 'tour' === get_post_type() ) { | ||
| ?> | ||
| <?php | ||
| $meta_class = 'lsx-to-meta-data lsx-to-meta-data-'; | ||
|
|
||
| lsx_to_price( '<span class="' . $meta_class . 'price"><span class="lsx-to-meta-data-key">' . esc_html__( 'From price', 'tour-operator' ) . ':</span> ', '</span>' ); | ||
| lsx_to_duration( '<span class="' . $meta_class . 'duration"><span class="lsx-to-meta-data-key">' . esc_html__( 'Duration', 'tour-operator' ) . ':</span> ', '</span>' ); | ||
| the_terms( get_the_ID(), 'travel-style', '<span class="' . $meta_class . 'style"><span class="lsx-to-meta-data-key">' . esc_html__( 'Travel Style', 'tour-operator' ) . ':</span> ', ', ', '</span>' ); | ||
| lsx_to_connected_countries( '<span class="' . $meta_class . 'destinations"><span class="lsx-to-meta-data-key">' . esc_html__( 'Destinations', 'tour-operator' ) . ':</span> ', '</span>', true ); | ||
|
|
||
| if ( function_exists( 'lsx_to_connected_activities' ) ) { | ||
| lsx_to_connected_activities( '<span class="' . $meta_class . 'activities"><span class="lsx-to-meta-data-key">' . esc_html__( 'Activities', 'tour-operator' ) . ':</span> ', '</span>' ); | ||
| } | ||
| ?> | ||
| <?php | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Adds in the onsale classes. | ||
| * |
There was a problem hiding this comment.
The removal of the content_meta function might have an impact on the display of tour meta data in modals. If this functionality is being moved elsewhere, ensure that it's properly handled and there are no regressions. If this function is not used anymore, then the removal is fine. However, without more context, it's hard to say if this change could introduce bugs or not. Please make sure you've tested all the related functionalities thoroughly after removing this function.
| // Template Tags. | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/general.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/helpers.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/addons.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/accommodation.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/destination.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/tour.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/maps.php' ); | ||
| require_once( LSX_TO_PATH . 'includes/template-tags/videos.php' ); | ||
|
|
||
| // include context helper & autoloader. | ||
| require_once( LSX_TO_PATH . 'includes/tour-operator.php' ); |
There was a problem hiding this comment.
The changes seem to be removing the inclusion of several files (addons.php, accommodation.php, destination.php, tour.php, videos.php). If these files are no longer needed, this change is fine. However, if they contain necessary functionality, their removal could cause issues. Please ensure that all necessary functions from these files have been moved elsewhere or are no longer needed before proceeding with this change.
- require_once( LSX_TO_PATH . 'includes/template-tags/addons.php' );
- require_once( LSX_TO_PATH . 'includes/template-tags/accommodation.php' );
- require_once( LSX_TO_PATH . 'includes/template-tags/destination.php' );
- require_once( LSX_TO_PATH . 'includes/template-tags/tour.php' );
- require_once( LSX_TO_PATH . 'includes/template-tags/maps.php' );
- require_once( LSX_TO_PATH . 'includes/template-tags/videos.php' );
+ require_once( LSX_TO_PATH . 'includes/template-tags/general.php' );
+ require_once( LSX_TO_PATH . 'includes/template-tags/helpers.php' );
+ require_once( LSX_TO_PATH . 'includes/template-tags/maps.php' );
The following commit contains the PHP Coding standards updates, as well as the removal of unused files and functions from the code.
Summary by CodeRabbit
strip_tagswithwp_strip_all_tagsfor improved content sanitization.MetaboxesandShare, along with several unused functions and template tags, improving code maintainability.