feat: EC-CUBE 4.4への対応#23
Conversation
📝 WalkthroughWalkthroughEC-CUBE 4.4向けにプラグイン識別情報、名前空間、PHP属性、型宣言、プラグイン管理処理を更新しました。PHPUnit・Rector・PHP-CS-Fixer設定、CIの対応環境、静的解析ジョブ、リリース時の配布除外処理も追加・変更しています。 ChangesEC-CUBE 4.4移行
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
KenTanaka
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PluginManager.php (1)
154-161: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
$PageLayoutがnullの場合のTypeErrorを防ぐため、削除前にnullチェックを追加してください。Doctrine ORM 3.x では
EntityManager::remove()の引数に厳格な型宣言 (object $object) が追加されています。
対象のページレイアウトが運用中に変更または削除されていてfindOneByがnullを返した場合、そのまま$em->remove($PageLayout)に渡すとTypeErrorが発生し、プラグインの無効化やアンインストール処理が中断してしまいます。🛠 提案する修正
if ($Page !== null) { $Layout = $em->getRepository(Layout::class)->find(Layout::DEFAULT_LAYOUT_UNDERLAYER_PAGE); $PageLayout = $em->getRepository(PageLayout::class)->findOneBy(['Page' => $Page, 'Layout' => $Layout]); - $em->remove($PageLayout); + if ($PageLayout !== null) { + $em->remove($PageLayout); + } $em->remove($Page); $em->flush(); }🤖 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 `@PluginManager.php` around lines 154 - 161, Before calling EntityManager::remove() in the Page deletion block, check whether $PageLayout returned by findOneBy() is non-null; remove it only when present, while always removing $Page and flushing as currently done.
🤖 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/main.yml:
- Line 1:
ワークフロー全体の権限設定としてpermissionsを追加し、GITHUB_TOKENを読み取り専用に制限してください。CIで必要な権限のみを明示し、それ以外の権限は付与しない構成にします。
- Around line 166-168: Update the plugin test block in the workflow so a missing
app/Plugin/${PLUGIN_CODE}/Tests directory causes the CI job to fail instead of
silently skipping PHPUnit. Keep PHPUnit execution unchanged when the directory
exists, and make the missing-directory branch explicitly exit with a nonzero
status.
- Around line 210-213: Update the “Setup PHP” workflow step to pin
nanasess/setup-php to commit 3f36b116d024300a7df978629a2bad9afceb2be3 instead of
tracking master.
In `@Controller/TwoFactorAuthCustomerAppController.php`:
- Around line 170-176: Update verifyCode() so its authKey and token parameters
accept nullable strings, then handle null values before calling
$this->tfa->verifyCode(). Return false for missing inputs and invoke the
verifier only when both arguments are non-null.
---
Outside diff comments:
In `@PluginManager.php`:
- Around line 154-161: Before calling EntityManager::remove() in the Page
deletion block, check whether $PageLayout returned by findOneBy() is non-null;
remove it only when present, while always removing $Page and flushing as
currently done.
🪄 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: 1260bfa6-9678-4eb3-9ec8-93ce1b988942
📒 Files selected for processing (14)
.github/workflows/main.yml.github/workflows/release.ymlController/TwoFactorAuthCustomerAppController.phpEntity/CustomerTrait.phpEvent.phpForm/Type/Extension/Admin/TwoFactorAuthCustomerTypeExtension.phpForm/Type/TwoFactorAuthAppTypeCustomer.phpPluginManager.phpREADME.mdResource/.php-cs-fixer.dist.phpResource/rector.phpTests/bootstrap.phpcomposer.jsonphpunit.xml.dist
| @@ -1,4 +1,4 @@ | |||
| name: CI for TwoFactorAuthCustomerApp42 | |||
| name: CI for TwoFactorAuthCustomerApp44 | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
GITHUB_TOKEN を読み取り専用に制限してください。
permissions がないため、トークン権限がリポジトリ・組織の既定値に依存します。このCIに必要な権限を明示してください。GitHubも最小権限の明示を推奨しています。 (docs.github.com)
修正案
name: CI for TwoFactorAuthCustomerApp44
+permissions:
+ contents: read
+📝 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: CI for TwoFactorAuthCustomerApp44 | |
| name: CI for TwoFactorAuthCustomerApp44 | |
| permissions: | |
| contents: read |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 1-281: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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/main.yml at line 1,
ワークフロー全体の権限設定としてpermissionsを追加し、GITHUB_TOKENを読み取り専用に制限してください。CIで必要な権限のみを明示し、それ以外の権限は付与しない構成にします。
Source: Linters/SAST tools
| if [ -d "app/Plugin/${PLUGIN_CODE}/Tests" ]; then | ||
| ./vendor/bin/phpunit -c app/Plugin/${PLUGIN_CODE}/phpunit.xml.dist app/Plugin/${PLUGIN_CODE}/Tests | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
テストディレクトリがない場合もCIを失敗させてください。
現在はプラグインの配置不備やテスト欠落が発生しても、PHPUnitを一度も実行せず成功します。
修正案
- if [ -d "app/Plugin/${PLUGIN_CODE}/Tests" ]; then
- ./vendor/bin/phpunit -c app/Plugin/${PLUGIN_CODE}/phpunit.xml.dist app/Plugin/${PLUGIN_CODE}/Tests
- fi
+ test -d "app/Plugin/${PLUGIN_CODE}/Tests"
+ ./vendor/bin/phpunit -c app/Plugin/${PLUGIN_CODE}/phpunit.xml.dist app/Plugin/${PLUGIN_CODE}/Tests📝 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 [ -d "app/Plugin/${PLUGIN_CODE}/Tests" ]; then | |
| ./vendor/bin/phpunit -c app/Plugin/${PLUGIN_CODE}/phpunit.xml.dist app/Plugin/${PLUGIN_CODE}/Tests | |
| fi | |
| test -d "app/Plugin/${PLUGIN_CODE}/Tests" | |
| ./vendor/bin/phpunit -c app/Plugin/${PLUGIN_CODE}/phpunit.xml.dist app/Plugin/${PLUGIN_CODE}/Tests |
🤖 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/main.yml around lines 166 - 168, Update the plugin test
block in the workflow so a missing app/Plugin/${PLUGIN_CODE}/Tests directory
causes the CI job to fail instead of silently skipping PHPUnit. Keep PHPUnit
execution unchanged when the directory exists, and make the missing-directory
branch explicitly exit with a nonzero status.
| - name: Setup PHP | ||
| uses: nanasess/setup-php@master | ||
| with: | ||
| php-version: '8.5' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 固定候補のSHAを取得し、内容を監査してから workflow に設定する
gh api repos/nanasess/setup-php/commits/master --jq '.sha'Repository: EC-CUBE/TwoFactorAuthCustomerApp42
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '200,220p' .github/workflows/main.yml
echo
echo "== commit existence check =="
gh api repos/nanasess/setup-php/commits/3f36b116d024300a7df978629a2bad9afceb2be3 --jq '.sha'Repository: EC-CUBE/TwoFactorAuthCustomerApp42
Length of output: 827
nanasess/setup-php を 3f36b116d024300a7df978629a2bad9afceb2be3 に固定してください。 master のままだと更新がそのまま CI に入ります。
🤖 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/main.yml around lines 210 - 213, Update the “Setup PHP”
workflow step to pin nanasess/setup-php to commit
3f36b116d024300a7df978629a2bad9afceb2be3 instead of tracking master.
| /** | ||
| * 認証コードを取得. | ||
| * | ||
| * @param string $authKey | ||
| * @param string $token | ||
| * | ||
| * @return boolean | ||
| * 認証コードを検証. | ||
| */ | ||
| private function verifyCode($authKey, $token) | ||
| private function verifyCode(string $authKey, string $token): bool | ||
| { | ||
| return $this->tfa->verifyCode($authKey, $token, 1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
セッション消失等の予期せぬ状態での TypeError （500エラー）を防ぐため、引数に null を許容してください。
PHP 8 では型宣言が厳格であるため、string 型の引数に null を渡すと TypeError が発生してシステムエラーとなります。
フォームのCSRFトークン検証（isValid()）によってセッション切れリクエストの多くは事前に弾かれますが、予期せぬ状態やイレギュラーなPOSTにおいて $auth_key が null のまま到達した場合にクラッシュするリスクがあります。引数に ?string を指定し、内部で安全に null チェックを行うことを推奨します。
🛠 提案する修正
- private function verifyCode(string $authKey, string $token): bool
+ private function verifyCode(?string $authKey, ?string $token): bool
{
+ if ($authKey === null || $token === null) {
+ return false;
+ }
return $this->tfa->verifyCode($authKey, $token, 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.
| /** | |
| * 認証コードを取得. | |
| * | |
| * @param string $authKey | |
| * @param string $token | |
| * | |
| * @return boolean | |
| * 認証コードを検証. | |
| */ | |
| private function verifyCode($authKey, $token) | |
| private function verifyCode(string $authKey, string $token): bool | |
| { | |
| return $this->tfa->verifyCode($authKey, $token, 1); | |
| } | |
| /** | |
| * 認証コードを検証. | |
| */ | |
| private function verifyCode(?string $authKey, ?string $token): bool | |
| { | |
| if ($authKey === null || $token === null) { | |
| return false; | |
| } | |
| return $this->tfa->verifyCode($authKey, $token, 1); | |
| } |
🤖 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 `@Controller/TwoFactorAuthCustomerAppController.php` around lines 170 - 176,
Update verifyCode() so its authKey and token parameters accept nullable strings,
then handle null values before calling $this->tfa->verifyCode(). Return false
for missing inputs and invoke the verifier only when both arguments are
non-null.
1 participant
概要
会員向けアプリ認証（TOTP）プラグインを EC-CUBE 4.4（Symfony 7.4 / Doctrine ORM 3.0 / PHP 8.2+） に対応させ、コードを
TwoFactorAuthCustomerApp42→TwoFactorAuthCustomerApp44に改名します。親プラグイン依存もTwoFactorAuthCustomer42→TwoFactorAuthCustomer44に更新します。4.3 とは非互換（属性必須・ORM 3・PHP 8.2+）のため、新規4.4ブランチへの取り込みです。参考: 4.3→4.4 マイグレーション手順 (doc #346) /
変更内容
1. EC-CUBE 4.4 対応（コア移行）
@Route/@Template→#[Route]/#[Template]（Sensio依存除去）、EntityExtension の@EntityExtension→#[EntityExtension]（Eccube\Attribute\EntityExtension）Types::STRING）、メソッド引数・戻り値型（createSecret(): string/verifyCode(...): bool等）enable/disable/uninstallほか関連メソッドに: void@TwoFactorAuthCustomerApp44/...およびページ登録パスをTwoFactorAuthCustomerApp44に統一requireをec-cube/twofactorauthcustomer44へ、code/version: 4.4.0をTwoFactorAuthCustomerApp44に統一phpunit.xml.distを<source>/<extensions>形式へ、Tests/bootstrap.phpを追加2. 動作改善（初回APP認証）
/mypage/two_factor_auth/app/create）でセッション切れ等により秘密鍵が欠落した場合、鍵を再発行して入力画面を出し直すフォールバックを追加verifyCodeを実行3. 静的解析・整形ツール
Resource/rector.php/Resource/.php-cs-fixer.dist.phpを追加（.php設定は本体のPlugin\:サービス検出で 500 を避けるためResource/配下に配置）4. CI（
.github/workflows/main.ymlほか）checkout@v4・$GITHUB_OUTPUT化TwoFactorAuthCustomer44を checkout / archive し、mock-package-api+eccube:composer:requireで依存解決のうえ有効化--ignore-platform-req=ext-redis: Symfony 7.4 のsymfony/cacheが古いphp-redisと衝突して本体 composer install が失敗するのを回避（redis は未使用）cache:warmup: 有効プラグインのルート確定のため warmup を実行（Tests がある場合のみ PHPUnit 実行）release.yml: 配布パッケージからResource/rector.php/Resource/.php-cs-fixer.dist.phpを除外テスト
release.yml相当）から開発用ファイルが除外されることを確認Summary by CodeRabbit