Fix beta versions, apply fixes for shellcheck hints - #389
Conversation
The script failed with -betaX as the version
📝 WalkthroughWalkthroughThe installation template updates WordPress version selection, shell path handling, test-suite configuration, database host parsing, database administration commands, and reinstall confirmation input. ChangesWordPress test installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@templates/install-wp-tests.sh`:
- Around line 239-242: Update the sed substitutions in the install configuration
flow to escape DB_NAME, DB_USER, DB_PASS, and DB_HOST for replacement syntax,
following the existing WP_CORE_DIR_ESCAPED pattern. Use one consistent sed
delimiter across all substitutions so values containing &, the delimiter, or
backslashes are inserted literally.
- Around line 288-296: Replace the EXTRA option string in
templates/install-wp-tests.sh:288-296 with a Bash array and append each host,
port, protocol, or socket option as separate elements. Update the command
invocations at templates/install-wp-tests.sh:256-258, 270-272, and 306-306 to
pass "${EXTRA[@]}" after --password="$DB_PASS", preserving correct argument
boundaries at every site.
- Around line 283-295: Update the DB_HOST parsing logic before the existing
PARTS-based hostname/port handling to recognize bracketed IPv6 values such as
[::1]:3306, preserving the full IPv6 hostname and extracting only the trailing
port or socket. Keep the current hostname-and-port/socket behavior unchanged for
non-bracketed DB_HOST values, using DB_HOSTNAME and DB_SOCK_OR_PORT for the
subsequent EXTRA construction.
- Line 145: Update the VERSION_ESCAPED assignment to use shell parameter
expansion on WP_VERSION for replacing dots with escaped dots, removing the echo
and sed pipeline that triggers ShellCheck SC2001. Preserve the existing escaped
VERSION_ESCAPED value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab8f5e13-2d94-4020-b55c-0bf235637bf2
📒 Files selected for processing (1)
templates/install-wp-tests.sh
| local VERSION_ESCAPED=`echo $WP_VERSION | sed 's/\./\\\\./g'` | ||
| LATEST_VERSION=$(grep -o '"version":"'$VERSION_ESCAPED'[^"]*' $TMPDIR/wp-latest.json | sed 's/"version":"//' | head -1) | ||
| local VERSION_ESCAPED | ||
| VERSION_ESCAPED=$(echo "$WP_VERSION" | sed 's/\./\\\\./g') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the remaining ShellCheck SC2001 warning.
Use parameter expansion for this fixed substitution instead of spawning echo and sed.
Proposed fix
- VERSION_ESCAPED=$(echo "$WP_VERSION" | sed 's/\./\\\\./g')
+ VERSION_ESCAPED=${WP_VERSION//./\\.}📝 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.
| VERSION_ESCAPED=$(echo "$WP_VERSION" | sed 's/\./\\\\./g') | |
| VERSION_ESCAPED=${WP_VERSION//./\\.} |
🧰 Tools
🪛 Shellcheck (0.11.0)
[style] 145-145: See if you can use ${variable//search/replace} instead.
(SC2001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/install-wp-tests.sh` at line 145, Update the VERSION_ESCAPED
assignment to use shell parameter expansion on WP_VERSION for replacing dots
with escaped dots, removing the echo and sed pipeline that triggers ShellCheck
SC2001. Preserve the existing escaped VERSION_ESCAPED value.
Source: Linters/SAST tools
| sed "$ioption" "s/youremptytestdbnamehere/$DB_NAME/" "$WP_TESTS_DIR"/wp-tests-config.php | ||
| sed "$ioption" "s/yourusernamehere/$DB_USER/" "$WP_TESTS_DIR"/wp-tests-config.php | ||
| sed "$ioption" "s/yourpasswordhere/$DB_PASS/" "$WP_TESTS_DIR"/wp-tests-config.php | ||
| sed "$ioption" "s|localhost|${DB_HOST}|" "$WP_TESTS_DIR"/wp-tests-config.php |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escape database values for sed replacement syntax.
Quoted expansions only protect the shell; DB_NAME, DB_USER, DB_PASS, and DB_HOST can still alter the sed replacement when they contain &, a delimiter, or \. Escape these values like WP_CORE_DIR_ESCAPED and use one delimiter consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/install-wp-tests.sh` around lines 239 - 242, Update the sed
substitutions in the install configuration flow to escape DB_NAME, DB_USER,
DB_PASS, and DB_HOST for replacement syntax, following the existing
WP_CORE_DIR_ESCAPED pattern. Use one consistent sed delimiter across all
substitutions so values containing &, the delimiter, or backslashes are inserted
literally.
| # parse DB_HOST for port or socket references | ||
| local PARTS=(${DB_HOST//\:/ }) | ||
| local PARTS | ||
| read -ra PARTS <<< "${DB_HOST//:/ }" | ||
| local DB_HOSTNAME=${PARTS[0]}; | ||
| local DB_SOCK_OR_PORT=${PARTS[1]}; | ||
| local EXTRA="" | ||
|
|
||
| if ! [ -z $DB_HOSTNAME ] ; then | ||
| if [ $(echo $DB_SOCK_OR_PORT | grep -e '^[0-9]\{1,\}$') ]; then | ||
| if [ -n "$DB_HOSTNAME" ] ; then | ||
| if [[ "$DB_SOCK_OR_PORT" =~ ^[0-9]+$ ]]; then | ||
| EXTRA=" --host=$DB_HOSTNAME --port=$DB_SOCK_OR_PORT --protocol=tcp" | ||
| elif ! [ -z $DB_SOCK_OR_PORT ] ; then | ||
| elif [ -n "$DB_SOCK_OR_PORT" ] ; then | ||
| EXTRA=" --socket=$DB_SOCK_OR_PORT" | ||
| elif ! [ -z $DB_HOSTNAME ] ; then | ||
| elif [ -n "$DB_HOSTNAME" ] ; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve bracketed IPv6 database hosts.
Replacing every colon breaks valid values such as [::1]:3306: DB_HOSTNAME becomes [ and the next fragment is incorrectly used as a socket. Parse bracketed IPv6 hosts before handling the hostname-and-port/socket form.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/install-wp-tests.sh` around lines 283 - 295, Update the DB_HOST
parsing logic before the existing PARTS-based hostname/port handling to
recognize bracketed IPv6 values such as [::1]:3306, preserving the full IPv6
hostname and extracting only the trailing port or socket. Keep the current
hostname-and-port/socket behavior unchanged for non-bracketed DB_HOST values,
using DB_HOSTNAME and DB_SOCK_OR_PORT for the subsequent EXTRA construction.
| local EXTRA="" | ||
|
|
||
| if ! [ -z $DB_HOSTNAME ] ; then | ||
| if [ $(echo $DB_SOCK_OR_PORT | grep -e '^[0-9]\{1,\}$') ]; then | ||
| if [ -n "$DB_HOSTNAME" ] ; then | ||
| if [[ "$DB_SOCK_OR_PORT" =~ ^[0-9]+$ ]]; then | ||
| EXTRA=" --host=$DB_HOSTNAME --port=$DB_SOCK_OR_PORT --protocol=tcp" | ||
| elif ! [ -z $DB_SOCK_OR_PORT ] ; then | ||
| elif [ -n "$DB_SOCK_OR_PORT" ] ; then | ||
| EXTRA=" --socket=$DB_SOCK_OR_PORT" | ||
| elif ! [ -z $DB_HOSTNAME ] ; then | ||
| elif [ -n "$DB_HOSTNAME" ] ; then | ||
| EXTRA=" --host=$DB_HOSTNAME --protocol=tcp" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass database connection options as an argument array.
Quoting "$EXTRA" while adjoining it to --password="$DB_PASS" creates one argument, e.g. --password=secret --host=localhost; authentication fails whenever EXTRA is populated. Use a Bash array instead of an option string.
templates/install-wp-tests.sh#L288-L296: buildEXTRAwithlocal -a EXTRA=()and append each option separately.templates/install-wp-tests.sh#L256-L258: pass"${EXTRA[@]}"after--password="$DB_PASS".templates/install-wp-tests.sh#L270-L272: pass"${EXTRA[@]}"after--password="$DB_PASS".templates/install-wp-tests.sh#L306-L306: pass"${EXTRA[@]}"after--password="$DB_PASS".
📍 Affects 1 file
templates/install-wp-tests.sh#L288-L296(this comment)templates/install-wp-tests.sh#L256-L258templates/install-wp-tests.sh#L270-L272templates/install-wp-tests.sh#L306-L306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/install-wp-tests.sh` around lines 288 - 296, Replace the EXTRA
option string in templates/install-wp-tests.sh:288-296 with a Bash array and
append each host, port, protocol, or socket option as separate elements. Update
the command invocations at templates/install-wp-tests.sh:256-258, 270-272, and
306-306 to pass "${EXTRA[@]}" after --password="$DB_PASS", preserving correct
argument boundaries at every site.
|
Thanks for opening this! Would you mind addressing the review comments & the failing tests? Some tests for the changed behavior for beta versions would be nice too. |
2 participants
The script failed with
-betaXas the version.While at it, I fixed all the things
shellcheckwas complaining about. If you do not want them, this is the part that is needed:Summary by CodeRabbit
instal-wp-tests-fail.txt