2.0 WETU map embed block #437
Conversation
krugazul
added
[Type] Enhancement
WalkthroughThe changes in this pull request encompass various modifications across multiple files, including the addition of new functionalities, removal of obsolete code, and updates to existing configurations. Key alterations include the introduction of new block variations and functionalities in JavaScript files, the restructuring of SCSS files for improved styling management, and the removal of several legacy classes and configurations. Additionally, the Changes
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (124)
.gitignore (2)
7-14:
⚠️ Potential issueRemove duplicate .DS_Store entries
Strewth! There are multiple duplicate entries for
.DS_Storefiles which are completely unnecessary, hey. Let's clean this up properly.Apply this diff to remove the duplicates and group related entries:
-vendor/cmb-field-select2/.DS_Store -vendor/cmb2-field-map/.DS_Store -vendor/.DS_Store -.DS_Store -vendor/.DS_Store -.DS_Store -.DS_Store -vendor/.DS_Store +# macOS system files +.DS_Store + +# Vendor directory +vendor/**/.DS_Store📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# macOS system files .DS_Store # Vendor directory vendor/**/.DS_Store
7-14: 🧹 Nitpick (assertive)
Consider using more specific patterns
Howzit! Instead of listing individual vendor paths, we could make this more maintainable with patterns.
Here's a more comprehensive approach:
-vendor/cmb-field-select2/.DS_Store -vendor/cmb2-field-map/.DS_Store -vendor/.DS_Store -.DS_Store -vendor/.DS_Store -.DS_Store -.DS_Store -vendor/.DS_Store +# System files +**/.DS_Store +**/Thumbs.db + +# IDE files +**/*.sublime-workspace +**/.idea + +# Dependencies +node_modules/ +vendor/ + +# Build artifacts +.sass-cache/ +wp-content/ +package-lock.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# System files **/.DS_Store **/Thumbs.db # IDE files **/*.sublime-workspace **/.idea # Dependencies node_modules/ vendor/ # Build artifacts .sass-cache/ wp-content/ package-lock.jsonincludes/partials/settings.php (6)
1-18: 🧹 Nitpick (assertive)
Consider implementing settings API properly.
The current implementation doesn't fully utilise WordPress Settings API. Consider restructuring to use
register_setting()andadd_settings_section().Would you like me to provide a complete example of how to implement this using the WordPress Settings API?
1-4: 🛠️ Refactor suggestion
Add error handling for post types retrieval.
The
get_post_types()method call could potentially fail or return empty results. Consider adding error handling to gracefully handle such scenarios.<?php +if ( ! function_exists( 'tour_operator' ) || ! is_callable( array( tour_operator()->legacy, 'get_post_types' ) ) ) { + wp_die( esc_html__( 'Tour Operator plugin is not properly initialized.', 'tour-operator' ) ); +} + $settings_pages = tour_operator()->legacy->get_post_types(); + +if ( empty( $settings_pages ) ) { + add_settings_error( + 'tour_operator_settings', + 'no_post_types', + esc_html__( 'No post types available for configuration.', 'tour-operator' ), + 'error' + ); +} ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<?php if ( ! function_exists( 'tour_operator' ) || ! is_callable( array( tour_operator()->legacy, 'get_post_types' ) ) ) { wp_die( esc_html__( 'Tour Operator plugin is not properly initialized.', 'tour-operator' ) ); } $settings_pages = tour_operator()->legacy->get_post_types(); if ( empty( $settings_pages ) ) { add_settings_error( 'tour_operator_settings', 'no_post_types', esc_html__( 'No post types available for configuration.', 'tour-operator' ), 'error' ); } ?>
5-7:
⚠️ Potential issueAdd security measures to the form.
The form lacks necessary security measures such as nonce verification and capability checks.
<div class="wrap lsx-to-settings"> <h1><?php echo esc_html__( 'LSX Tour Operator Settings', 'tour-operator' ); ?></h1> - <form method="post"> + <?php + if ( ! current_user_can( 'manage_options' ) ) { + wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'tour-operator' ) ); + } + ?> + <form method="post" action="options.php"> + <?php + settings_fields( 'tour_operator_settings' ); + wp_nonce_field( 'tour_operator_settings_action', 'tour_operator_settings_nonce' ); + ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div class="wrap lsx-to-settings"> <h1><?php echo esc_html__( 'LSX Tour Operator Settings', 'tour-operator' ); ?></h1> <?php if ( ! current_user_can( 'manage_options' ) ) { wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'tour-operator' ) ); } ?> <form method="post" action="options.php"> <?php settings_fields( 'tour_operator_settings' ); wp_nonce_field( 'tour_operator_settings_action', 'tour_operator_settings_nonce' ); ?>
16-18: 🧹 Nitpick (assertive)
Enhance form submission UX.
The submit button lacks proper styling classes and feedback mechanism for users.
- <input name="submit" class="button button-primary" type="submit" value="Save Settings" /> + <?php + submit_button( esc_html__( 'Save Settings', 'tour-operator' ), 'primary large', 'submit', false ); + ?> + <div id="setting-error-settings_updated" class="notice notice-success settings-error is-dismissible" style="display:none;"> + <p><strong><?php esc_html_e( 'Settings saved successfully.', 'tour-operator' ); ?></strong></p> + </div> </form> </div> +<?php +// Add success message display logic +if ( isset( $_GET['settings-updated'] ) && $_GET['settings-updated'] ) { + add_settings_error( + 'tour_operator_settings', + 'settings_updated', + esc_html__( 'Settings saved successfully.', 'tour-operator' ), + 'success' + ); +} +settings_errors( 'tour_operator_settings' ); +?>Committable suggestion skipped: line range outside the PR's diff.
8-10: 🛠️ Refactor suggestion
Verify partial file existence before including.
The code directly includes partial files without checking if they exist, which could lead to fatal errors.
- <?php - include( LSX_TO_PATH . 'includes/partials/navigation.php' ); - include( LSX_TO_PATH . 'includes/partials/general.php' ); + <?php + $partials = array( + 'navigation.php', + 'general.php' + ); + + foreach ( $partials as $partial ) { + $partial_path = LSX_TO_PATH . 'includes/partials/' . $partial; + if ( file_exists( $partial_path ) ) { + include $partial_path; + } else { + add_settings_error( + 'tour_operator_settings', + 'missing_partial', + sprintf( esc_html__( 'Missing required partial: %s', 'tour-operator' ), $partial ), + 'error' + ); + } + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<?php $partials = array( 'navigation.php', 'general.php' ); foreach ( $partials as $partial ) { $partial_path = LSX_TO_PATH . 'includes/partials/' . $partial; if ( file_exists( $partial_path ) ) { include $partial_path; } else { add_settings_error( 'tour_operator_settings', 'missing_partial', sprintf( esc_html__( 'Missing required partial: %s', 'tour-operator' ), $partial ), 'error' ); } }
12-14: 🧹 Nitpick (assertive)
Optimise the settings pages loop.
The current implementation includes a file within a loop, which could impact performance with many post types.
- foreach ( $settings_pages as $tab_index => $tab ) { - include( LSX_TO_PATH . 'includes/partials/post-type.php' ); - } + if ( ! empty( $settings_pages ) ) { + $post_type_template = LSX_TO_PATH . 'includes/partials/post-type.php'; + if ( file_exists( $post_type_template ) ) { + foreach ( $settings_pages as $tab_index => $tab ) { + include $post_type_template; + } + } + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( ! empty( $settings_pages ) ) { $post_type_template = LSX_TO_PATH . 'includes/partials/post-type.php'; if ( file_exists( $post_type_template ) ) { foreach ( $settings_pages as $tab_index => $tab ) { include $post_type_template; } } }includes/partials/navigation.php (3)
1-3:
⚠️ Potential issueAdd error handling for tour operator object
Howzit! The code assumes the tour operator object is always available. We should add proper error checking to prevent fatal errors if the plugin isn't properly initialized.
Here's a safer implementation:
<?php +if ( ! function_exists( 'tour_operator' ) || ! is_object( tour_operator()->legacy ) ) { + return; +} $settings_pages = tour_operator()->legacy->get_post_types(); ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<?php if ( ! function_exists( 'tour_operator' ) || ! is_object( tour_operator()->legacy ) ) { return; } $settings_pages = tour_operator()->legacy->get_post_types(); ?>
6-9: 🧹 Nitpick (assertive)
Enhance tab validation and define default tab as constant
The current tab handling could be more robust. Let's define the default tab as a constant and validate that the requested tab exists.
Consider this improvement:
+define( 'TO_DEFAULT_TAB', 'general' ); -$current_tab = 'general'; +$current_tab = TO_DEFAULT_TAB; if ( isset( $_GET['tab'] ) ) { - $current_tab = sanitize_key( $_GET['tab'] ); + $requested_tab = sanitize_key( $_GET['tab'] ); + $current_tab = isset( $settings_pages[$requested_tab] ) ? $requested_tab : TO_DEFAULT_TAB; }Committable suggestion skipped: line range outside the PR's diff.
11-18: 🧹 Nitpick (assertive)
Improve template structure and active tab logic
The markup generation could be more maintainable, and the active tab logic can be simplified.
Consider this improvement:
-echo wp_kses_post( '<a class="nav-tab nav-tab-active" href="#ui-general">' . esc_html__( 'General', 'tour-operator' ) . '</a>' ); +$tabs = array_merge( + array( TO_DEFAULT_TAB => esc_html__( 'General', 'tour-operator' ) ), + $settings_pages +); + +foreach ( $tabs as $tab_index => $tab ) { + printf( + '<a class="nav-tab %s" href="#ui-%s">%s</a>', + esc_attr( $tab_index === $current_tab ? 'nav-tab-active' : '' ), + esc_attr( $tab_index ), + wp_kses_post( $tab ) + ); +} -foreach ( $settings_pages as $tab_index => $tab ) { - $class = ''; - if ( $tab_index === $current_tab ) { - $class = 'nav-tab-active'; - } - echo wp_kses_post( '<a class="nav-tab ' . $class . '" href="#ui-' . $tab_index . '">' . $tab . '</a>' ); -}Committable suggestion skipped: line range outside the PR's diff.
includes/partials/post-type.php (3)
4-11: 🧹 Nitpick (assertive)
Consider enhancing accessibility for the form field heading.
The heading structure could be improved for better accessibility.
Here's a suggested improvement:
<tr class="form-field"> - <th scope="row" colspan="2"> + <th scope="row" colspan="2" id="placeholder-settings-<?php echo esc_attr( $tab_index ); ?>"> <label> - <h3> <?php echo esc_html( $tab ); ?> <?php esc_html_e( 'Placeholder Settings', 'tour-operator' ); ?></h3> + <h3 aria-labelledby="placeholder-settings-<?php echo esc_attr( $tab_index ); ?>"> + <?php echo esc_html( $tab ); ?> <?php esc_html_e( 'Placeholder Settings', 'tour-operator' ); ?> + </h3> </label> </th> </tr>Committable suggestion skipped: line range outside the PR's diff.
14-21: 🧹 Nitpick (assertive)
Apply consistent accessibility improvements to Template Settings section.
Similar accessibility improvements should be applied to maintain consistency.
Here's the suggested improvement:
<tr class="form-field"> - <th scope="row" colspan="2"> + <th scope="row" colspan="2" id="template-settings-<?php echo esc_attr( $tab_index ); ?>"> <label> - <h3><?php esc_html_e( 'Template Settings', 'tour-operator' ); ?></h3> + <h3 aria-labelledby="template-settings-<?php echo esc_attr( $tab_index ); ?>"> + <?php esc_html_e( 'Template Settings', 'tour-operator' ); ?> + </h3> </label> </th> </tr>Committable suggestion skipped: line range outside the PR's diff.
1-25: 🧹 Nitpick (assertive)
Consider adding ARIA attributes for tab panel.
The tab panel structure could benefit from additional ARIA attributes for better accessibility.
Apply these improvements to the main container:
-<div id="ui-<?php echo esc_attr( $tab_index ); ?>" class="ui-tab tabs-content"> +<div id="ui-<?php echo esc_attr( $tab_index ); ?>" + class="ui-tab tabs-content" + role="tabpanel" + aria-labelledby="tab-<?php echo esc_attr( $tab_index ); ?>">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div id="ui-<?php echo esc_attr( $tab_index ); ?>" class="ui-tab tabs-content" role="tabpanel" aria-labelledby="tab-<?php echo esc_attr( $tab_index ); ?>"> <table class="form-table"> <tbody> <tr class="form-field"> <th scope="row" colspan="2"> <label> <h3> <?php echo esc_html( $tab ); ?> <?php esc_html_e( 'Placeholder Settings', 'tour-operator' ); ?></h3> </label> </th> </tr> <?php do_action( 'lsx_to_framework_post_type_tab_content', 'placeholder', $tab_index ); ?> <tr class="form-field"> <th scope="row" colspan="2"> <label> <h3><?php esc_html_e( 'Template Settings', 'tour-operator' ); ?></h3> </label> </th> </tr> <?php do_action( 'lsx_to_framework_post_type_tab_content', 'template', $tab_index ); ?> </tbody> </table> </div>assets/js/blocks/tour.js (2)
3-8: 🧹 Nitpick (assertive)
Consider using a more appropriate icon for the map block.
The current icon 'admin-site' is quite generic. WordPress includes more suitable icons for maps, such as 'location' or 'location-alt', which would better represent the block's purpose.
- icon: 'admin-site', + icon: 'location-alt',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// WETU Map Embed wp.blocks.registerBlockVariation('core/group', { name: 'lsx-tour-operator/wetu-map', title: 'WETU Map', icon: 'location-alt', category: 'lsx-tour-operator',
37-58: 💡 Codebase verification
⚠️ Potential issueUpdate all hardcoded image URLs and add alt text for accessibility
The hardcoded domain "tour-operator.lsx.design" is used extensively across the codebase for various images, including icons, banners, and placeholders. This requires a systematic update:
In
assets/js/blocks/tour.js:
- Remove domain from map image URL
- Add descriptive alt text
In
assets/js/blocks/variations.js:
- Remove domain from all icon and placeholder image URLs
- Add descriptive alt text for each icon
In template files:
- Update all banner and icon image URLs to be relative
- Add meaningful alt text for all images
Example fix for the map image:
-url: "https://tour-operator.lsx.design/wp-content/uploads/2024/09/wetu-map-figme-prototype-image.png", -alt: "", +url: "/wp-content/uploads/2024/09/wetu-map-figme-prototype-image.png", +alt: "Interactive WETU map showing tour route and destinations",🔗 Analysis chain
Address accessibility and environment-specific concerns.
Two issues require attention:
- The hardcoded image URL points to a specific environment (tour-operator.lsx.design), which might not work in other environments.
- The image lacks alternative text, which is crucial for accessibility.
- url: "https://tour-operator.lsx.design/wp-content/uploads/2024/09/wetu-map-figme-prototype-image.png", - alt: "", + url: "/wp-content/uploads/2024/09/wetu-map-figme-prototype-image.png", + alt: "WETU Map Preview",Let's verify if this image URL is used elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded URLs that might need updating rg -l "tour-operator.lsx.design"Length of output: 360
Script:
#!/bin/bash # Check the context of these URLs in the files rg -A 2 -B 2 "tour-operator.lsx.design" --type html --type jsLength of output: 55161
includes/classes/blocks/class-patterns.php (6)
1-4: 🧹 Nitpick (assertive)
Add file-level documentation.
Proper documentation is essential for maintaining code quality. Please add a file-level docblock that includes:
- File description
- Package information
- Copyright notice
- Licence information
+/** + * LSX Tour Operator Block Patterns + * + * @package LSX Tour Operator + * @author LightSpeed + * @license GPL-3.0+ + * @copyright 2024 LightSpeed + */ <?php namespace lsx\blocks;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * LSX Tour Operator Block Patterns * * @package LSX Tour Operator * @author LightSpeed * @license GPL-3.0+ * @copyright 2024 LightSpeed */ <?php namespace lsx\blocks;🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 1-1: includes/classes/blocks/class-patterns.php#L1
Missing file doc comment. (Squiz.Commenting.FileComment)
5-13: 🧹 Nitpick (assertive)
Enhance property documentation.
The property documentation could be more descriptive. Consider clarifying its purpose and relationship to block patterns.
/** - * Holds the slug of the projects pattern category. + * Holds the unique identifier for the LSX Tour Operator block pattern category. + * This slug is used to group related block patterns under a common category. * * @var string */ private $category = 'lsx-tour-operator';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class Patterns { /** * Holds the unique identifier for the LSX Tour Operator block pattern category. * This slug is used to group related block patterns under a common category. * * @var string */ private $category = 'lsx-tour-operator';
55-59: 🧹 Nitpick (assertive)
Complete the incomplete docblock.
The docblock for
register_block_patterns()is incomplete./** - * Registers our block patterns with the + * Registers our block patterns with WordPress. + * + * This method loads pattern definitions from separate files and registers them + * with the block editor system. * * @return void */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Registers our block patterns with WordPress. * * This method loads pattern definitions from separate files and registers them * with the block editor system. * * @return void */
1-69: 🧹 Nitpick (assertive)
💡 Codebase verification
WETU map block pattern is missing from implementation
The verification confirms that:
- Only the itinerary card pattern is currently implemented at
includes/patterns/itinerary-card.php- No WETU map-related patterns exist in the patterns directory
- The patterns registration array only includes the itinerary card pattern
To implement the WETU map embed block pattern:
- Create a new pattern file at
includes/patterns/wetu-map.php- Add the pattern to the registration array in
class-patterns.php- Include the necessary block markup for the WETU map embed
🔗 Analysis chain
Verify WETU map block pattern integration.
The current implementation only includes the itinerary card pattern. Based on the PR objectives, we should also see the WETU map embed block pattern.
Would you like assistance in implementing the WETU map embed block pattern?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for WETU map-related patterns and templates echo "Searching for WETU map-related files..." fd -e php -e json "wetu.*map" "includes/patterns" echo "Checking pattern registration calls..." rg -l "register_block_pattern.*wetu.*map" "includes/classes"Length of output: 286
Script:
#!/bin/bash # Let's check for any WETU or map related files and patterns more broadly echo "Searching for any WETU related files..." fd -t f "wetu" echo "Searching for map related patterns..." fd -t f "map" "includes/patterns" echo "Checking pattern registration in Patterns class..." rg -A 5 "patterns = array" "includes/classes/blocks/class-patterns.php" echo "Checking if there are any other pattern files..." fd -e php . "includes/patterns"Length of output: 875
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 1-1: includes/classes/blocks/class-patterns.php#L1
Missing file doc comment. (Squiz.Commenting.FileComment)
21-28: 🧹 Nitpick (assertive)
Consider implementing priority constants.
The hardcoded priority values in the action hooks could be replaced with class constants for better maintainability.
+ /** + * Default priority for category registration. + */ + const CATEGORY_PRIORITY = 10; + public function __construct() { //Register our categories - add_filter( 'block_categories_all', array( $this, 'register_block_category' ), 10, 1 ); + add_filter( 'block_categories_all', array( $this, 'register_block_category' ), self::CATEGORY_PRIORITY, 1 ); add_action( 'init', array( $this, 'register_block_pattern_category' ) ); // Register our block patterns - add_action( 'init', array( $this, 'register_block_patterns' ), 10 ); + add_action( 'init', array( $this, 'register_block_patterns' ), self::CATEGORY_PRIORITY );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Default priority for category registration. */ const CATEGORY_PRIORITY = 10; public function __construct() { //Register our categories add_filter( 'block_categories_all', array( $this, 'register_block_category' ), self::CATEGORY_PRIORITY, 1 ); add_action( 'init', array( $this, 'register_block_pattern_category' ) ); // Register our block patterns add_action( 'init', array( $this, 'register_block_patterns' ), self::CATEGORY_PRIORITY ); }
60-68:
⚠️ Potential issueVerify pattern file existence and enhance error handling.
The current implementation assumes the pattern file exists. Let's add proper error handling.
public function register_block_patterns() { + $pattern_file = LSX_TO_PATH . '/includes/patterns/itinerary-card.php'; $patterns = array( - 'lsx-tour-operator/itinerary-card' => require( LSX_TO_PATH . '/includes/patterns/itinerary-card.php' ), + 'lsx-tour-operator/itinerary-card' => file_exists($pattern_file) ? require($pattern_file) : array(), ); foreach ( $patterns as $key => $function ) { + if (empty($function)) { + error_log(sprintf('LSX Tour Operator: Pattern file for %s not found or empty.', $key)); + continue; + } register_block_pattern( $key, $function ); } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public function register_block_patterns() { $pattern_file = LSX_TO_PATH . '/includes/patterns/itinerary-card.php'; $patterns = array( 'lsx-tour-operator/itinerary-card' => file_exists($pattern_file) ? require($pattern_file) : array(), ); foreach ( $patterns as $key => $function ) { if (empty($function)) { error_log(sprintf('LSX Tour Operator: Pattern file for %s not found or empty.', $key)); continue; } register_block_pattern( $key, $function ); } }assets/js/blocks/query-loops.js (3)
51-64: 🧹 Nitpick (assertive)
Consider enhancing the query configuration
The current query configuration might benefit from:
- Pagination support for the 8-item limit
- Additional filtering options for better content curation
Consider adding these configurations:
query: { perPage: 8, - pages: 0, + pages: 1, offset: 0, postType: 'destination', order: 'asc', orderBy: 'date', + taxQuery: { + destination-type: [], + destination-region: [] + }, author: '',Committable suggestion skipped: line range outside the PR's diff.
67-68: 🛠️ Refactor suggestion
Add responsive considerations for the grid layout
The fixed 4-column grid might not provide the best experience on smaller screens.
Consider implementing a responsive grid:
-[ 'core/post-template', { layout: { type: 'grid', columnCount: 4 } }, [ +[ 'core/post-template', { + layout: { + type: 'grid', + columnCount: 4 + }, + style: { + spacing: { + blockGap: { top: 'var:preset|spacing|medium' } + } + }, + className: 'is-responsive-grid' +}, [Committable suggestion skipped: line range outside the PR's diff.
67-69: 💡 Codebase verification
Hardcoded block reference needs attention
The block reference '3164' appears to be hardcoded in a query loop template without any corresponding reusable block definition found in the codebase. This could cause issues if:
- The referenced block doesn't exist in the environment
- The block ID differs across environments
- The block is accidentally deleted
Consider:
- Using a dynamic block reference mechanism instead of a hardcoded ID
- Creating the block template programmatically
- Adding documentation about the required reusable block if it must be manually created
🔗 Analysis chain
Verify the hardcoded block reference
The block reference '3164' might cause issues if the referenced block doesn't exist in all environments. Consider making this reference more dynamic or documenting its requirements.
The previous search was too narrow. Let's broaden it to:
- Search for the reference across all file types
- Look for block reference patterns in general
- Check block registration and template patterns that might define these references
Let's search for:
- Block template registrations that might define these references
- Block type registrations related to templates
- Template definitions in JSON configuration files
This will help understand if the block reference is part of a template system or standalone usage.
Let's search more specifically for:
- Any reusable block definitions with ID 3164
- Block patterns or templates referencing this ID
- Code that handles block references in general
This will help determine if this is a valid reusable block reference and how it's being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for block definition with ID 3164 rg -l "3164" --type jsonLength of output: 24
Script:
#!/bin/bash # Search for block reference '3164' in all files, not just JSON rg -l "3164" # Also search for patterns related to block references ast-grep --pattern 'ref: $_' # Look for block registration or template patterns rg -A 5 "registerBlockType|template.*\["Length of output: 113469
Script:
#!/bin/bash # Search for block template patterns and registrations that might define block references ast-grep --pattern 'registerBlockTemplate' # Look for block reference patterns in block registration code rg -A 5 "registerBlockType.*cover|registerBlockType.*template" # Search for block template definitions in JSON files rg -l "\"template\":" --type jsonLength of output: 785
Script:
#!/bin/bash # Look for reusable block definitions that might use ID 3164 rg -A 5 "wp_block.*3164|block.*3164" # Check for any block patterns or templates that might reference this ID rg -A 5 "\"ref\".*3164|ref:.*3164" --type json --type js # Search for block registration code that handles references ast-grep --pattern 'core/block'Length of output: 651
assets/js/blocks/slotfills.js (4)
34-54: 🛠️ Refactor suggestion
Refactor duplicate code and fix variable naming
Shame, we've got some duplicate code here! The
DisableSingleTogglecomponent is nearly identical toStickyToggle. Also, the variableisStickyis incorrectly named for this context.Let's create a reusable toggle component:
const MetaToggle = ({ metaKey, label }) => { const { editPost } = useDispatch("core/editor"); const handleChange = (newChecked) => { editPost({ meta: { [metaKey]: newChecked } }); }; const isEnabled = useSelect(function (select) { return select('core/editor').getEditedPostAttribute('meta')[metaKey]; }, [metaKey]); return createElement(ToggleControl, { label: i18n.__(label), checked: isEnabled, onChange: handleChange }); }; MetaToggle.propTypes = { metaKey: PropTypes.string.isRequired, label: PropTypes.string.isRequired }; // Usage: const StickyToggle = () => createElement(MetaToggle, { metaKey: 'featured', label: 'Featured' }); const DisableSingleToggle = () => createElement(MetaToggle, { metaKey: 'disable_single', label: 'Disable Single' });
1-9: 🧹 Nitpick (assertive)
Remove unused HStack import
Howzit! I noticed that the
HStackcomponent is imported but never used in the code. Let's keep our imports clean.- const { HStack } = wp.components;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.(function (wp) { const { registerPlugin } = wp.plugins; const { PluginPostStatusInfo } = wp.editPost; const { ToggleControl } = wp.components; const { useSelect, useDispatch } = wp.data; const { createElement, useState } = wp.element; const i18n = window.wp.i18n;
56-84: 🧹 Nitpick (assertive)
Simplify component structure
The component structure can be simplified by using Fragment and removing unnecessary div wrappers.
const LsxToStatusPlugin = function() { return createElement( PluginPostStatusInfo, { name: 'lsx-to-toggles', className: 'lsx-to-toggles' }, - createElement( - 'div', + createElement( + wp.element.Fragment, { - className: 'lsx-to-toggles__inner' }, - createElement( - 'div', - { - className: 'toggle-row' - }, - createElement(StickyToggle), - ), - createElement( - 'div', - { - className: 'toggle-row' - }, - createElement(DisableSingleToggle), - ) + createElement(StickyToggle), + createElement(DisableSingleToggle) ) ); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const LsxToStatusPlugin = function() { return createElement( PluginPostStatusInfo, { name: 'lsx-to-toggles', className: 'lsx-to-toggles' }, createElement( wp.element.Fragment, { }, createElement(StickyToggle), createElement(DisableSingleToggle) ) ); };
11-31: 🧹 Nitpick (assertive)
Improve variable naming and add PropTypes
The variable name
isStickyshould be renamed toisFeaturedto better reflect its purpose, as it's managing the 'featured' meta attribute.Additionally, while the component works, it would be lekker to add PropTypes for better development experience and code maintainability.
+import PropTypes from 'prop-types'; + const StickyToggle = () => { const { editPost } = useDispatch("core/editor"); const handleChange = (newChecked) => { editPost({ meta: { 'featured': newChecked } }); }; - const isSticky = useSelect( function ( select ) { + const isFeatured = useSelect( function ( select ) { return select( 'core/editor' ).getEditedPostAttribute( 'meta' )[ 'featured' ]; }, [] ); return createElement( ToggleControl, { label: i18n.__('Featured'), - checked: isSticky, + checked: isFeatured, onChange: handleChange } ); }; + +StickyToggle.propTypes = {};Committable suggestion skipped: line range outside the PR's diff.
assets/js/src/admin.js (3)
15-15: 🧹 Nitpick (assertive)
Consider modernising the image selection implementation.
Howzit! The current implementation could do with some improvements:
- Replace ThickBox with the WordPress Media Modal API for a more modern approach
- Add error handling for missing image attributes
- Consider using destructuring for cleaner attribute extraction
Here's a suggested implementation:
- tb_show('Select an Image', 'media-upload.php?type=image&feature_image_text_button=1&TB_iframe=1'); + const frame = wp.media({ + title: 'Select an Image', + multiple: false, + library: { type: 'image' } + }); + + frame.on('select', function() { + const attachment = frame.state().get('selection').first().toJSON(); + if (!attachment) return; + + const { id, url, width, height } = attachment; + + $td.find('.thumbnail-preview img') + .attr('src', url) + .parent() + .show(); + + $td.find('input.input_image') + .val(id) + .attr('width', width || '') + .attr('height', height || ''); + + $this.hide(); + $td.find('.lsx-thumbnail-image-delete, .lsx-thumbnail-image-remove').show(); + }); + + frame.open();Also applies to: 21-21, 30-31
70-71: 🛠️ Refactor suggestion
Enhance tab navigation with accessibility and modern practices.
Ja, let's make this more robust:
- Add proper ARIA attributes for accessibility
- Support deep linking with URL hash
- Use event delegation more efficiently
- jQuery(document).on('click', '.nav-tab-wrapper a', function(e) { + const $wrapper = jQuery('.nav-tab-wrapper'); + $wrapper + .attr('role', 'tablist') + .find('a') + .attr('role', 'tab') + .parent() + .attr('role', 'presentation'); + + jQuery('.ui-tab') + .attr('role', 'tabpanel') + .attr('aria-hidden', 'true'); + + $wrapper.on('click', 'a', function(e) { e.preventDefault(); e.stopPropagation(); const $this = jQuery(this); const $target = jQuery($this.attr('href')); + // Update ARIA states + $wrapper.find('a') + .attr('aria-selected', 'false') + .removeClass('nav-tab-active'); + + $this + .attr('aria-selected', 'true') + .addClass('nav-tab-active'); + + jQuery('.ui-tab') + .attr('aria-hidden', 'true') + .removeClass('active'); + + $target + .attr('aria-hidden', 'false') + .addClass('active'); + + // Update URL hash without scrolling + history.pushState(null, '', $this.attr('href'));Also applies to: 77-78, 80-80
56-56: 🛠️ Refactor suggestion
Add confirmation and cleanup width/height attributes.
Shame, we should prevent accidental deletions and ensure proper cleanup:
- $td.find('input.input_image').val(''); - $td.find('.thumbnail-preview img').attr('src','').parent().hide(); + if (confirm('Are you sure you want to remove this image?')) { + const $input = $td.find('input.input_image'); + const $preview = $td.find('.thumbnail-preview img'); + + $input + .val('') + .removeAttr('width') + .removeAttr('height'); + + $preview + .attr('src', '') + .parent() + .hide();Committable suggestion skipped: line range outside the PR's diff.
assets/css/scss/_icons.scss (4)
106-117: 🛠️ Refactor suggestion
Remove commented legacy code
Ag, keeping commented-out code makes maintenance more difficult and clutters the codebase. If this styling is no longer needed, we should remove it entirely. If it might be needed later, rather track it in version control.
Remove the entire commented block to improve code maintainability.
133-134: 🛠️ Refactor suggestion
Remove unused destination styles comment
Shame, this commented line isn't adding any value. Let's remove it to keep our codebase clean.
Remove the commented line entirely.
119-131: 🧹 Nitpick (assertive)
Clean up price wrapper styles
- Remove the commented background image code as it's no longer used.
- The
0pxunit is unnecessary; use0instead.Apply this diff to clean up the code:
.lsx-price-wrapper { - /*&:before { - background-image: url( ../img/icons/price-icon.png ); - }*/ - & .amount { @extend %currency-icons; .currency-icon { - &:after { margin-right: 0px; } + &:after { margin-right: 0; } } } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..lsx-price-wrapper { & .amount { @extend %currency-icons; .currency-icon { &:after { margin-right: 0; } } } }
1-104: 🧹 Nitpick (assertive)
⚠️ Potential issueFix incorrect currency symbol and enhance maintainability
Howzit! I've spotted a few issues that need attention:
- The IDR (Indonesian Rupiah) symbol is incorrectly using the Indian Rupee symbol (₹). The correct Unicode for IDR is "\0052\0070".
- Consider extracting the margin values (3px) to SCSS variables for easier maintenance.
Apply this diff to fix the IDR symbol:
&.idr { - &:after { content: '\20B9'; } + &:after { content: '\0052\0070'; } }Consider defining margin variables at the top of the file:
+$currency-icon-margin: 3px; + %currency-icons { .currency-icon { &:after { - margin-left: 3px; - margin-right: 3px; + margin-left: $currency-icon-margin; + margin-right: $currency-icon-margin; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$currency-icon-margin: 3px; %currency-icons { .currency-icon { &:after { margin-left: $currency-icon-margin; margin-right: $currency-icon-margin; } &.usd, &.nad, &.cad, &.hkd, &.sgd, &.nzd, &.aud { &:after { content: '\0024'; } } &.eur { &:after { content: '\20AC'; } } &.gbp { &:after { content: '\00A3'; } } &.zar { &:after { content: '\0052'; } } &.brl { &:after { content: '\0052\0024'; } } &.bwp { &:after { content: '\0050'; } } &.cny, &.jpy { &:after { content: '\00A5'; } } &.inr { &:after { content: '\20B9'; } } &.idr { &:after { content: '\0052\0070'; } } &.ils { &:after { content: '\20AA'; } } &.kes { &:after { content: '\004b'; } } &.lak { &:after { content: '\20ad'; } } &.mwk { &:after { content: '\004d\004b'; } } &.myr { &:after { content: '\0052\004d'; } } &.mzn { &:after { content: '\004d\0054'; } } &.nok, &.sek { &:after { content: '\006b\0072'; } } &.rub { &:after { content: '\0440\0443\0431'; } } &.chf { &:after { content: '\0043\0048\0046'; } } &.tzs { &:after { content: '\0054\0053\0068'; } } &.aed { &:after { content: '\062f\002e\0625'; } } &.zmw { &:after { content: '\005a\006b'; } } &.zwl { &:after { content: '\005a\0024'; } } } }assets/css/style.css (3)
76-91: 🧹 Nitpick (assertive)
Remove commented code block
Kindly remove this commented-out CSS block. If this styling might be needed later, rather track it in version control history or create a task to properly implement it.
-/*.lsx-price-wrapper, -.lsx-destination-to-tour-wrapper { - &:before { - background-repeat: no-repeat; - background-size: contain; - width: 20px; - height: 30px; - content: " "; - background-position: center top; - } -}*/ -.lsx-price-wrapper { - /*&:before { - background-image: url( ../img/icons/price-icon.png ); - }*/ -}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
92-93: 🛠️ Refactor suggestion
Consolidate margin rules
This margin override should be consolidated with the earlier margin definitions (lines 5-8) to improve maintainability and prevent confusion.
-/* Earlier in the file */ -.lsx-price-wrapper .amount .currency-icon:after { - margin-left: 3px; - margin-right: 3px; -} - -/* Later override */ -.lsx-price-wrapper .amount .currency-icon:after { - margin-right: 0px; -} +/* Single, consolidated rule */ +.lsx-price-wrapper .amount .currency-icon:after { + margin-left: 3px; + margin-right: 0; +}Committable suggestion skipped: line range outside the PR's diff.
5-72: 🧹 Nitpick (assertive)
Standardise margin spacing and consider RTL support
The margin spacing between currency symbols is inconsistent. Line 5-8 defines both left and right margins as 3px, whilst line 92-93 overrides the right margin to 0px. Additionally, for currencies like AED (د.إ), consider adding RTL support.
.lsx-price-wrapper .amount .currency-icon:after { - margin-left: 3px; - margin-right: 3px; + margin-left: 0; + margin-right: 3px; } .lsx-price-wrapper .amount .currency-icon.aed:after { content: "د.إ"; + direction: rtl; }Committable suggestion skipped: line range outside the PR's diff.
includes/classes/legacy/class-video.php (2)
80-80: 🧹 Nitpick (assertive)
Consider enhancing the URL field description.
While the descriptions are generally excellent, the URL field could benefit from additional guidance about supported video platforms or URL formats.
Apply this diff to enhance the URL field description:
array( 'id' => 'url', 'name' => __( 'Url', 'tour-operator' ), - 'desc' => __( 'The URL link to the video.', 'tour-operator' ), + 'desc' => __( 'The URL link to the video. Supports YouTube, Vimeo, and direct video URLs.', 'tour-operator' ), 'type' => 'text', ),Also applies to: 86-86, 92-92, 98-98
70-75:
⚠️ Potential issuePlease resolve the conflicting sortable configurations.
There's a conflict in the sorting configuration:
- Line 69:
'sortable' => true(parent configuration)- Line 74:
'sortable' => false(options configuration)Additionally, consider enhancing the group title template to include more context, e.g., "Video {#} of {total}".
Apply this diff to resolve the conflict and improve the title template:
'sortable' => false, 'options' => array( - 'group_title' => __( 'Video {#}', 'tour-operator' ), + 'group_title' => __( 'Video {#} of {total}', 'tour-operator' ), 'add_button' => __( 'Add Video', 'tour-operator' ), 'remove_button' => __( 'Remove Video', 'tour-operator' ), 'sortable' => false,Committable suggestion skipped: line range outside the PR's diff.
includes/classes/admin/class-pages.php (1)
75-75: 🧹 Nitpick (assertive)
Well-placed action hook for menu page registration!
The addition of this action hook follows WordPress best practices by providing a clean extension point for registering additional menu pages. The placement before the main page creation ensures that custom pages can be properly integrated into the menu hierarchy.
Consider documenting this hook in your plugin's developer documentation to help other developers understand:
- When this hook fires
- What parameters are available
- Example usage for registering custom menu pages
assets/css/scss/admin.scss (1)
69-74: 🧹 Nitpick (assertive)
Lekker implementation, but we can make it even better!
The tab visibility control works well, but consider adding a fade transition for smoother tab switching.
Here's how you can enhance it:
.ui-tab { - display: none; + opacity: 0; + visibility: hidden; + transition: opacity 0.3s ease; + height: 0; &.active { - display: block; + opacity: 1; + visibility: visible; + height: auto; } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..ui-tab { opacity: 0; visibility: hidden; transition: opacity 0.3s ease; height: 0; &.active { opacity: 1; visibility: visible; height: auto; }includes/classes/class-tour-operator.php (2)
7-12: 🧹 Nitpick (assertive)
Well-structured namespace organization!
The separation into admin and blocks namespaces shows good architectural thinking. This modular approach will make the codebase more maintainable.
Consider documenting the responsibility of each namespace in a README.md file to help other developers understand the system architecture.
220-226: 🧹 Nitpick (assertive)
Consider lazy loading for service initialization
The change from static initialization to instance-based is good for testing and dependency management. However, creating all service instances at once might impact performance.
Consider implementing lazy loading for these services:
- $this->admin = new Admin(); - $this->settings = Settings::init(); - $this->setup = Setup::init(); - $this->bindings = new Bindings(); - $this->registration = new Registration(); - $this->patterns = new Patterns(); - $this->templates = new Templates(); + $this->initializeServices(); + + private function initializeServices() { + static $initialized = false; + if (!$initialized) { + $this->admin = new Admin(); + $this->settings = Settings::init(); + $this->setup = Setup::init(); + $this->bindings = new Bindings(); + $this->registration = new Registration(); + $this->patterns = new Patterns(); + $this->templates = new Templates(); + $initialized = true; + } + }Committable suggestion skipped: line range outside the PR's diff.
includes/classes/blocks/class-templates.php (6)
35-101: 🧹 Nitpick (assertive)
Consider extracting template configurations to a separate file.
The large array of post type configurations makes the method lengthy and harder to maintain. Consider:
- Moving configurations to a separate JSON/PHP config file
- Adding constants for repeated strings like 'tour-operator'
Would you like me to help create a separate configuration file structure?
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 56-56: includes/classes/blocks/class-templates.php#L56
Array double arrow not aligned correctly; expected 3 space(s) between "'archive-destination'" and double arrow, but found 1. (WordPress.Arrays.MultipleStatementAlignment-[fixable])
[warning] 44-44: includes/classes/blocks/class-templates.php#L44
Expected 1 space before the array closer in a single line array. Found: no spaces. (NormalizedArrays.Arrays.ArrayBraceSpacing-[fixable])
[warning] 97-97: includes/classes/blocks/class-templates.php#L97
Short array syntax is not allowed. (Universal.Arrays.DisallowShortArraySyntax-[fixable])
[notice] 61-61: includes/classes/blocks/class-templates.php#L61
Array double arrow not aligned correctly; expected 11 space(s) between "'single-tour'" and double arrow, but found 2. (WordPress.Arrays.MultipleStatementAlignment-[fixable])
[warning] 49-49: includes/classes/blocks/class-templates.php#L49
Expected 1 space before the array closer in a single line array. Found: no spaces. (NormalizedArrays.Arrays.ArrayBraceSpacing-[fixable])
40-101: 🧹 Nitpick (assertive)
Maintain consistent array formatting.
Several array formatting inconsistencies were detected:
- Inconsistent spacing around array double arrows
- Inconsistent array syntax (short vs long)
Consider running PHP Code Sniffer to automatically fix these formatting issues.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 56-56: includes/classes/blocks/class-templates.php#L56
Array double arrow not aligned correctly; expected 3 space(s) between "'archive-destination'" and double arrow, but found 1. (WordPress.Arrays.MultipleStatementAlignment-[fixable])
[warning] 44-44: includes/classes/blocks/class-templates.php#L44
Expected 1 space before the array closer in a single line array. Found: no spaces. (NormalizedArrays.Arrays.ArrayBraceSpacing-[fixable])
[warning] 97-97: includes/classes/blocks/class-templates.php#L97
Short array syntax is not allowed. (Universal.Arrays.DisallowShortArraySyntax-[fixable])
[notice] 61-61: includes/classes/blocks/class-templates.php#L61
Array double arrow not aligned correctly; expected 11 space(s) between "'single-tour'" and double arrow, but found 2. (WordPress.Arrays.MultipleStatementAlignment-[fixable])
[warning] 49-49: includes/classes/blocks/class-templates.php#L49
Expected 1 space before the array closer in a single line array. Found: no spaces. (NormalizedArrays.Arrays.ArrayBraceSpacing-[fixable])
19-28: 🧹 Nitpick (assertive)
Fix access level documentation inconsistency.
The method is documented as private but implemented as public. Update the documentation to match the implementation.
/** * Initialize the plugin by setting localization, filters, and administration functions. * * @since 1.0.0 * - * @access private + * @access public */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Initialize the plugin by setting localization, filters, and administration functions. * * @since 1.0.0 * * @access public */ public function __construct() { add_action( 'init', [ $this, 'register_post_type_templates' ] ); }
12-17: 🧹 Nitpick (assertive)
Enhance property documentation with array structure details.
The
$templatesproperty documentation could be more specific about the expected array structure. Consider adding a code example or describing the array format./** * Holds array of out templates to be registered. + * + * Expected array structure: + * [ + * 'template-slug' => [ + * 'title' => string, + * 'description' => string, + * 'post_types' => array (optional) + * ] + * ] * * @var array */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Holds array of out templates to be registered. * * Expected array structure: * [ * 'template-slug' => [ * 'title' => string, * 'description' => string, * 'post_types' => array (optional) * ] * ] * * @var array */ public $templates = [];
116-126: 🛠️ Refactor suggestion
Improve method type safety and documentation.
The method documentation and implementation need improvements:
- Incorrect return type documentation
- Missing parameter type hint
- No validation of template content
/** * Gets the PHP template file and returns the content. * - * @param [type] $template - * @return void + * @param string $template The template file name + * @return string The template content + * @throws \RuntimeException If template file is not readable */ - protected function get_template_content( $template ) { + protected function get_template_content( string $template ): string { + $template_file = LSX_TO_PATH . "/templates/{$template}"; + if ( ! is_readable( $template_file ) ) { + throw new \RuntimeException( sprintf( 'Template file not readable: %s', $template_file ) ); + } ob_start(); - include LSX_TO_PATH . "/templates/{$template}"; + include $template_file; return ob_get_clean(); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Gets the PHP template file and returns the content. * * @param string $template The template file name * @return string The template content * @throws \RuntimeException If template file is not readable */ protected function get_template_content( string $template ): string { $template_file = LSX_TO_PATH . "/templates/{$template}"; if ( ! is_readable( $template_file ) ) { throw new \RuntimeException( sprintf( 'Template file not readable: %s', $template_file ) ); } ob_start(); include $template_file; return ob_get_clean(); }🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 120-120: includes/classes/blocks/class-templates.php#L120
Function return type is void, but function contains return statement. (Squiz.Commenting.FunctionComment)
103-114:
⚠️ Potential issueAdd error handling for template registration.
The template registration lacks error handling for:
- Missing template files
- Failed template registration
foreach ( $post_types as $key => $labels ) { + $template_file = LSX_TO_PATH . "/templates/{$key}.html"; + if ( ! file_exists( $template_file ) ) { + error_log( sprintf( 'Template file missing: %s', $template_file ) ); + continue; + } $args = [ 'title' => $labels['title'], 'description' => $labels['description'], 'content' => $this->get_template_content( $key . '.html' ), ]; if ( isset( $labels['post_types'] ) ) { $args['post_types'] = $labels['post_types']; } - register_block_template( 'lsx-tour-operator//' . $key, $args ); + $result = register_block_template( 'lsx-tour-operator//' . $key, $args ); + if ( is_wp_error( $result ) ) { + error_log( sprintf( 'Failed to register template %s: %s', $key, $result->get_error_message() ) ); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.foreach ( $post_types as $key => $labels ) { $template_file = LSX_TO_PATH . "/templates/{$key}.html"; if ( ! file_exists( $template_file ) ) { error_log( sprintf( 'Template file missing: %s', $template_file ) ); continue; } $args = [ 'title' => $labels['title'], 'description' => $labels['description'], 'content' => $this->get_template_content( $key . '.html' ), ]; if ( isset( $labels['post_types'] ) ) { $args['post_types'] = $labels['post_types']; } $result = register_block_template( 'lsx-tour-operator//' . $key, $args ); if ( is_wp_error( $result ) ) { error_log( sprintf( 'Failed to register template %s: %s', $key, $result->get_error_message() ) ); } } }includes/constants/settings-fields.php (6)
22-53: 🧹 Nitpick (assertive)
Add data-saving options for low-bandwidth areas
Whilst the maps_disabled option is good, consider adding more granular controls for areas with limited connectivity:
- Option to load maps on demand
- Lower resolution map tiles for slower connections
- Offline map caching capabilities
54-73: 🛠️ Refactor suggestion
Implement image optimisation for placeholders
For improved performance, especially in areas with limited bandwidth:
- Add file size restrictions
- Include WebP support
- Specify recommended image dimensions
74-98:
⚠️ Potential issueCritical: Google Fusion Tables service is deprecated
Google Fusion Tables service has been discontinued. Consider:
- Removing these settings entirely
- Migrating to Google Maps Data Layer
- Using alternative solutions like GeoJSON
99-104: 🛠️ Refactor suggestion
Enhance Google Maps API key configuration
Please add:
- API key format validation
- Usage instructions
- Rate limit warnings
- Billing information notice
105-140: 🛠️ Refactor suggestion
Eliminate duplicate placeholder settings
The placeholder settings are duplicated from the main placeholder section. Consider:
- Creating a reusable configuration array
- Using array_merge() for extending settings
- Implementing a helper function for common settings
// Example approach: $placeholder_defaults = array( 'featured_placeholder' => array(/*...*/), 'map_placeholder_enabled' => array(/*...*/), 'map_placeholder' => array(/*...*/), ); $settings_fields['post_types']['placeholder'] = array_merge( $placeholder_defaults, array(/* post-type specific overrides */) );
3-21: 🧹 Nitpick (assertive)
Consider adding more African currencies for regional tourism
The currency settings are well-structured, with ZAR as the default. However, for regional tourism operations, consider adding these African currencies:
- BWP (Botswanan Pula)
- MZN (Mozambican Metical)
- ZMW (Zambian Kwacha)
'options' => array( 'USD' => esc_html__( 'USD (united states dollar)', 'tour-operator' ), 'GBP' => esc_html__( 'GBP (british pound)', 'tour-operator' ), 'ZAR' => esc_html__( 'ZAR (south african rand)', 'tour-operator' ), 'NAD' => esc_html__( 'NAD (namibian dollar)', 'tour-operator' ), + 'BWP' => esc_html__( 'BWP (botswanan pula)', 'tour-operator' ), + 'MZN' => esc_html__( 'MZN (mozambican metical)', 'tour-operator' ), + 'ZMW' => esc_html__( 'ZMW (zambian kwacha)', 'tour-operator' ), 'CAD' => esc_html__( 'CAD (canadian dollar)', 'tour-operator' ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'currency' => array( 'currency' => array( 'label' => esc_html__( 'Currency', 'tour-operator' ), 'type' => 'select', 'default' => 'ZAR', 'options' => array( 'USD' => esc_html__( 'USD (united states dollar)', 'tour-operator' ), 'GBP' => esc_html__( 'GBP (british pound)', 'tour-operator' ), 'ZAR' => esc_html__( 'ZAR (south african rand)', 'tour-operator' ), 'NAD' => esc_html__( 'NAD (namibian dollar)', 'tour-operator' ), 'BWP' => esc_html__( 'BWP (botswanan pula)', 'tour-operator' ), 'MZN' => esc_html__( 'MZN (mozambican metical)', 'tour-operator' ), 'ZMW' => esc_html__( 'ZMW (zambian kwacha)', 'tour-operator' ), 'CAD' => esc_html__( 'CAD (canadian dollar)', 'tour-operator' ), 'EUR' => esc_html__( 'EUR (euro)', 'tour-operator' ), 'HKD' => esc_html__( 'HKD (hong kong dollar)', 'tour-operator' ), 'SGD' => esc_html__( 'SGD (singapore dollar)', 'tour-operator' ), 'NZD' => esc_html__( 'NZD (new zealand dollar)', 'tour-operator' ), 'AUD' => esc_html__( 'AUD (australian dollar)', 'tour-operator' ), ), ), ),includes/classes/legacy/class-itinerary-query.php (3)
154-158: 🧹 Nitpick (assertive)
Remove or document commented code
The commented code block appears to be important functionality for handling room and parent images. Either remove it if it's obsolete or document why it's temporarily disabled.
173-179: 🧹 Nitpick (assertive)
Consider renaming method for clarity
The method name
append_room_imagessuggests it modifies an existing array, but it returns a new merged array. Consider renaming toget_room_images_mergedor similar for better clarity.
103-106:
⚠️ Potential issuePotential type mismatch in itineraries handling
The change to
get_post_meta(..., true)could return a single value instead of an array, but the subsequent code assumes an array type. This might cause issues when$this->itinerariescontains a single value.Apply this diff to handle both single values and arrays:
$this->itineraries = get_post_meta( $this->post_id, 'itinerary', true ); +if ( ! empty( $this->itineraries ) ) { + $this->itineraries = is_array( $this->itineraries ) ? $this->itineraries : array( $this->itineraries ); if ( is_array( $this->itineraries ) && ! empty( $this->itineraries ) ) { $this->has_itinerary = true; $this->count = count( $this->itineraries ); } +}Committable suggestion skipped: line range outside the PR's diff.
includes/metaboxes/config-destination.php (4)
22-22: 🧹 Nitpick (assertive)
Well-structured travel information fields with clear descriptions
The new WYSIWYG fields for travel information are comprehensive and include helpful descriptions that guide content editors. The structure follows a logical flow from essential information (electricity, banking) to general travel advice.
However, consider adding character or word limits to prevent overly lengthy content.
Add length constraints to the WYSIWYG fields:
'options' => array( 'editor_height' => '100', + 'textarea_rows' => 5, + 'teeny' => true, ),Also applies to: 32-32, 42-42, 52-52, 62-62, 72-72, 82-82, 92-92, 102-102, 112-112
181-189: 🧹 Nitpick (assertive)
Related content selection needs performance optimization
The switch to 'pw_multiselect' is good for usability, but large datasets might affect performance.
Consider implementing these optimizations:
- Add pagination to the selection queries
- Implement AJAX loading for large datasets
- Cache frequently accessed options
Example implementation:
'type' => 'pw_multiselect', -'use_ajax' => false, +'use_ajax' => true, +'ajax_args' => array( + 'posts_per_page' => 10, + 'cache_results' => true +),Also applies to: 194-202, 207-214
160-169: 🧹 Nitpick (assertive)
Map placeholder configuration needs improvement
The file type restrictions are good, but the configuration could be more robust.
Add image dimension constraints and WebP support:
'query_args' => array( 'type' => array( 'image/gif', 'image/jpeg', 'image/png', + 'image/webp', ), ), +'image_size_limit' => array( + 'min_width' => 800, + 'min_height' => 600, +),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'type' => 'file', 'repeatable' => false, 'show_size' => false, 'query_args' => array( 'type' => array( 'image/gif', 'image/jpeg', 'image/png', 'image/webp', ), ), 'image_size_limit' => array( 'min_width' => 800, 'min_height' => 600, ),
125-133: 🛠️ Refactor suggestion
Gallery implementation needs additional validation
The switch to 'file_list' type is appropriate, but the configuration could benefit from additional safeguards.
Add size and file count limitations:
'type' => 'file_list', 'preview_size' => 'thumbnail', 'query_args' => array( 'type' => 'image' ), +'max_files' => 10, +'file_size_limit' => 5242880, // 5MB in bytes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'name' => esc_html__( 'Gallery', 'tour-operator' ), 'id' => 'gallery', 'type' => 'file_list', 'desc' => esc_html__( 'Add images related to the country to be displayed in the Destination's gallery.', 'tour-operator' ), 'preview_size' => 'thumbnail', // Image size to use when previewing in the admin. 'query_args' => array( 'type' => 'image' ), // Only images attachment 'max_files' => 10, 'file_size_limit' => 5242880, // 5MB in bytes 'text' => array( 'add_upload_files_text' => esc_html__( 'Add new image', 'tour-operator' ), // default: "Add or Upload Files" ),includes/metaboxes/config-accommodation.php (5)
176-185: 🧹 Nitpick (assertive)
Enhance related content selection interface
The related content selectors could be more user-friendly with additional features:
'type' => 'pw_multiselect', 'use_ajax' => false, 'repeatable' => false, 'allow_none' => true, 'options' => array( 'post_type_args' => 'post', + 'search_text' => esc_html__( 'Search posts...', 'tour-operator' ), + 'show_thumbnails' => true, + 'filter_by' => array( + 'category' => true, + 'date' => true + ), + 'preview' => array( + 'show' => true, + 'size' => 'thumbnail' + ) ),Apply similar improvements to
destination_to_accommodationandtour_to_accommodationfields.Also applies to: 189-198, 202-211
98-106: 🧹 Nitpick (assertive)
Enhance the media gallery functionality
The gallery configuration could be more robust with:
- Batch upload support for multiple images
- Categories or tags for better media organisation
- Sorting capabilities
'name' => esc_html__( 'Gallery', 'tour-operator' ), 'desc' => esc_html__( 'Add images related to the accommodation to be displayed in the Accommodation\'s gallery.', 'tour-operator' ), 'id' => 'gallery', 'type' => 'file_list', 'preview_size' => 'thumbnail', 'query_args' => array( 'type' => 'image' ), +'sanitization_cb' => 'sanitize_gallery', 'text' => array( 'add_upload_files_text' => esc_html__( 'Add new image', 'tour-operator' ), + 'bulk_upload_text' => esc_html__( 'Bulk Upload', 'tour-operator' ), ), +'options' => array( + 'enable_sorting' => true, + 'categories' => true +),Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 103-103: includes/metaboxes/config-accommodation.php#L103
Tabs must be used to indent lines; spaces are not allowed. (Generic.WhiteSpace.DisallowSpaceIndent-[fixable])
72-88: 🧹 Nitpick (assertive)
Consider enhancing the map placeholder configuration, mate
The map placeholder configuration could use some improvements:
- Add file size limits to prevent massive image uploads
- Add image dimension validation
- Include an alt text field for accessibility
'id' => 'map_placeholder', 'name' => esc_html__( 'Map Placeholder', 'tour-operator' ), 'desc' => esc_html__( 'A placeholder image for the map if no address or GPS data is available.', 'tour-operator' ), 'type' => 'file', 'repeatable' => false, 'show_size' => false, 'preview_size' => 'thumbnail', 'query_args' => array( 'type' => array( 'image/gif', 'image/jpeg', 'image/png', ), + 'post_mime_type' => array('image'), + 'posts_per_page' => 1, ), 'options' => array( - 'url' => false, // Hide the text input for the url + 'url' => false, + 'max_file_size' => '2MB', + 'max_width' => 1920, + 'max_height' => 1080 ), +'fields' => array( + array( + 'name' => esc_html__( 'Alt Text', 'tour-operator' ), + 'id' => 'alt_text', + 'type' => 'text', + ) +),Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 81-81: includes/metaboxes/config-accommodation.php#L81
Found precision alignment of 1 spaces. (Universal.WhiteSpace.PrecisionAlignment-[fixable])
[warning] 86-86: includes/metaboxes/config-accommodation.php#L86
Inline comments must end in full-stops, exclamation marks, or question marks. (Squiz.Commenting.InlineComment)
151-154: 🛠️ Refactor suggestion
Add validation and additional fields for unit pricing
The price field needs proper validation and additional pricing-related fields:
'id' => 'price', 'name' => esc_html__( 'Unit Price', 'tour-operator' ), 'desc' => esc_html__( 'Enter the price of the unit.', 'tour-operator' ), -'type' => 'text', +'type' => 'text_money', +'before_field' => 'R', +'attributes' => array( + 'type' => 'number', + 'min' => '0', + 'step' => '0.01' +), +'sanitization_cb' => 'sanitize_price',Committable suggestion skipped: line range outside the PR's diff.
129-134: 🛠️ Refactor suggestion
Add occupancy and amenities information to unit configuration
The unit type configuration should include essential accommodation details:
array( 'id' => 'type', 'name' => esc_html__( 'Unit Type', 'tour-operator' ), 'desc' => esc_html__( 'Select the type of unit (e.g., room, suite) from the dropdown.', 'tour-operator' ), 'type' => 'select', 'options' => \lsx\legacy\Accommodation::get_instance()->unit_types, ), +'array( + 'id' => 'occupancy', + 'name' => esc_html__( 'Occupancy', 'tour-operator' ), + 'type' => 'group', + 'fields' => array( + array( + 'id' => 'min', + 'name' => esc_html__( 'Minimum', 'tour-operator' ), + 'type' => 'text_small', + 'attributes' => array('type' => 'number', 'min' => '1') + ), + array( + 'id' => 'max', + 'name' => esc_html__( 'Maximum', 'tour-operator' ), + 'type' => 'text_small', + 'attributes' => array('type' => 'number', 'min' => '1') + ) + ) +), +'array( + 'id' => 'amenities', + 'name' => esc_html__( 'Amenities', 'tour-operator' ), + 'type' => 'multicheck', + 'options' => array( + 'wifi' => esc_html__( 'Wi-Fi', 'tour-operator' ), + 'aircon' => esc_html__( 'Air Conditioning', 'tour-operator' ), + 'minibar' => esc_html__( 'Mini Bar', 'tour-operator' ), + 'tv' => esc_html__( 'Television', 'tour-operator' ), + 'safe' => esc_html__( 'Safe', 'tour-operator' ), + 'balcony' => esc_html__( 'Balcony/Patio', 'tour-operator' ) + ) +),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'id' => 'type', 'name' => esc_html__( 'Unit Type', 'tour-operator' ), 'desc' => esc_html__( 'Select the type of unit (e.g., room, suite) from the dropdown.', 'tour-operator' ), 'type' => 'select', 'options' => \lsx\legacy\Accommodation::get_instance()->unit_types, ), array( 'id' => 'occupancy', 'name' => esc_html__( 'Occupancy', 'tour-operator' ), 'type' => 'group', 'fields' => array( array( 'id' => 'min', 'name' => esc_html__( 'Minimum', 'tour-operator' ), 'type' => 'text_small', 'attributes' => array('type' => 'number', 'min' => '1') ), array( 'id' => 'max', 'name' => esc_html__( 'Maximum', 'tour-operator' ), 'type' => 'text_small', 'attributes' => array('type' => 'number', 'min' => '1') ) ) ), array( 'id' => 'amenities', 'name' => esc_html__( 'Amenities', 'tour-operator' ), 'type' => 'multicheck', 'options' => array( 'wifi' => esc_html__( 'Wi-Fi', 'tour-operator' ), 'aircon' => esc_html__( 'Air Conditioning', 'tour-operator' ), 'minibar' => esc_html__( 'Mini Bar', 'tour-operator' ), 'tv' => esc_html__( 'Television', 'tour-operator' ), 'safe' => esc_html__( 'Safe', 'tour-operator' ), 'balcony' => esc_html__( 'Balcony/Patio', 'tour-operator' ) ) ),includes/classes/legacy/class-schema.php (1)
124-125: 🧹 Nitpick (assertive)
Consider adding fallback for backwards compatibility.
Since this is a breaking change in the data structure, consider adding a fallback mechanism for backwards compatibility.
Here's a suggested implementation:
- $lat_address = $address_accommodation['latitude']; - $long_address = $address_accommodation['longitude']; + $lat_address = isset($address_accommodation['latitude']) ? $address_accommodation['latitude'] : + (isset($address_accommodation['lat']) ? $address_accommodation['lat'] : null); + $long_address = isset($address_accommodation['longitude']) ? $address_accommodation['longitude'] : + (isset($address_accommodation['long']) ? $address_accommodation['long'] : null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$lat_address = isset($address_accommodation['latitude']) ? $address_accommodation['latitude'] : (isset($address_accommodation['lat']) ? $address_accommodation['lat'] : null); $long_address = isset($address_accommodation['longitude']) ? $address_accommodation['longitude'] : (isset($address_accommodation['long']) ? $address_accommodation['long'] : null);includes/patterns/itinerary-card.php (5)
2-8: 🧹 Nitpick (assertive)
Please add a meaningful description for the block pattern.
The empty description string reduces discoverability in the block inserter. Consider adding a brief description that explains the purpose and use case of this itinerary card pattern.
19-21: 🛠️ Refactor suggestion
Replace hardcoded day number with a dynamic placeholder.
The heading contains a static "Day 1" text which should be replaced with a dynamic value.
Apply this diff:
-<h3 class="wp-block-heading alignwide itinerary-title has-primary-color has-text-color has-link-color">Day 1 (itinerary day)</h3> +<h3 class="wp-block-heading alignwide itinerary-title has-primary-color has-text-color has-link-color">Day [n] (itinerary day)</h3>Committable suggestion skipped: line range outside the PR's diff.
26-28: 🧹 Nitpick (assertive)
Enhance the description placeholder text.
Consider using more descriptive placeholder text to better guide content editors.
Apply this diff:
-<p class="itinerary-description has-septenary-color has-text-color has-link-color"><strong>Itinerary Description</strong></p> +<p class="itinerary-description has-septenary-color has-text-color has-link-color"><strong>Enter a detailed description of the day's activities and highlights here...</strong></p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div class="wp-block-column" style="flex-basis:66.66%"><!-- wp:paragraph {"className":"itinerary-description","style":{"elements":{"link":{"color":{"text":"var:preset|color|septenary"}}}},"textColor":"septenary"} --> <p class="itinerary-description has-septenary-color has-text-color has-link-color"><strong>Enter a detailed description of the day's activities and highlights here...</strong></p> <!-- /wp:paragraph --></div>
11-13:
⚠️ Potential issueReplace hardcoded image values with placeholders.
The current implementation has several issues:
- Hardcoded image ID (2981) will not exist in other environments
- Development URL (beta.local) is not suitable for production
- Missing alt text affects accessibility
Apply this diff to use proper placeholders:
-<figure class="wp-block-image size-full itinerary-image"><img src="https://beta.local/wp-content/uploads/2024/09/R0I3460.jpg" alt="" class="wp-image-2981" title=""/></figure> +<figure class="wp-block-image size-full itinerary-image"><img src="[placeholder-image]" alt="Itinerary day image" class="wp-image-[id]"/></figure>Committable suggestion skipped: line range outside the PR's diff.
32-111: 🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Replace icon placeholders with actual icon blocks.
The details section contains multiple instances of "Icon" text placeholders. These should be replaced with proper WordPress icon blocks or SVG icons for:
- Location
- Accommodation
- Type
- Drinks Basis
- Room Basis
Example replacement for one icon (repeat for others):
-<p class="has-senary-color has-text-color has-link-color"><strong>Icon</strong></p> +<!-- wp:lsx/icon {"icon":"map-marker-alt","className":"has-senary-color"} /-->Additionally, consider extracting the repeated group structure into a reusable template part to improve maintainability.
Committable suggestion skipped: line range outside the PR's diff.
includes/metaboxes/config-tour.php (3)
278-283: 🧹 Nitpick (assertive)
Enable itinerary sorting functionality
The itinerary entries are not sortable, which might limit tour operators' ability to reorganize days.
Enable sorting for better user experience:
'options' => array( 'group_title' => __( 'Itinerary {#}', 'tour-operator' ), 'add_button' => __( 'Add Another', 'tour-operator' ), 'remove_button' => __( 'Remove', 'tour-operator' ), - 'sortable' => false, + 'sortable' => true, ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'options' => array( 'group_title' => __( 'Itinerary {#}', 'tour-operator' ), // since version 1.1.4, {#} gets replaced by row number 'add_button' => __( 'Add Another', 'tour-operator' ), 'remove_button' => __( 'Remove', 'tour-operator' ), 'sortable' => true, ),
116-126: 🧹 Nitpick (assertive)
Add file size restrictions for map placeholder
The file upload configuration allows image types but lacks size restrictions, which could impact performance.
Add size restrictions to the query args:
'query_args' => array( 'type' => array( 'image/gif', 'image/jpeg', 'image/png', ), + 'post_mime_type' => array('image'), + 'posts_per_page' => 1, + 'post_status' => 'inherit', + 'max_file_size' => 5 * 1024 * 1024, // 5MB limit ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'desc' => esc_html__( 'A placeholder image for the map if no address or GPS data is available.', 'tour-operator' ), 'type' => 'file', 'repeatable' => false, 'show_size' => false, 'query_args' => array( 'type' => array( 'image/gif', 'image/jpeg', 'image/png', ), 'post_mime_type' => array('image'), 'posts_per_page' => 1, 'post_status' => 'inherit', 'max_file_size' => 5 * 1024 * 1024, // 5MB limit ),🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 125-125: includes/metaboxes/config-tour.php#L125
Array closer not aligned correctly; expected 12 space(s) but found 11. (WordPress.Arrays.ArrayIndentation-[fixable])
145-153: 🧹 Nitpick (assertive)
Consider adding image size recommendations
The gallery configuration could benefit from guidance on optimal image dimensions.
Update the description to include size recommendations:
-'desc' => esc_html__( 'Add images related to the tour to be displayed in the Tour gallery.', 'tour-operator' ), +'desc' => esc_html__( 'Add images (recommended size: 1200x800px, max 2MB) related to the tour to be displayed in the Tour gallery.', 'tour-operator' ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'name' => esc_html__( 'Gallery', 'tour-operator' ), 'desc' => esc_html__( 'Add images (recommended size: 1200x800px, max 2MB) related to the tour to be displayed in the Tour gallery.', 'tour-operator' ), 'id' => 'gallery', 'type' => 'file_list', 'preview_size' => 'thumbnail', // Image size to use when previewing in the admin. 'query_args' => array( 'type' => 'image' ), // Only images attachment 'text' => array( 'add_upload_files_text' => esc_html__( 'Add new image', 'tour-operator' ), // default: "Add or Upload Files" ),🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 147-147: includes/metaboxes/config-tour.php#L147
Array double arrow not aligned correctly; expected 11 space(s) between "'id'" and double arrow, but found 3. (WordPress.Arrays.MultipleStatementAlignment-[fixable])assets/js/src/custom.js (1)
142-142: 🛠️ Refactor suggestion
Consider using a more robust i18n approach for multilingual support
While adding support for Dutch is good, hardcoding the string 'Lees verder' might not be the best approach for internationalisation. Consider using WordPress's i18n functions or a translation object to handle multiple languages more effectively.
Here's a suggested improvement:
- if ( 'Read More' === $( this ).html() || 'Lees verder' === $( this ).html() ) { + if ( lsx_to_params.read_more_strings.includes( $( this ).html() ) ) {You'll need to define
read_more_stringsin your localised script data:// In your PHP file where you localise the script wp_localize_script( 'lsx-to-script', 'lsx_to_params', array( 'read_more_strings' => array( __( 'Read More', 'tour-operator' ), __( 'Lees verder', 'tour-operator' ) ), // ... other params ) );includes/classes/legacy/schema/class-lsx-to-schema-graph-piece.php (1)
468-473: 🧹 Nitpick (assertive)
Coordinate key names standardisation looks good, consider adding value validation
The change to use
latitudeandlongitudeas keys is a good improvement that aligns with standard geographical coordinate naming conventions. However, consider adding validation to ensure the coordinate values are within valid ranges (latitude: -90 to 90, longitude: -180 to 180).Here's a suggested improvement:
if ( isset( $value['latitude'] ) && isset( $value['longitude'] ) ) { + $latitude = floatval($value['latitude']); + $longitude = floatval($value['longitude']); + + if ($latitude >= -90 && $latitude <= 90 && $longitude >= -180 && $longitude <= 180) { $data[ $data_key ] = array( '@type' => 'GeoCoordinates', - 'latitude' => $value['latitude'], - 'longitude' => $value['longitude'], + 'latitude' => $latitude, + 'longitude' => $longitude, ); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( isset( $value['latitude'] ) && isset( $value['longitude'] ) ) { $latitude = floatval($value['latitude']); $longitude = floatval($value['longitude']); if ($latitude >= -90 && $latitude <= 90 && $longitude >= -180 && $longitude <= 180) { $data[ $data_key ] = array( '@type' => 'GeoCoordinates', 'latitude' => $latitude, 'longitude' => $longitude, ); } }includes/classes/legacy/class-maps.php (1)
167-168: 🛠️ Refactor suggestion
Parameter renaming improves clarity but needs compatibility consideration
The renaming from 'lat'/'long' to 'latitude'/'longitude' enhances code readability and aligns with geographical coordinate naming conventions. However, this change might affect existing implementations.
Consider implementing backwards compatibility by:
- Adding parameter aliases
- Deprecating old parameter names gradually
public function map_output( $post_id = false, $args = array() ) { $defaults = array( - 'latitude' => '-33.914482', - 'longitude' => '18.3758789', + 'latitude' => '-33.914482', // @since 2.0 + 'longitude' => '18.3758789', // @since 2.0 + 'lat' => '-33.914482', // @deprecated 2.0 Use 'latitude' instead + 'long' => '18.3758789', // @deprecated 2.0 Use 'longitude' instead ); $args = wp_parse_args( $args, $defaults ); + // Handle deprecated parameters + if ( isset( $args['lat'] ) ) { + $args['latitude'] = $args['lat']; + trigger_error( 'Parameter "lat" is deprecated since version 2.0. Use "latitude" instead.', E_USER_DEPRECATED ); + } + if ( isset( $args['long'] ) ) { + $args['longitude'] = $args['long']; + trigger_error( 'Parameter "long" is deprecated since version 2.0. Use "longitude" instead.', E_USER_DEPRECATED ); + }Also applies to: 205-205, 212-212, 238-238, 252-252, 265-265
changelog.md (1)
3-10: 🛠️ Refactor suggestion
Fix formatting issues in the 2.0.0 section
The formatting in the 2.0.0 section needs attention:
- Links are using incorrect Markdown syntax
- Missing blank lines around headings and lists
Apply these changes:
## [[2.0.0]](https://github.com/lightspeeddevelopment/tour-operator/releases/tag/2.0.0) - IN DEVELOPMENT ### Removed - UIX vendor library handling the TO Settings and refactored the settings code - [PR #332](https://github.com/lightspeedwp/tour-operator/pull/332) - Various settings which are now handled by Block and Site Editor options. - [PR #332](https://github.com/lightspeedwp/tour-operator/pull/332) ### Security - General testing to ensure compatibility with latest WordPress version (6.6.1).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.## [[2.0.0]](https://github.com/lightspeeddevelopment/tour-operator/releases/tag/2.0.0) - IN DEVELOPMENT ### Removed - UIX vendor library handling the TO Settings and refactored the settings code - [PR #332](https://github.com/lightspeedwp/tour-operator/pull/332) - Various settings which are now handled by Block and Site Editor options. - [PR #332](https://github.com/lightspeedwp/tour-operator/pull/332) ### Security - General testing to ensure compatibility with latest WordPress version (6.6.1).🧰 Tools
🪛 Markdownlint
6-6: (PR #332)[https://github.com//pull/332]
Reversed link syntax(MD011, no-reversed-links)
7-7: (PR #332)[https://github.com//pull/332]
Reversed link syntax(MD011, no-reversed-links)
5-5: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
9-9: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
6-6: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
10-10: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
includes/classes/legacy/class-frontend.php (1)
223-223:
⚠️ Potential issueCritical: Script enqueue is commented out but still being referenced
The tour-operator script enqueue is commented out, but the script parameters are still being localized on line 258. This will cause JavaScript errors as wp_localize_script() requires the script to be enqueued first.
Either:
- Remove the comment to re-enable the script:
-//wp_enqueue_script( 'tour-operator-script', LSX_TO_URL . 'assets/js/' . $prefix . 'custom' . $suffix . '.js', array( 'jquery', 'slick', 'slick-lightbox'/*, 'fixto'*/ ), LSX_TO_VER, true ); +wp_enqueue_script( 'tour-operator-script', LSX_TO_URL . 'assets/js/' . $prefix . 'custom' . $suffix . '.js', array( 'jquery', 'slick', 'slick-lightbox' ), LSX_TO_VER, true );
- Or remove both the commented script enqueue and the wp_localize_script call if the script is no longer needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.wp_enqueue_script( 'tour-operator-script', LSX_TO_URL . 'assets/js/' . $prefix . 'custom' . $suffix . '.js', array( 'jquery', 'slick', 'slick-lightbox' ), LSX_TO_VER, true );includes/classes/legacy/class-tour-operator.php (3)
183-183:
⚠️ Potential issueBreaking change: Option key modification requires data migration
The change from '_lsx-to_settings' to 'lsx_to_settings' will cause existing installations to lose their settings. A data migration strategy is required.
Consider adding migration code like this:
$old_options = get_option('_lsx-to_settings', array()); if (!empty($old_options)) { update_option('lsx_to_settings', $old_options); delete_option('_lsx-to_settings'); }
369-371:
⚠️ Potential issueBreaking change: Google Maps API key structure modified
The Google Maps API key is now accessed directly from options instead of through the 'api' array. This change will break existing map functionality for current installations.
Add migration code to handle the restructuring:
$options = get_option('lsx_to_settings', array()); if (isset($options['api']['googlemaps_key'])) { $options['googlemaps_key'] = $options['api']['googlemaps_key']; unset($options['api']['googlemaps_key']); update_option('lsx_to_settings', $options); }
983-983:
⚠️ Potential issueCritical: Theme compatibility notice disabled
The commented-out admin notice prevents users from being informed about LSX theme compatibility requirements. This could lead to support issues if users install the plugin with incompatible themes.
Restore the admin notice or implement a better compatibility check mechanism:
-//add_action( 'admin_notices', array( $this, 'compatible_theme_notice' ), 199 ); +add_action( 'admin_notices', array( $this, 'compatible_theme_notice' ), 199 );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.add_action( 'admin_notices', array( $this, 'compatible_theme_notice' ), 199 );includes/layout.php (1)
83-83: 💡 Codebase verification
Change from 'lsx-thumbnail-square' to 'medium' is inconsistent with theme's thumbnail usage
The change to 'medium' size deviates from the theme's established thumbnail sizing convention. The codebase consistently uses custom LSX thumbnail sizes ('lsx-thumbnail-wide', 'lsx-thumbnail-single') across templates and widgets. Reverting to WordPress's default 'medium' size may cause visual inconsistencies.
includes/layout.php: Other sections use 'lsx-thumbnail-wide'templates/content-*.php: All widget templates use 'lsx-thumbnail-wide'templates/content-modal.php: Uses 'lsx-thumbnail-single'Consider maintaining consistency by using one of the theme's custom thumbnail sizes instead of 'medium'.
🔗 Analysis chain
Verify thumbnail size change impact
The change from 'lsx-thumbnail-square' to 'medium' might affect image aspect ratios and layout consistency. Please ensure this change maintains visual consistency across team member displays.
Let me gather more information about the thumbnail usage and related functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other uses of lsx-thumbnail-square size and potential layout issues # Search for other instances of the thumbnail size rg -A 2 "lsx-thumbnail-square" # Search for any custom thumbnail size definitions ast-grep --pattern 'add_image_size($_, "lsx-thumbnail-square"'Length of output: 323
Script:
#!/bin/bash # Check for lsx_thumbnail function definition and usage ast-grep --pattern 'function lsx_thumbnail' # Search for other thumbnail size usages in the codebase rg -A 2 'lsx_thumbnail\(' # Check if there are any image size registrations rg -A 2 'add_image_size'Length of output: 1608
includes/partials/general.php (1)
8-10: 🛠️ Refactor suggestion
Remove unnecessary
<label>tags around<h3>elementsWrapping
<h3>elements inside<label>tags is semantically incorrect, as<label>tags are intended for form controls. Please remove the<label>tags to improve semantic HTML structure.Apply this diff to one of the sections (lines 8-10):
- <label> <h3><?php esc_html_e( 'Currency Settings', 'tour-operator' ); ?></h3> - </label> + <h3><?php esc_html_e( 'Currency Settings', 'tour-operator' ); ?></h3>Repeat similar changes for the other sections at lines 17-19, 26-28, 35-37, and 44-46.
Also applies to: 17-19, 26-28, 35-37, 44-46
includes/classes/admin/class-admin.php (2)
33-37:
⚠️ Potential issuePotential security risk: Allowing SVG file uploads
Allowing SVG uploads can introduce security vulnerabilities, as SVG files can contain malicious code. Consider sanitising SVG files or using a trusted SVG sanitisation library before allowing uploads.
56-60: 🛠️ Refactor suggestion
Escape URLs in plugin action links for security
To prevent potential security issues, it's recommended to use
esc_urlwhen outputting URLs in HTML attributes.Apply this diff to properly escape the URLs:
$mylinks = array( - '<a href="' . admin_url( 'admin.php?page=lsx-to-settings' ) . '">' . esc_html__( 'Settings', 'tour-operator' ) . '</a>', - '<a href="https://www.lsdev.biz/lsx/documentation/lsx-tour-operator/" target="_blank">' . esc_html__( 'Documentation', 'tour-operator' ) . '</a>', - '<a href="https://www.lsdev.biz/lsx/support/" target="_blank">' . esc_html__( 'Support', 'tour-operator' ) . '</a>', + '<a href="' . esc_url( admin_url( 'admin.php?page=lsx-to-settings' ) ) . '">' . esc_html__( 'Settings', 'tour-operator' ) . '</a>', + '<a href="' . esc_url( 'https://www.lsdev.biz/lsx/documentation/lsx-tour-operator/' ) . '" target="_blank">' . esc_html__( 'Documentation', 'tour-operator' ) . '</a>', + '<a href="' . esc_url( 'https://www.lsdev.biz/lsx/support/' ) . '" target="_blank">' . esc_html__( 'Support', 'tour-operator' ) . '</a>', );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$mylinks = array( '<a href="' . esc_url( admin_url( 'admin.php?page=lsx-to-settings' ) ) . '">' . esc_html__( 'Settings', 'tour-operator' ) . '</a>', '<a href="' . esc_url( 'https://www.lsdev.biz/lsx/documentation/lsx-tour-operator/' ) . '" target="_blank">' . esc_html__( 'Documentation', 'tour-operator' ) . '</a>', '<a href="' . esc_url( 'https://www.lsdev.biz/lsx/support/' ) . '" target="_blank">' . esc_html__( 'Support', 'tour-operator' ) . '</a>', );assets/js/blocks/linked-cover.js (3)
61-66: 🧹 Nitpick (assertive)
Improve accessibility by adding
helptextIncluding
helptext in theToggleControlenhances accessibility and provides users with additional context.You can update the
ToggleControlas follows:createElement(ToggleControl, { label: 'Open link in a new tab', checked: linkTarget === '_blank', + help: 'Select to open the link in a new browser tab.', onChange: function (value) { setAttributes({ linkTarget: value ? '_blank' : '_self' }); } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.label: 'Open link in a new tab', checked: linkTarget === '_blank', help: 'Select to open the link in a new browser tab.', onChange: function (value) { setAttributes({ linkTarget: value ? '_blank' : '_self' }); } })
17-26:
⚠️ Potential issueAvoid mutating the
settingsobject directlyMutating the
settingsobject can lead to unexpected side effects. Instead, return a new object to maintain immutability.Here's how you can modify the code:
function addLinkAttributes(settings, name) { if (name !== 'core/cover') { return settings; } - settings.attributes = Object.assign({}, settings.attributes, { + const newSettings = Object.assign({}, settings, { + attributes: Object.assign({}, settings.attributes, { linkUrl: { type: 'string', default: '' }, linkTarget: { type: 'string', default: '_self' // '_self' for same tab, '_blank' for new tab } + }) }); - return settings; + return newSettings; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const newSettings = Object.assign({}, settings, { attributes: Object.assign({}, settings.attributes, { linkUrl: { type: 'string', default: '' }, linkTarget: { type: 'string', default: '_self' // '_self' for same tab, '_blank' for new tab } }) });
76-92:
⚠️ Potential issueInclude
relattribute when usingtarget="_blank"for securityWhen
target="_blank"is used, addingrel="noopener noreferrer"is important to prevent security vulnerabilities.Here's the updated code:
if (linkUrl) { return createElement( 'a', - { href: linkUrl, target: linkTarget }, + Object.assign( + { href: linkUrl }, + linkTarget === '_blank' + ? { target: linkTarget, rel: 'noopener noreferrer' } + : { target: linkTarget } + ), element ); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function modifyCoverSaveElement(element, blockType, attributes) { if (blockType.name !== 'core/cover') { return element; } const { linkUrl, linkTarget } = attributes; if (linkUrl) { return createElement( 'a', Object.assign( { href: linkUrl }, linkTarget === '_blank' ? { target: linkTarget, rel: 'noopener noreferrer' } : { target: linkTarget } ), element ); } return element; }includes/classes/admin/class-setup.php (4)
46-52: 🛠️ Refactor suggestion
Correct the
@accessannotation in the constructor docblockThe constructor is declared as
public, but the docblock specifies@access private. Please update the@accessannotation topublicto accurately reflect the visibility of the constructor.Apply this diff to correct the docblock:
* - * @access private + * @access public */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Initialize the plugin by setting localization, filters, and administration functions. * * @since 1.0.0 * * @access public */
2-10: 🧹 Nitpick (assertive)
Update the file header to reflect the
SetupclassThe file header comments refer to 'LSX Tour Operator - Pages Class', which does not accurately describe the purpose of the
Setupclass. Please update the description to better reflect the functionality of this class.Apply this diff to update the file header:
/** - * LSX Tour Operator - Pages Class + * LSX Tour Operator - Setup Class *📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * LSX Tour Operator - Setup Class * * @package lsx * @author LightSpeed * @license GPL-2.0+ * @link * @copyright 2017 LightSpeedDevelopment */
64-68: 🛠️ Refactor suggestion
Update the
@returnannotation to match the correct classThe
@returnannotation in the docblock of theinit()method incorrectly referencesTour_Operator. It should be updated to\lsx\admin\Setupto reflect the actual class being returned.Apply this diff to correct the docblock:
/** - * @return Tour_Operator A single instance + * @return \lsx\admin\Setup A single instance */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.* Return an instance of this class. * * @since 1.1.0 * @return \lsx\admin\Setup A single instance */
136-142: 🧹 Nitpick (assertive)
Align array double arrows as per WordPress coding standards
Static analysis tools have flagged misalignment of array double arrows at lines 137 and 139. Please adjust the spacing to align the double arrows properly, adhering to the WordPress coding standards.
Apply this diff to fix the alignment:
$cmb[ $metabox_counter ] = new_cmb2_box( array( 'id' => 'lsx_to_metabox_' . $post_type . '_' . $metabox_counter, - 'title' => $field['name'], + 'title' => $field['name'], 'object_types' => array( $post_type ), // Post type - 'context' => 'normal', + 'context' => 'normal', 'priority' => 'high', 'show_names' => true, ) );Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 139-139: includes/classes/admin/class-setup.php#L139
Array double arrow not aligned correctly; expected 6 space(s) between "'context'" and double arrow, but found 7. (WordPress.Arrays.MultipleStatementAlignment-[fixable])
[notice] 137-137: includes/classes/admin/class-setup.php#L137
Array double arrow not aligned correctly; expected 8 space(s) between "'title'" and double arrow, but found 9. (WordPress.Arrays.MultipleStatementAlignment-[fixable])includes/classes/blocks/class-registration.php (6)
16-16: 🧹 Nitpick (assertive)
Correct the access modifier in the documentation block
The method
__construct()is declared aspublic, but the docblock has@access private. Please update the@accesstag topublicto reflect the correct visibility.
26-29: 🧹 Nitpick (assertive)
Specify the access level in the docblock
The method
enqueue_block_variations_script()lacks an@accesstag in its docblock. Please add@access publicto indicate its visibility.
81-83:
⚠️ Potential issueRemove undocumented parameter from the docblock
The parameter
$parent_blockis documented in the docblock ofpre_render_featured_block(), but it is not present in the function signature. Please remove$parent_blockfrom the docblock to maintain consistency.
74-74: 🧹 Nitpick (assertive)
Adjust spacing after function to meet coding standards
According to coding standards, there should be two blank lines after a function declaration. Only one blank line is present after the
enqueue_block_variations_script()method.🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 74-74: includes/classes/blocks/class-registration.php#L74
Expected 2 blank lines after function; 1 found. (Squiz.WhiteSpace.FunctionSpacing-[fixable])
101-101: 🧹 Nitpick (assertive)
Remove unnecessary space after opening parenthesis
In the
ifstatement, there is an extra space after the opening parenthesis. It should be formatted asif (! is_array( $sticky_posts ) ) {to adhere to coding standards.🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 101-101: includes/classes/blocks/class-registration.php#L101
Expected 0 spaces after opening bracket; 1 found. (Squiz.WhiteSpace.ControlStructureSpacing-[fixable])
88-120: 🛠️ Refactor suggestion
Avoid adding filters within functions to prevent multiple registrations
Adding the
query_loop_block_query_varsfilter inside thepre_render_featured_block()method can lead to the filter being registered multiple times, which may cause unexpected behaviour or performance issues. Consider registering the filter once during class initialization and checking the block variation within the filter callback.Apply this refactor to register the filter in the constructor:
public function __construct() { add_action( 'enqueue_block_editor_assets', array( $this, 'enqueue_block_variations_script' ), 10 ); add_filter( 'pre_render_block', array( $this, 'pre_render_featured_block' ), 10, 2 ); + add_filter( 'query_loop_block_query_vars', array( $this, 'modify_query_vars' ), 10, 2 ); } -public function pre_render_featured_block( $pre_render, $parsed_block ) { - // Determine if this is the custom block variation. - if ( isset( $parsed_block['attrs']['namespace'] ) && 'lsx/lsx-featured-posts' === $parsed_block['attrs']['namespace'] ) { - add_filter( - 'query_loop_block_query_vars', - function( $query, $block ) use ( $parsed_block ) { - // Modifications to $query... - return $query; - }, - 10, - 2 - ); - } - return $pre_render; -} +public function pre_render_featured_block( $pre_render, $parsed_block ) { + $this->current_block = $parsed_block; + return $pre_render; +} +public function modify_query_vars( $query, $block ) { + if ( isset( $this->current_block['attrs']['namespace'] ) && 'lsx/lsx-featured-posts' === $this->current_block['attrs']['namespace'] ) { + // Modify $query as needed... + } + return $query; +}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 101-101: includes/classes/blocks/class-registration.php#L101
Expected 0 spaces after opening bracket; 1 found. (Squiz.WhiteSpace.ControlStructureSpacing-[fixable])includes/classes/legacy/class-unit-query.php (2)
177-190: 🧹 Nitpick (assertive)
Addition of
item_pricemethodThe new
item_pricemethod enhances functionality by allowing retrieval of the item's price. Ensure that thepricekey exists within$this->query_itemto prevent undefined index notices.Consider adding a check to verify that
$this->query_item['price']is set before attempting to use it.
192-206: 🧹 Nitpick (assertive)
Addition of
item_typemethodIntroducing the
item_typemethod is beneficial for accessing the item's type. As withitem_price, ensure thetypekey exists in$this->query_itemto avoid potential errors.Consider verifying the existence of
$this->query_item['type']before using it.includes/classes/legacy/class-tour.php (1)
187-189: 🧹 Nitpick (assertive)
Fix spacing before closing parenthesis in if statement
There's an extra space before the closing parenthesis on line 187, which doesn't adhere to the coding standards.
Apply this diff to correct the spacing:
- if ( is_object( $tour_operator ) && isset( $tour_operator->options['currency'] ) && ! empty( $tour_operator->options['currency'] ) ) { + if ( is_object( $tour_operator ) && isset( $tour_operator->options['currency'] ) && ! empty( $tour_operator->options['currency'] )) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 187-187: includes/classes/legacy/class-tour.php#L187
Expected 0 spaces before closing bracket; 1 found. (Squiz.WhiteSpace.ControlStructureSpacing-[fixable])includes/classes/legacy/class-admin.php (6)
254-255: 🧹 Nitpick (assertive)
Simplify conditional display logic for 'Choose Image' and 'Remove Image' buttons
To improve readability and maintainability, consider using CSS classes to show or hide the buttons instead of inline styles with PHP conditions. This approach separates logic from presentation and makes the code cleaner.
Possible refactored code:
- <a style="<?php if ( '' !== $value && false !== $value ) { ?> display:none;<?php } ?>" class="button-secondary lsx-thumbnail-image-add"><?php esc_html_e( 'Choose Image', 'tour-operator' ); ?></a> - <a style="<?php if ( '' === $value || false === $value ) { ?>display:none;<?php } ?>" class="button-secondary lsx-thumbnail-image-remove"><?php esc_html_e( 'Remove Image', 'tour-operator' ); ?></a> + <a class="button-secondary lsx-thumbnail-image-add<?php if ( '' !== $value && false !== $value ) { echo ' hidden'; } ?>"><?php esc_html_e( 'Choose Image', 'tour-operator' ); ?></a> + <a class="button-secondary lsx-thumbnail-image-remove<?php if ( '' === $value || false === $value ) { echo ' hidden'; } ?>"><?php esc_html_e( 'Remove Image', 'tour-operator' ); ?></a>And define the
.hiddenclass in your stylesheet:.hidden { display: none; }
79-79:
⚠️ Potential issueReview the conditional check for
$this->taxonomiesSince
$this->taxonomiesis initialised as an array, the conditionif ( false !== $this->taxonomies )will always evaluate to true. If the intention is to check if$this->taxonomiesis not empty, consider usingif ( ! empty( $this->taxonomies ) )instead.Apply this diff:
- if ( false !== $this->taxonomies ) { + if ( ! empty( $this->taxonomies ) ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( ! empty( $this->taxonomies ) ) {
250-250:
⚠️ Potential issueUse
esc_attr()for escaping attribute valuesWhen outputting data into HTML attributes, like the
valueof an input field, it's best to useesc_attr()to ensure proper encoding and prevent XSS vulnerabilities.Apply this diff:
- <input class="input_image" type="hidden" name="thumbnail" value="<?php echo wp_kses_post( $value ); ?>"> + <input class="input_image" type="hidden" name="thumbnail" value="<?php echo esc_attr( $value ); ?>">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<input class="input_image" type="hidden" name="thumbnail" value="<?php echo esc_attr( $value ); ?>">
319-319:
⚠️ Potential issueUse
esc_attr()for escaping attribute valuesWhen outputting data into HTML attributes, like the
valueof an input field, it's best to useesc_attr()to ensure proper encoding and prevent XSS vulnerabilities.Apply this diff:
- <input name="tagline" id="tagline" type="text" value="<?php echo wp_kses_post( $value ); ?>" size="40"> + <input name="tagline" id="tagline" type="text" value="<?php echo esc_attr( $value ); ?>" size="40">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<input name="tagline" id="tagline" type="text" value="<?php echo esc_attr( $value ); ?>" size="40">
29-35:
⚠️ Potential issueInitialise newly declared properties to prevent undefined behaviour
The properties
$connections,$single_fields, and$taxonomiesare declared but not initialised, which may lead to undefined variables if accessed before being set. It's good practice to initialise them, preferably in the constructor.Apply this diff to initialise the properties:
public function __construct() { $this->options = get_option( '_lsx-to_settings', false ); + $this->connections = array(); + $this->single_fields = array(); + $this->taxonomies = array(); $this->set_vars(); $this->videos = Video::get_instance(); add_action( 'init', array( $this, 'init' ) ); add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_admin_stylescripts' ), 999 ); add_action( 'cmb_save_custom', array( $this, 'post_relations' ), 3, 20 ); }Committable suggestion skipped: line range outside the PR's diff.
280-299: 🛠️ Refactor suggestion
Enhance validation of 'thumbnail' and 'tagline' meta fields
While you're sanitising the 'thumbnail' and 'tagline' fields using
sanitize_text_field(), consider further validating the 'thumbnail' as an integer since it's expected to be a numeric ID. This adds an extra layer of security.Apply this diff to validate the 'thumbnail' field:
$thumbnail_meta = sanitize_text_field( $_POST['thumbnail'] ); + if ( is_numeric( $thumbnail_meta ) ) { + $thumbnail_meta = intval( $thumbnail_meta ); + } else { + $thumbnail_meta = ''; + } $thumbnail_meta = ! empty( $thumbnail_meta ) ? $thumbnail_meta : '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( isset( $_POST['thumbnail'] ) && ! empty( $_POST['thumbnail'] ) ) { $thumbnail_meta = sanitize_text_field( $_POST['thumbnail'] ); if ( is_numeric( $thumbnail_meta ) ) { $thumbnail_meta = intval( $thumbnail_meta ); } else { $thumbnail_meta = ''; } $thumbnail_meta = ! empty( $thumbnail_meta ) ? $thumbnail_meta : ''; if ( empty( $thumbnail_meta ) ) { delete_term_meta( $term_id, 'thumbnail' ); } else { update_term_meta( $term_id, 'thumbnail', $thumbnail_meta ); } } if ( isset( $_POST['tagline'] ) && ! empty( $_POST['tagline'] ) ) { $meta = sanitize_text_field( $_POST['tagline'] ); $meta = ! empty( $meta ) ? $meta : ''; if ( empty( $meta ) ) { delete_term_meta( $term_id, 'tagline' ); } else { update_term_meta( $term_id, 'tagline', $meta ); }includes/functions.php (1)
347-361: 🧹 Nitpick (assertive)
Introduce
lsx_to_itinerary_accommodation_type()and consider code reuseThe new function
lsx_to_itinerary_accommodation_type()enhances functionality by outputting accommodation types associated with the itinerary. However, it shares similar logic withlsx_to_itinerary_accommodation()for handling$itinerary_accommodation. Consider abstracting the common code into a helper function to adhere to the DRY (Don't Repeat Yourself) principle.Apply this refactor to extract shared logic:
function get_itinerary_accommodation( $tour_itinerary ) { $itinerary_accommodation = $tour_itinerary->itinerary['accommodation_to_tour']; if ( ! is_array( $itinerary_accommodation ) ) { $itinerary_accommodation = array( $itinerary_accommodation ); } return $itinerary_accommodation; }Then update both functions to use
get_itinerary_accommodation().includes/classes/admin/class-settings.php (4)
327-362: 🧹 Nitpick (assertive)
Consider refactoring to reduce code duplication
The methods
select_field,checkbox_field,text_field, andimage_fieldhave similar structures and code patterns. Refactoring these methods to utilise a common base function or class could improve maintainability and readability.Also applies to: 372-402, 412-438, 448-500
431-431:
⚠️ Potential issueEscape output when rendering input fields
The
$valuevariable is output directly into the HTML without escaping. This could result in XSS vulnerabilities. Please useesc_attr()to safely output the value.Apply this diff to fix the issue:
- $field[] = '<input value="' . $value . '" type="' . $params['type'] . '" name="' . $field_id . '" />'; + $field[] = '<input value="' . esc_attr( $value ) . '" type="' . esc_attr( $params['type'] ) . '" name="' . esc_attr( $field_id ) . '" />';Committable suggestion skipped: line range outside the PR's diff.
482-482:
⚠️ Potential issueEscape output in image preview
The variables
$prev_css,$image[0], and$params['preview_w']are output without escaping. This could allow for XSS attacks if these variables contain malicious content. Please ensure they are properly escaped.Apply this diff to fix the issue:
- $field[] = '<div class="thumbnail-preview" style="' . $prev_css . '"><img src="' . $image[0] . '" width="' . $params['preview_w'] . '" style="color:black;" /></div>'; + $field[] = '<div class="thumbnail-preview" style="' . esc_attr( $prev_css ) . '"><img src="' . esc_url( $image[0] ) . '" width="' . esc_attr( $params['preview_w'] ) . '" style="color:black;" /></div>';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$field[] = '<div class="thumbnail-preview" style="' . esc_attr( $prev_css ) . '"><img src="' . esc_url( $image[0] ) . '" width="' . esc_attr( $params['preview_w'] ) . '" style="color:black;" /></div>';
355-355:
⚠️ Potential issueEscape output in select options
Variables
$o_keyand$o_valare output without escaping in the<option>tags. This can lead to security issues. It's advisable to useesc_attr()andesc_html()to escape these values.Apply this diff to fix the issue:
- $field[] = '<option ' . $select_param . ' value="' . $o_key . '">' . $o_val . '</option>'; + $field[] = '<option ' . $select_param . ' value="' . esc_attr( $o_key ) . '">' . esc_html( $o_val ) . '</option>';Committable suggestion skipped: line range outside the PR's diff.
includes/classes/blocks/class-bindings.php (7)
478-478: 🧹 Nitpick (assertive)
Use tabs for indentation
Line 478 is indented with spaces instead of tabs. Please use tabs for indentation as per the project's coding standards.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 478-478: includes/classes/blocks/class-bindings.php#L478
Tabs must be used to indent lines; spaces are not allowed. (Generic.WhiteSpace.DisallowSpaceIndent-[fixable])
298-298: 🧹 Nitpick (assertive)
Remove trailing whitespace at end of line
There's unnecessary whitespace at the end of line 298. Please remove it to comply with code styling guidelines.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 298-298: includes/classes/blocks/class-bindings.php#L298
Whitespace found at end of line. (Squiz.WhiteSpace.SuperfluousWhitespace-[fixable])
73-73: 🧹 Nitpick (assertive)
Remove unnecessary spaces after opening parentheses
According to coding standards, there should be no space after an opening parenthesis in control structures. Please remove the extra spaces.
Apply these diffs to fix the formatting:
- if ( ! function_exists( 'register_block_bindings_source' ) ) { + if (! function_exists( 'register_block_bindings_source' ) ) {- if ( true === $only_parents ) { + if (true === $only_parents ) {Also applies to: 147-147
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 73-73: includes/classes/blocks/class-bindings.php#L73
Expected 0 spaces after opening bracket; 1 found. (Squiz.WhiteSpace.ControlStructureSpacing-[fixable])
702-702:
⚠️ Potential issueFix typo in method name
maybe_hide_varitaionin method definitionThe method
maybe_hide_varitaionis misspelled. It should bemaybe_hide_variationto match the correctedadd_filtercall and maintain consistency.Apply this diff to correct the method name:
- public function maybe_hide_varitaion( $block_content, $parsed_block, $block_obj ) { + public function maybe_hide_variation( $block_content, $parsed_block, $block_obj ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public function maybe_hide_variation( $block_content, $parsed_block, $block_obj ) {
69-69:
⚠️ Potential issueFix typo in method name
maybe_hide_varitaioninadd_filterThe method name
maybe_hide_varitaionis misspelled in theadd_filtercall. It should bemaybe_hide_variationto ensure consistency and prevent potential errors.Apply this diff to correct the method name:
- add_filter( 'render_block', array( $this, 'maybe_hide_varitaion' ), 10, 3 ); + add_filter( 'render_block', array( $this, 'maybe_hide_variation' ), 10, 3 );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.add_filter( 'render_block', array( $this, 'maybe_hide_variation' ), 10, 3 );
693-700: 🧹 Nitpick (assertive)
Add missing doc comment for parameter
$block_objThe doc comment for the parameter
$block_objis missing in the methodmaybe_hide_variation(). Please add it to fully document the function.Apply this diff to add the missing doc comment:
/** * A function to detect variation and alter the query args. * * Following the https://developer.wordpress.org/news/2022/12/building-a-book-review-grid-with-a-query-loop-block-variation/ * * @param string|null $pre_render The pre-rendered content. Default null. * @param array $parsed_block The block being rendered. + * @param WP_Block|null $block_obj If this is a nested block, a reference to the parent block. */Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 693-693: includes/classes/blocks/class-bindings.php#L693
Doc comment for parameter "$block_obj" missing. (Squiz.Commenting.FunctionComment)
677-681:
⚠️ Potential issueFix duplicate
classattributes, correctsrcattribute, and escape$wetu_idin<iframe>tagThe
<iframe>tag contains duplicateclassattributes, which is invalid HTML. Additionally, thesrcattribute uses a hardcoded value instead of the dynamic$wetu_id, and$wetu_idshould be properly escaped to prevent XSS vulnerabilities. Please combine the classes into a singleclassattribute, update thesrcattribute to use$wetu_id, and escape$wetu_idusingesc_attr().Apply this diff to fix the issues:
- $map = '<iframe width="100%" height="500" frameborder="0" allowfullscreen="" class="wetu-map" class="block perfmatters-lazy entered pmloaded" data-src="https://wetu.com/Itinerary/VI/' . $wetu_id . '?m=bdep" data-ll-status="loaded" src="https://wetu.com/Itinerary/VI/c5ab6e23-b482-4256-b509-1069506fe1c2?m=bdep"></iframe>'; + $map = '<iframe width="100%" height="500" frameborder="0" allowfullscreen="" class="wetu-map block perfmatters-lazy entered pmloaded" src="' . esc_attr( 'https://wetu.com/Itinerary/VI/' . $wetu_id . '?m=bdep' ) . '" data-ll-status="loaded"></iframe>';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$wetu_id = get_post_meta( get_the_ID(), 'lsx_wetu_id', true ); if ( ! empty( $wetu_id ) ) { $map = '<iframe width="100%" height="500" frameborder="0" allowfullscreen="" class="wetu-map block perfmatters-lazy entered pmloaded" src="' . esc_attr( 'https://wetu.com/Itinerary/VI/' . $wetu_id . '?m=bdep' ) . '" data-ll-status="loaded"></iframe>'; } break;
Description of the Change
We have created a group block variation that will add in an image to the editor as a placeholder, and it will output the WETU map on the frontend.
Screenshots
Add Block
Frontend
Editor
Applicable Issues
#365
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores