fix(install): Webインストーラーの起動不能とTypeErrorを修正#6946
Open
nanasess wants to merge 2 commits into
Open
Conversation
4.4 で入った回帰により、Web インストーラーが起動できず、起動しても インストールを完了できない状態だったため修正する。 起動不能 (A non-empty secret is required.): framework.secret が単一の env 変数だけで構成される場合、Symfony は secrets vault にその env 変数を復号鍵から導出する役割を持たせる (FrameworkExtension::registerSecretsConfiguration / SodiumVault::loadEnvVars)。 vault も復号鍵も無いインストール時は導出結果が空文字となり、env var loader の 空文字が eccube.yaml の env(ECCUBE_AUTH_MAGIC): '<change.me>' を上書きするため、 framework.secret が空になり SignatureHasher が例外を投げていた。EC-CUBE は secrets vault を使わないため、インストール環境では無効化する。これにより InstallController::isInstalled() が未インストールを「インストール済み」と 誤判定する問題も解消する。 Step4Type::validate() の TypeError: Assert\Callback はフィールド値 (文字列) を渡すが、引数が array 宣言だった。 誤った PHPDoc がネイティブ型へ変換された際の混入 (fbad0fd)。あわせて、 違反が現在のコンテキストへ追加されず NotBlank が機能していなかった点も直す。 InstallController::convertAdminAllowHosts() の TypeError: admin_allow_hosts は任意入力のため未入力時は null が渡るが string 宣言だった。 同じく fbad0fd による混入で、IP 制限を入力しない限り必ず失敗していた。 完了画面「管理画面を表示」の 400: install_plugin_redirect は de288c4 で XHR ガードが追加されたが、ボタンは 素の <a href> によるブラウザ遷移のため 400 になっていた (4.3 系のリリースには 未到達で 4.4 のみの回帰)。jQuery 非依存の fetch で XHR として要求する。 サーバー側のトランザクションファイル検証はそのまま維持する。あわせて、 CacheUtil::clearCache() が kernel.terminate でキャッシュを削除するため、 レスポンス直後に遷移すると 500 になる競合を、応答確認後に遷移することで回避する。 テスト: Step4TypeTest の markTestIncomplete を外して有効化する。また、インストール完了 までを通す Playwright E2E (install-tests project) と専用 CI ジョブを追加する。 インストール後は APP_ENV=prod となり、prod の session cookie は SameSite=None の ため HTTP では維持できないため、CI では symfony CLI により HTTPS で起動する。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthrough未インストール状態の EC-CUBE を対象に、インストーラ用 Playwright E2E テスト、HTTPS 対応 CI ジョブ、完了後の管理画面遷移、インストール関連の設定・バリデーション修正を追加しました。 Changesインストーラの設定・バリデーション
インストール完了後の遷移
Playwright インストーラテスト
CI 実行経路
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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 @.github/workflows/e2e-test.yml:
- Around line 240-241: Update the actions/checkout step in the workflow to set
persist-credentials to false, ensuring the checkout action does not retain the
GitHub token in .git/config for subsequent Composer, npm, or build steps.
In `@e2e/tests/install-complete.spec.ts`:
- Around line 59-64: Update the response listener in install-complete.spec.ts so
transient HTTP 500 responses encountered while waitForAdminPage() is retrying
are excluded from responseErrors, while other error responses remain collected
and continue to fail the test as before.
In `@src/Eccube/Resource/template/install/complete.twig`:
- Around line 135-153: Update the click handler’s fetch/waitForAdminPage promise
chain so communication errors and non-2xx responses stop processing instead of
reaching window.location.href. Ensure waitForAdminPage failure also rejects or
otherwise prevents navigation; in every failure path, re-enable the button and
display the existing error UI, while preserving the admin redirect only after
successful completion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba4bccf0-020d-4856-9cc7-576625f60d30
📒 Files selected for processing (9)
.github/workflows/e2e-test.ymlapp/config/eccube/packages/install/framework.yamle2e/pages/install.page.tse2e/playwright.config.tse2e/tests/install-complete.spec.tssrc/Eccube/Controller/Install/InstallController.phpsrc/Eccube/Form/Type/Install/Step4Type.phpsrc/Eccube/Resource/template/install/complete.twigtests/Eccube/Tests/Form/Type/Install/Step4TypeTest.php
Comment on lines
+240
to
+241
| - name: Checkout | ||
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate workflow and inspect relevant ranges around the checkout step
file=".github/workflows/e2e-test.yml"
wc -l "$file"
sed -n '200,290p' "$file" | cat -nRepository: EC-CUBE/ec-cube
Length of output: 4236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git usage in workflow =="
rg -n '\bgit\b' .github/workflows/e2e-test.yml || true
echo
echo "== local composer action =="
fd -a '.github/actions/composer' . || true
if [ -f .github/actions/composer/action.yml ]; then
wc -l .github/actions/composer/action.yml
sed -n '1,220p' .github/actions/composer/action.yml | cat -n
fi
echo
echo "== git usage in .github/actions/composer =="
rg -n '\bgit\b' .github/actions/composer || trueRepository: EC-CUBE/ec-cube
Length of output: 1290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/e2e-test.yml"
sed -n '90,240p' "$file" | cat -nRepository: EC-CUBE/ec-cube
Length of output: 6687
actions/checkout の persist-credentials を false にしてください。
後続の Composer/npm/build でリポジトリ内コードが実行されるため、.git/config に GitHub トークンを残す必要はありません。
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 240-241: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/e2e-test.yml around lines 240 - 241, Update the
actions/checkout step in the workflow to set persist-credentials to false,
ensuring the checkout action does not retain the GitHub token in .git/config for
subsequent Composer, npm, or build steps.
Source: Linters/SAST tools
Comment on lines
+59
to
+64
| const responseErrors: string[] = []; | ||
| page.on('response', (response) => { | ||
| if (response.status() >= 400) { | ||
| responseErrors.push(`${response.status()} ${response.request().method()} ${response.url()}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test file around the cited lines and nearby references.
git ls-files e2e/tests/install-complete.spec.ts
wc -l e2e/tests/install-complete.spec.ts
sed -n '1,180p' e2e/tests/install-complete.spec.ts
# Also look for the admin directory constant and any related readiness checks.
rg -n "ADMIN_DIR|responseErrors|response\.status\(\) >= 400|install-complete|readiness" e2e/tests -SRepository: EC-CUBE/ec-cube
Length of output: 5910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the file in the relevant area with line numbers if the first pass shows it's not enough.
cat -n e2e/tests/install-complete.spec.ts | sed -n '45,120p'Repository: EC-CUBE/ec-cube
Length of output: 3137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the page object methods used by the test to see what requests are expected.
git ls-files e2e/pages/install.page.ts
wc -l e2e/pages/install.page.ts
ast-grep outline e2e/pages/install.page.ts --view expanded
sed -n '1,260p' e2e/pages/install.page.tsRepository: EC-CUBE/ec-cube
Length of output: 5701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the admin readiness/redirect endpoint and any retry logic that could emit 500s.
rg -n "install_plugin_redirect|complete_管理画面|管理画面を表示|管理画面ボタン|ADMIN_DIR|ready|readiness|500" -S e2e src app public testsRepository: EC-CUBE/ec-cube
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the install completion template around the redirect/retry logic.
sed -n '100,210p' src/Eccube/Resource/template/install/complete.twig
# Inspect the install plugin redirect controller to see the route and expected responses.
sed -n '1,220p' src/Eccube/Controller/InstallPluginController.phpRepository: EC-CUBE/ec-cube
Length of output: 11218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the exact retry helper and any commentary about transient 500s or cache clearing.
rg -n "waitForAdminPage|cache.*500|削除中|500 になることがある|install_plugin_redirect|管理画面を表示" src/Eccube/Resource/template/install/complete.twig src/Eccube/Controller/InstallPluginController.php -n -SRepository: EC-CUBE/ec-cube
Length of output: 1105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact install completion JS around the transient 500 handling.
cat -n src/Eccube/Resource/template/install/complete.twig | sed -n '115,190p'Repository: EC-CUBE/ec-cube
Length of output: 3230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the install plugin redirect controller for the expected response shape.
cat -n src/Eccube/Controller/InstallPluginController.php | sed -n '1,220p'Repository: EC-CUBE/ec-cube
Length of output: 8398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the install completion template around the transient 500 handling.
cat -n src/Eccube/Resource/template/install/complete.twig | sed -n '115,190p'Repository: EC-CUBE/ec-cube
Length of output: 3230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where admin_url is defined/passed in the install complete template/controller context.
rg -n "admin_url" src/Eccube/Resource/template/install/complete.twig src/Eccube/Controller src/Eccube -SRepository: EC-CUBE/ec-cube
Length of output: 4755
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the template context to see the actual admin URL that the test will hit.
rg -n "admin_url" src/Eccube/Controller src/Eccube/Resource/template/install -S
sed -n '1,220p' src/Eccube/Controller/InstallController.phpRepository: EC-CUBE/ec-cube
Length of output: 1059
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where the install complete page receives admin_url.
rg -n "admin_url" src/Eccube/Controller src/Eccube/Resource/template/install -S
sed -n '1,220p' src/Eccube/Controller/InstallController.phpRepository: EC-CUBE/ec-cube
Length of output: 1059
管理画面待機中の一時的な 500 を集計対象から外してください。
完了画面の waitForAdminPage() はキャッシュ削除中の 500 を前提に再試行するため、ここを responseErrors に入れると、最終的にログイン画面へ遷移できてもテストが失敗します。
🤖 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 `@e2e/tests/install-complete.spec.ts` around lines 59 - 64, Update the response
listener in install-complete.spec.ts so transient HTTP 500 responses encountered
while waitForAdminPage() is retrying are excluded from responseErrors, while
other error responses remain collected and continue to fail the test as before.
Comment on lines
+135
to
+153
| button.addEventListener('click', function (event) { | ||
| event.preventDefault(); | ||
|
|
||
| // redirect: 'manual' でリダイレクトを追跡しない. | ||
| // トランザクションファイルの削除だけを行わせ, 遷移は自前で行う. | ||
| fetch(redirectUrl, { | ||
| headers: { | ||
| 'X-Requested-With': 'XMLHttpRequest', | ||
| 'ECCUBE-CSRF-TOKEN': token | ||
| }, | ||
| credentials: 'same-origin', | ||
| redirect: 'manual' | ||
| }).catch(function (error) { | ||
| console.log(error); | ||
| }).then(function () { | ||
| return waitForAdminPage(adminUrl, 20, 500); | ||
| }).then(function () { | ||
| window.location.href = adminUrl; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant file and surrounding context
git ls-files 'src/Eccube/Resource/template/install/complete.twig' 'src/Eccube/Resource/template/**' | cat
echo '--- complete.twig (around lines 120-200) ---'
sed -n '120,200p' src/Eccube/Resource/template/install/complete.twig
echo '--- search waitForAdminPage ---'
rg -n "waitForAdminPage|redirectUrl|adminUrl|ECCUBE-CSRF-TOKEN|modal-footer|progress_message" src/Eccube/Resource/template/install/complete.twig src -g '!**/node_modules/**'Repository: EC-CUBE/ec-cube
Length of output: 28311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "function waitForAdminPage|const waitForAdminPage|waitForAdminPage\s*=\s*" src -g '!**/node_modules/**'Repository: EC-CUBE/ec-cube
Length of output: 265
失敗時は管理画面へ遷移しないようにしてください
src/Eccube/Resource/template/install/complete.twig
fetch() の通信エラーや 4xx/5xx を握りつぶしたまま waitForAdminPage() に進み、さらにこの関数は retries <= 0 でも失敗を返さないため、待機失敗後も window.location.href = adminUrl が実行されます。失敗時は遷移を止め、ボタンを再有効化してエラーを表示してください。
🤖 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 `@src/Eccube/Resource/template/install/complete.twig` around lines 135 - 153,
Update the click handler’s fetch/waitForAdminPage promise chain so communication
errors and non-2xx responses stop processing instead of reaching
window.location.href. Ensure waitForAdminPage failure also rejects or otherwise
prevents navigation; in every failure path, re-enable the button and display the
existing error UI, while preserving the admin redirect only after successful
completion.
RemoveUselessParamTagRector が、ネイティブ型宣言と重複する @param mixed $data を冗長として指摘するため削除する。 説明は PHPDoc の本文コメントに残す。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6946 +/- ##
==========================================
+ Coverage 75.79% 76.04% +0.24%
==========================================
Files 546 546
Lines 27136 27136
==========================================
+ Hits 20569 20635 +66
+ Misses 6567 6501 -66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要(Overview・Refs Issue)
.envが無い状態（＝インストール前）で Web インストーラーが起動できず、起動しても最後まで完了できない状態だったため修正します。いずれも 4.4 で入った回帰です。/installが 500A non-empty secret is required.ECCUBE_AUTH_MAGICを「復号鍵から導出」する役割を持ち、導出結果の空文字がデフォルト値を上書きTypeErrorStep4Type::validate()の引数がarray宣言（Assert\Callbackは文字列を渡す）TypeErrorconvertAdminAllowHosts()の引数がstring宣言（未入力時はnull）2 と 3 は
fbad0fd1ea「PHPDocの型をネイティブ型へ変換」で、誤っていた PHPDoc がそのままネイティブ型になったことによる混入です。特に 3 は IP 制限を入力しない限り必ず失敗するため、既定の手順ではインストールを完了できませんでした。4 は
de288c4203で 4 ルート一律に XHR ガードを追加した際の巻き込みです。git tag --containsとコードの中身の双方で確認したところ、4.3.0 / 4.3.1 / 4.3.1-p1 には未到達で、4.4 のみの回帰です。方針(Policy)
1. 起動不能（secrets vault）
framework.secretが単一の env 変数だけで構成される場合、Symfony は secrets vault にその env 変数を復号鍵から導出させます（FrameworkExtension::registerSecretsConfiguration→SodiumVault::loadEnvVars）。vault も復号鍵も無いインストール時は導出結果が空文字になり、env var loader が返した空文字はfalseではないため、EnvVarProcessorがeccube.yamlのenv(ECCUBE_AUTH_MAGIC): '<change.me>'を参照しません。EC-CUBE は secrets vault を使わないため、インストール環境でのみ無効化しました。
ECCUBE_AUTH_MAGICを.env.installに書き足す案もありましたが、同じ番人値が 3 箇所（eccube.yaml/InstallController::DEFAULT_AUTH_MAGIC/.env.install）に散るため採りませんでした。これにより
InstallController::isInstalled()が未インストールを「インストール済み」と誤判定する問題（'<change.me>' !== ''）も解消します。2. 「管理画面を表示」の 400
de288c4203の意図（トランザクションファイルの有効期限チェック）を維持するため、サーバー側は変更せず、ボタン側を XHR 化しました。fetchは jQuery と違いX-Requested-Withを自動付与しないため明示しています。hrefには管理画面 URL を入れ、JS 無効時や別タブでも 400 にならないようにしています。実装は jQuery 非依存です。3. 漏洩パスワードチェックは無効化していません
E2E のために
install環境のNotCompromisedPasswordを無効化する案もありましたが、installは実ユーザーが通る本番の経路であり、全インストールのパスワード強度を下げてしまうため採用していません。代わりに E2E 側で実行ごとにランダムなパスワードを生成しています（skipOnError: trueのため CI が HIBP に到達できなくても壊れません）。実装に関する補足(Appendix)
キャッシュ削除との競合（500）
CacheUtil::clearCache()はキャッシュ削除を予約するだけで、実際のcache:clearはkernel.terminate（レスポンス送信後）に走ります。そのためinstall_plugin_redirectのレスポンス直後に遷移すると、削除中のリクエストとなり 500 になります（ロード済みコンテナが遅延 require するファイルが消えるため）。これは 4.3 でも同じ構造の既存の競合ですが、E2E で恒常的に踏むため、完了画面側で管理画面の応答を確認してから遷移するようにしました。実ユーザーがインストール直後に 500 を踏む問題も併せて解消します。なお
redirectAdmin()のclearCache()は、complete()が既にclearCache('prod')済みで、プラグイン有効化もclearCacheOnTerminate()で自前に削除しているため冗長の可能性がありますが、確証が無いため本 PR では触っていません。E2E の前提条件
インストーラ E2E は既存の
playwrightジョブと同居できないため（.envを作成しschema:create済みのため）、専用ジョブを新設しました。既存ジョブは 1 行も変更していません。特にECCUBE_AUTH_MAGICを渡してはいけない点に注意が必要です（isInstalled()が true になり、step3 が存在しないdtb_base_infoを SELECT して落ちるため）。CI のサーバーは
php -Sではなく symfony CLI を使っています。インストール後はAPP_ENV=prodとなり、prod の session cookie はcookie_samesite: none/cookie_secure: autoのため、HTTP では Secure が付かずブラウザが cookie を破棄してログインを検証できないためです。curl | bashの公式インストーラはサプライチェーン上のリスクがあるため使わず、バージョン固定＋SHA256 検証でインストールしています。テンプレートの既知の罠（今回は未修正）
#mainの class が step 番号とズレています（step3 →class="step4"、step4 →step5、step5 →step6）。E2E では class ではなくh1と URL で判定しています。テスト（Test)
インストール完了までを通す Playwright E2E を新規追加しました（旧
ZZ99InstallerCestは step1〜step2 の権限チェックのみで、かつ現在どの CI からも実行されていません）。3 種類の DB すべてで実機確認済み（
symfony serve+ HTTPS）:DATABASE_SERVER_VERSION=18.4（pgsql 固有のバージョン抽出）DATABASE_CHARSET=utf8mb4（mysql のみの分岐）、8.4.103 DB とも 70 テーブル・店舗名・管理者・商品が正しく作成され、
.envのECCUBE_AUTH_MAGICは 32 文字のランダム値、ECCUBE_ADMIN_ALLOW_HOSTSは[]になることを確認しています。その他:
Step4TypeTestのmarkTestIncompleteを外して有効化（このマーカーがバグを隠していました）bin/phpunit tests/Eccube/Tests/Form/Type/Install/→ 36 tests / 36 assertions OKvendor/bin/phpstan analyse（level 6）→ No errorsvendor/bin/php-cs-fixer fix --dry-run→ 0 violationsnpx tsc --noEmit（e2e）→ エラーなし相談（Discussion）
complete.twig) はframe.twigやhtml/template/install/assets/js/function.jsと併せて jQuery に依存しています。今回追加した箇所は jQuery 非依存で書きましたが、install 画面全体の脱 jQuery は別途の作業になります。redirectAdmin()のclearCache()の冗長性（上記）について、意図をご存知でしたら教えてください。不要であればそちらが根本解決になります。doctrine_migration_versionsを作成しません（dropTables()が明示的に DROP している設計上の挙動）。今回は範囲外としましたが、意図どおりかご確認いただけると助かります。マイナーバージョン互換性保持のための制限事項チェックリスト
レビュワー確認項目
🤖 Generated with Claude Code
Summary by CodeRabbit