Skip to content

Conversation

@MarcGuay
Copy link
Contributor

Fixing issues with cache location described here #253

WP settings API will save empty strings for empty text fields which isset evaluates to true, so switch to !empty
@shadoath shadoath requested a review from Copilot May 24, 2025 22:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes how the cache directory path is resolved and updates the admin UI text to document the cache directory behavior.

  • Changed cache_dir_setting to prepend the base folder when a custom cache path is provided
  • Removed redundant base-folder prefix in settings output for cache_dir
  • Updated the admin description to include cache directory rules and defaults

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
wp-scss.php Modified how $cache_dir_setting is determined and used in settings
options.php Expanded the admin “Paths” section text to cover cache directory
Comments suppressed due to low confidence (1)

wp-scss.php:167

  • Add unit tests for the cache directory logic to cover cases where cache_dir is empty, starts with /, and omits the leading slash to prevent regressions.
$cache_dir_setting = !empty($wpscss_options['cache_dir']) ? $base_compiling_folder.$wpscss_options['cache_dir'] : WPSCSS_PLUGIN_DIR . '/cache/';

$cache_dir_setting = isset($wpscss_options['cache_dir']) ? $wpscss_options['cache_dir'] : WPSCSS_PLUGIN_DIR . '/cache/';
$scss_dir_setting = !empty($wpscss_options['scss_dir']) ? $wpscss_options['scss_dir'] : '';
$css_dir_setting = !empty($wpscss_options['css_dir']) ? $wpscss_options['css_dir'] : '';
$cache_dir_setting = !empty($wpscss_options['cache_dir']) ? $base_compiling_folder.$wpscss_options['cache_dir'] : WPSCSS_PLUGIN_DIR . '/cache/';
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

If the user-provided cache_dir value lacks a leading slash, concatenating it directly to $base_compiling_folder can produce an invalid path (/basepathcache/ instead of /basepath/cache/). Consider normalizing the input (e.g., ensure it starts with /) or using ltrim() and trailingslashit() to build the path.

Copilot uses AI. Check for mistakes.
$cache_dir_setting = isset($wpscss_options['cache_dir']) ? $wpscss_options['cache_dir'] : WPSCSS_PLUGIN_DIR . '/cache/';
$scss_dir_setting = !empty($wpscss_options['scss_dir']) ? $wpscss_options['scss_dir'] : '';
$css_dir_setting = !empty($wpscss_options['css_dir']) ? $wpscss_options['css_dir'] : '';
$cache_dir_setting = !empty($wpscss_options['cache_dir']) ? $base_compiling_folder.$wpscss_options['cache_dir'] : WPSCSS_PLUGIN_DIR . '/cache/';
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

[nitpick] Use WordPress helper functions (e.g., trailingslashit() and ltrim()) when constructing filesystem paths to ensure consistent slash handling and readability.

Suggested change
$cache_dir_setting = !empty($wpscss_options['cache_dir']) ? $base_compiling_folder.$wpscss_options['cache_dir'] : WPSCSS_PLUGIN_DIR . '/cache/';
$cache_dir_setting = !empty($wpscss_options['cache_dir']) ? trailingslashit($base_compiling_folder) . ltrim($wpscss_options['cache_dir'], '/') : trailingslashit(WPSCSS_PLUGIN_DIR) . 'cache/';

Copilot uses AI. Check for mistakes.
@shadoath shadoath merged commit 34a14a3 into ConnectThink:master Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants