Skip to content

feat(admin): .env が反映されない環境で設定変更を警告・抑止する (#6130) - #6959

Open
ttokoro20240902 wants to merge 5 commits into
4.4from
feature/issue-6130-env-not-effective-warning
Open

feat(admin): .env が反映されない環境で設定変更を警告・抑止する (#6130)#6959
ttokoro20240902 wants to merge 5 commits into
4.4from
feature/issue-6130-env-not-effective-warning

Conversation

@ttokoro20240902

@ttokoro20240902 ttokoro20240902 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

概要(Overview・Refs Issue)

Refs #6130

セキュリティ設定・テンプレート設定は .env を書き換えて機能を実現しているが、docker-compose の環境変数や本番環境など .env が使われない場面では書き換えても反映されず、「変更しようとして初めて効かないことに気付く」ユーザーが多かった。

.env への書き込みが実行時のロードに反映されない状況を検出し、対象画面(セキュリティ設定 / テンプレート設定)で 警告表示・登録ボタン無効化・サーバ側での保存拒否を行う。

方針(Policy)

  • .env が反映されない4条件を EnvFileService(新設)に集約し、両画面で共通利用(DRY):
    1. .env が存在しない(.env 未使用)
    2. .env に書き込み権限がない
    3. .env.local.php(dump-env の最適化済みスナップショット)が存在し .env より優先される
    4. その画面が書き込む環境変数が OS のプロセス環境変数として設定され .env を上書きしている
  • 多層防御(GET=警告+ボタン無効化 / POST=サーバ側拒否)で「押せるのに無反映」「気付けない」を両面から防ぐ。
  • 既存の TemplateController は環境変数オーバーライド時に「書き込み+警告」だったが、書き込みは実行時に無反映のため保存拒否に統一(無反映の書き込みをやめ、ユーザーに明示)。
  • 検出ロジックは既存 TemplateController の getenv() 判定(4.3 由来・テスト有り)を踏襲し一般化。

実装に関する補足(Appendix)

  • EnvFileService::getIneffectiveReasons(array $keys) が理由コード(REASON_*)を返し、コントローラは理由ごとに admin.system.env.ineffective.<reason> の警告を出す。
  • $keys は各画面が書き込む環境変数(Security=7キー / Template=ECCUBE_TEMPLATE_CODE)。OS 環境変数オーバーライド判定に使用。
  • Symfony の bootEnv は OS のプロセス環境変数を上書きしないため、getenv() が値を返すキーは .env 変更が無反映になる(既存挙動に準拠)。

テスト(Test)

  • EnvFileServiceTest(新規・単体): 4条件の検出を一時ディレクトリで検証(書込不可は root 実行環境では自動スキップ)。
  • SecurityControllerTest: 環境変数オーバーライド時に保存が拒否され .env が書き換わらないことを追加検証。
  • TemplateControllerTest: オーバーライド時の挙動を「書き込み+警告」から「保存拒否+エラー」に更新。
  • ローカル(Docker)で PHPUnit 全ケース green を確認(PHPStan / php-cs-fixer / lint:twig / lint:yaml も通過)。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません(※テンプレート設定の環境変数オーバーライド時のみ「無反映の書き込み+警告」から「保存拒否+エラー」に変更。反映されない書き込みを廃止するもので、実際に反映されていた動作の変更はありません)
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません(envWritable を追加のみ)
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません(新規 EnvFileService の追加のみ)
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

🤖 Generated with Claude Code

Summary by CodeRabbit

  • 新機能
    • 管理画面で、.env の反映可否を判定し、反映できない理由(不在、書き込み不可、.env.local.php 優先、環境変数による上書き)を表示できるようになりました(表示文言を追加)。
  • 改善
    • セキュリティ設定・テンプレート設定は、反映不可の状態では保存/登録を拒否し、登録ボタンも無効化されます。
  • テスト
    • .env と環境変数の条件別挙動(警告表示、保存拒否、理由表示)を追加・更新しました。

セキュリティ設定・テンプレート設定は .env を書き換えて機能するが、
docker-compose の環境変数や本番環境など .env が使われない場面では
書き換えても反映されず、ユーザーが変更できないことに気付けなかった。

- EnvFileService を新設。.env への書き込みが実行時に反映されない理由
  (不在 / 書込不可 / .env.local.php スナップショット / 対象キーが
  OS 環境変数でオーバーライド)を検出する。
- SecurityController / TemplateController に適用し、反映されない状況では:
  - GET 時に該当理由の警告を表示し、登録ボタンを無効化
  - POST 時はサーバ側で保存を拒否(多層防御)
- 4 理由の汎用メッセージを i18n(ja/en) に追加。
- EnvFileService の単体テスト、両画面の反映不可時テストを追加。

Refs #6130

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c17e615e-200c-467e-8fa8-79302314247e

📥 Commits

Reviewing files that changed from the base of the PR and between 04bf71f and 000627c.

📒 Files selected for processing (2)
  • src/Eccube/Resource/locale/messages.en.yaml
  • src/Eccube/Resource/locale/messages.ja.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Eccube/Resource/locale/messages.en.yaml
  • src/Eccube/Resource/locale/messages.ja.yaml

📝 Walkthrough

Walkthrough

EnvFileServiceを追加し、.envの反映可否を統一判定します。セキュリティ設定とテンプレート設定の保存を制御し、理由別警告、登録ボタンの無効化、関連テストと翻訳を追加します。

Changes

環境設定ファイル反映可否

Layer / File(s) Summary
EnvFileServiceの判定ロジック
src/Eccube/Service/EnvFileService.php, tests/Eccube/Tests/Service/EnvFileServiceTest.php
.envの存在・書き込み可否、.env.local.php、プロセス環境変数による上書きを判定し、各状態をテストします。
管理画面の保存判定と警告
app/config/eccube/services.yaml, src/Eccube/Controller/Admin/..., tests/Eccube/Tests/Web/Admin/...
両コントローラにEnvFileServiceを注入し、反映不能時の保存拒否、理由別警告、envWritableの提供を追加します。関連する保存拒否テストも更新します。
画面表示と理由別メッセージ
src/Eccube/Resource/locale/messages.*.yaml, src/Eccube/Resource/template/admin/...
反映不能理由の翻訳を追加し、envWritableがfalseの場合に登録ボタンを無効化します。

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant 管理者
  participant 管理画面
  participant EnvFileService
  participant .env
  管理者->>管理画面: 設定またはテンプレートを送信
  管理画面->>EnvFileService: isEffective(ENV_KEYS)
  EnvFileService->>.env: 状態と上書きを確認
  EnvFileService-->>管理画面: 反映可否と理由
  管理画面-->>管理者: 保存、警告、またはエラー
Loading

Possibly related PRs

  • EC-CUBE/ec-cube#6759: ECCUBE_TEMPLATE_CODEの環境変数上書き時における警告・保存拒否の流れが関連します。
  • EC-CUBE/ec-cube#6932: .env.local.php.envの状態に応じた管理画面の判定が関連します。

Suggested labels: affected:template, security

Suggested reviewers: nanasess, dotani1111

Poem

ぴょんと判定、envの道
書けぬ理由を四つ告げ
ボタンはそっとお休みし
書ける時だけ跳ね進む
うさぎも安心、設定ぴょん!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed .env が反映されない環境での管理画面設定変更を警告・抑止する内容で、PR の主目的を適切に要約しています。
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/issue-6130-env-not-effective-warning

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@src/Eccube/Resource/locale/messages.en.yaml`:
- Line 1430: Update the admin.system.env.ineffective.local_php message in
src/Eccube/Resource/locale/messages.en.yaml at lines 1430-1430 and
src/Eccube/Resource/locale/messages.ja.yaml at lines 1430-1430 to provide
executable recovery steps: after updating server-side .env values, rerun
composer dump-env <env> or dotenv:dump, or delete .env.local.php to resume
updates from the administration screen.

In `@tests/Eccube/Tests/Service/EnvFileServiceTest.php`:
- Around line 97-105: 各テストで環境変数を変更する前に既存値を保存し、finally
で保存値を復元してください。EnvFileServiceTest.php の
ECCUBE_TEST_OVERRIDE_KEY、SecurityControllerTest.php の
ECCUBE_ADMIN_ROUTE、TemplateControllerTest.php の ECCUBE_TEMPLATE_CODE
を対象とし、未設定だった場合のみ環境変数を削除して、事前設定値を保持してください。

In `@tests/Eccube/Tests/Web/Admin/Setting/System/SecurityControllerTest.php`:
- Around line 88-89: テストメソッド testSubmitRejectedWhenEnvOverridden() に戻り値型 void
を宣言してください。
🪄 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 Plus

Run ID: a1aaa9fe-9586-4627-bbac-04f6fae14233

📥 Commits

Reviewing files that changed from the base of the PR and between b1c2436 and 82bbf22.

📒 Files selected for processing (11)
  • app/config/eccube/services.yaml
  • src/Eccube/Controller/Admin/Setting/System/SecurityController.php
  • src/Eccube/Controller/Admin/Store/TemplateController.php
  • src/Eccube/Resource/locale/messages.en.yaml
  • src/Eccube/Resource/locale/messages.ja.yaml
  • src/Eccube/Resource/template/admin/Setting/System/security.twig
  • src/Eccube/Resource/template/admin/Store/template.twig
  • src/Eccube/Service/EnvFileService.php
  • tests/Eccube/Tests/Service/EnvFileServiceTest.php
  • tests/Eccube/Tests/Web/Admin/Setting/System/SecurityControllerTest.php
  • tests/Eccube/Tests/Web/Admin/Store/TemplateControllerTest.php

Comment thread src/Eccube/Resource/locale/messages.en.yaml Outdated
Comment thread tests/Eccube/Tests/Service/EnvFileServiceTest.php Outdated
Comment thread tests/Eccube/Tests/Web/Admin/Setting/System/SecurityControllerTest.php Outdated
ttokoro20240902 and others added 2 commits July 23, 2026 18:01
- messages(ja/en): local_php の案内を実行可能な手順に修正
  (管理画面ではブロックされるため、サーバ側で .env 更新後に dump-env 再実行、
  または .env.local.php 削除で再開、と明示)
- テスト3件: putenv での復元を「無条件 unset」から「事前値を保存し復元」に変更し、
  実行環境が設定済みの環境変数を消さないようにする
- SecurityControllerTest::testSubmitRejectedWhenEnvOverridden に戻り値型 void を付与

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
POST 拒否に加え、GET 時の「警告表示」と「登録ボタン無効化」(多層防御の
警告・抑止側)を Security/Template 両画面で回帰検証する。
環境変数オーバーライド状態で画面を開き、submit ボタンに disabled が付き、
反映されない旨の警告メッセージが描画されることを確認する。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ttokoro20240902 and others added 2 commits July 24, 2026 11:07
Rector (RemoveUnusedRequestParamRector) の指摘に対応。CI の rector ジョブを緑にする。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.07%. Comparing base (1cfea14) to head (000627c).

Files with missing lines Patch % Lines
...roller/Admin/Setting/System/SecurityController.php 0.00% 5 Missing ⚠️
...cube/Controller/Admin/Store/TemplateController.php 37.50% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              4.4    #6959      +/-   ##
==========================================
- Coverage   77.11%   77.07%   -0.04%     
==========================================
  Files         547      548       +1     
  Lines       27163    27184      +21     
==========================================
+ Hits        20946    20953       +7     
- Misses       6217     6231      +14     
Flag Coverage Δ
Unit 77.07% <64.28%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nanasess nanasess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#6130 の対応として、.env への書き込みが実行時に反映されない状況を検出して警告・ボタン無効化・サーバ側拒否を行う、という方針は妥当だと思います。判定ロジックを EnvFileService に集約して 2 画面で共有している点、GET/POST の多層防御にしている点も良いと思いました。特に、旧 TemplateController.env 不在時に「ECCUBE_TEMPLATE_CODE だけを含む .env を新規作成する」(=.env.dist へのフォールバックを壊してアプリごと落ちうる)挙動だったのを拒否に変えたのは、明確な改善だと思います。

一方で、判定ロジックそのものについて、index.php の実際のブート経路と突き合わせると 反映されるはずの保存をブロックしてしまうケース検出できないケース が残っているようです。ローカルで index.php の 2 経路を再現して実測したので、その結果を根拠に個別コメントしました。

確認した内容:

  • gh pr diff の全 11 ファイル(+370 / -33)
  • vendor/symfony/dotenv/Dotenv.phpbootEnv() / loadEnv() / populate() の実装
  • index.php / bin/consoleboot_env() 呼び出し(overrideExistingVars の分岐)と src/Eccube/Resource/functions/env.phpenv()
  • docker-compose.yml / .github/workflows/*.yml の環境変数設定状況
  • 旧翻訳キーの参照有無、対象 twig の submit ボタン、app/config/eccube/services.yaml の登録パターン
  • CI: 115 チェックすべて pass(E2E の admin-system.spec.ts はセキュリティ設定画面の submit を含みますが、php -S 側の env に ECCUBE_ADMIN_ROUTE は渡っていないため今回の変更に抵触していません)

主な指摘は次の 3 点です。

  1. [高] getenv() のみの判定は、index.phpboot_env(..., true) を使う経路(OS 環境変数 APP_ENV 未設定 = 非 Docker の一般的な構成)で誤検知になり、実際には反映される保存をブロックします(実測あり)。
  2. [高] セキュリティ設定は 7 キーの OR 判定なので、ECCUBE_ADMIN_ROUTE を 1 つ設定しているだけで、.env に書けば反映される他 6 キー(IP 制限・SSL 強制・TRUSTED_HOSTS)まで保存できなくなります。
  3. [中] .env.local / .env.$APP_ENV / .env.$APP_ENV.local.env を上書きしますが検出されません(実測あり)。index.php:23.env.local を設定の源泉として扱っているので、対象外にはしづらいと思います。

CodeRabbit の既指摘 3 件(putenv の復元 / : void / local_php の文言)は対応済みであることを確認したので、重複しては挙げていません。

Comment on lines +68 to +74
// bootEnv は OS のプロセス環境変数を上書きしないため, getenv が値を返すキーは .env の変更が反映されない
foreach ($keys as $key) {
if (false !== getenv($key)) {
$reasons[] = self::REASON_OVERRIDDEN;
break;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[高] getenv() だけの判定では、.env が勝つ経路を「上書きされている」と誤検知し、反映される保存をブロックします。

index.php は OS 環境変数 APP_ENV の有無で読み込み方を変えています。

  • index.php:35APP_ENV 未設定 = 非 Docker の一般的な構成): boot_env(__DIR__.'/.env', true)overrideExistingVars = true なので .env の値が OS のプロセス環境変数を上書きする
  • index.php:47APP_ENV 設定済み = Docker 等): boot_env(__DIR__.'/.env', false) → OS 環境変数が勝つ

一方 Symfony Dotenv は putenv() を使わない(usePutenv=false)ため、getenv() は前者でも OS 側の値を返し続けます。手元で index.php の 2 経路を再現した実測結果です(.envECCUBE_ADMIN_ROUTE=fromdotenv、OS 環境変数に ECCUBE_ADMIN_ROUTE=osvalue):

経路 getenv() env()(実際に効く値) SYMFONY_DOTENV_VARS に含まれるか
A: APP_ENV 未設定 → boot_env(.., true) osvalue fromdotenv yes
B: APP_ENV 設定済み → boot_env(.., false) osvalue osvalue no

A では .env の値が実際に効いている(=管理画面からの変更は反映される)にもかかわらず、本サービスは REASON_OVERRIDDEN を返すため、SecurityController / TemplateController は保存を拒否し、登録ボタンも無効化されます。これは今まで動いていた操作ができなくなる退行になります。

なお、ブート後は bootEnv()$_SERVER['APP_ENV'] を必ずセットするため、コントローラ実行時点で「どちらの経路だったか」を isset($_SERVER['APP_ENV']) で判別することはできません。上表のとおり、実際の勝敗と一致するのは SYMFONY_DOTENV_VARS(Dotenv が実際に populate したキーの一覧)です。Dotenv::populate() 自身も getenv() を避けて(実装中のコメント: "don't check existence with getenv() because of thread safety issues")このリストで判断しています。コア側の env()src/Eccube/Resource/functions/env.php:55-64)も $_ENV$_SERVERgetenv() の順で、getenv() は互換のためのフォールバック扱いです。本サービスだけ解決順が逆になっています。

// Dotenv が実際に .env 系から populate したキーは SYMFONY_DOTENV_VARS に載る
$dotenvVars = array_flip(array_filter(explode(',', $_SERVER['SYMFONY_DOTENV_VARS'] ?? $_ENV['SYMFONY_DOTENV_VARS'] ?? '')));

foreach ($keys as $key) {
    if (isset($dotenvVars[$key])) {
        continue; // .env 系の値が実際に適用されている = 書き込みは反映される
    }
    if (isset($_ENV[$key]) || isset($_SERVER[$key]) || false !== getenv($key)) {
        $reasons[] = self::REASON_OVERRIDDEN;
        break;
    }
}

この形にすると、A/B の判別に加えて、nginx の fastcgi_param や Apache の SetEnv のように $_SERVER 側にだけ入ってくる値(Dotenv::populate()isset($_ENV[$name]) を見てスキップするので、この場合も .env は負けます)も拾えるようになります。

Comment on lines +32 to +40
private const ENV_KEYS = [
'ECCUBE_FRONT_ALLOW_HOSTS',
'ECCUBE_FRONT_DENY_HOSTS',
'ECCUBE_ADMIN_ALLOW_HOSTS',
'ECCUBE_ADMIN_DENY_HOSTS',
'ECCUBE_FORCE_SSL',
'TRUSTED_HOSTS',
'ECCUBE_ADMIN_ROUTE',
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[高] 7 キーの OR 判定なので、1 キーが OS 環境変数として設定されているだけで、反映されるはずの残り 6 キーまで保存できなくなります。

EnvFileService::getIneffectiveReasons() は対象キーのどれか 1 つでも該当すれば REASON_OVERRIDDEN を返して break します(EnvFileService.php:69-74)。その結果、isEffective(self::ENV_KEYS)SecurityController.php:65)はセキュリティ設定画面全体をブロックします。

ECCUBE_ADMIN_ROUTE だけを OS 環境変数で設定する構成は現実的で、docker-compose.yml:50-59 も各キーを個別にコメントアウトして例示しています(ECCUBE_ADMIN_ROUTE は 54 行目)。この構成では:

  • ECCUBE_ADMIN_ROUTE の変更 → 確かに反映されない(ブロックは妥当)
  • TRUSTED_HOSTS / ECCUBE_FORCE_SSL / IP 制限 4 キー → .env に書けば反映される

にもかかわらず、後者も含めて登録ボタンが無効化され、POST も拒否されます。index.php:41-46 のコメント(「OS 環境変数を保護しつつ、.env 側の非 OS 変数は反映される」)が示すとおり、Docker 環境でも .env 側のキーは生きているので、これは今まで管理画面から変更できていた設定が変更できなくなる退行になります。PR 本文の「既存機能の仕様変更はありません」とも齟齬があると思います。

getIneffectiveReasons() が「どのキーが上書きされているか」まで返すようにして、

  • 上書きされているキーを名指しで警告する
  • 該当キーに対応するフォーム項目だけ無効化する(あるいは書き込み時に該当キーだけスキップする)
  • 画面全体のブロックは not_found / not_writable / local_php(=キーによらず全滅するケース)に限定する

という切り分けはいかがでしょうか。少なくとも「1 キーの上書きで画面全体を止める」のは過剰だと思います。

Comment on lines +63 to +66
// .env.local.php があると bootEnv は .env より優先するため, .env の変更は反映されない
if (file_exists($this->projectDir.'/.env.local.php')) {
$reasons[] = self::REASON_LOCAL_PHP;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[中] .env.local / .env.$APP_ENV / .env.$APP_ENV.local.env を上書きしますが、検出されていません。

Dotenv::loadEnv().env.env.local.env.$env.env.$env.local の順にカスケード読み込みし、後から読んだファイルが .env の値を上書きします(doLoad()populate() は、既に dotenv が読んだキーであれば SYMFONY_DOTENV_VARS 経由で上書きを許可するため)。この挙動は本リポジトリの src/Eccube/Resource/functions/env.php:19-20 の docblock 自身にも書かれています。

また index.php:23 / bin/console:19.env.local$dotenvExists の判定に含めており、EC-CUBE として正式にサポートされている配置です。

同じ検証スクリプトでの実測結果です(.envECCUBE_ADMIN_ROUTE=fromdotenv):

構成 env()(実際に効く値) isEffective(['ECCUBE_ADMIN_ROUTE'])
.env のみ fromdotenv true(正しい)
.env + .env.local fromlocal true ← 検出漏れ
.env + .env.prod fromprod true ← 検出漏れ

.env.local.php と同じ「書いても反映されないのに気付けない」ケースなので、本 PR の目的(#6130)からすると取りこぼしたくないところだと思います。

ただし .env.local.php(dump-env のスナップショットなので全キーを含む)と違い、これらは特定のキーだけを定義していることが多いので、ファイルの存在だけで一律ブロックすると逆に過剰になります。Dotenv::parse() で該当ファイルを読み、対象キーが定義されている場合だけ理由に加える、という形が実態に合うと思いますが、いかがでしょうか。

$appEnv = $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? null;
$cascades = array_filter([
    '.env.local',
    $appEnv ? ".env.{$appEnv}" : null,
    $appEnv ? ".env.{$appEnv}.local" : null,
]);

admin.system.env.ineffective.not_found: '.env ファイルが存在しないため、この設定は管理画面から変更できません。サーバーの環境変数を直接設定してください。'
admin.system.env.ineffective.not_writable: '.env ファイルに書き込み権限がないため、この設定は管理画面から変更できません。.env ファイルの書き込み権限を確認してください。'
admin.system.env.ineffective.local_php: '.env.local.php が存在するため、.env への変更は反映されず、管理画面からは変更できません。サーバー側で .env を更新してから composer dump-env を再実行するか、.env.local.php を削除して管理画面からの変更を再開してください。'
admin.system.env.ineffective.overridden: 'この設定の環境変数が OS のプロセス環境変数として設定されているため、.env への変更は反映されません。サーバーの環境変数を直接変更してください。'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[中] どの環境変数が原因なのかがメッセージから分かりません。

セキュリティ設定は 7 キーを対象にしているため(SecurityController.php:32-40)、この文言だけでは管理者がサーバ側のどの環境変数を直せばよいか判断できません。置き換え前の admin.store.template.env_override_warningmessages.ja.yaml:1522)は ECCUBE_TEMPLATE_CODE を名指ししていたので、テンプレート設定に関しては案内内容が後退しています。

getIneffectiveReasons() が上書きされているキーを返すようにしたうえで、プレースホルダで埋め込む形はいかがでしょうか(en 側も同様)。

admin.system.env.ineffective.overridden: 'この設定の環境変数(%keys%)が OS のプロセス環境変数として設定されているため、.env への変更は反映されません。サーバーの環境変数を直接変更してください。'

上書きされているキーを名指しできれば、セキュリティ設定画面の過剰ブロック(SecurityController.php:32-40 のコメント参照)の解消にもそのまま使えると思います。

*/
#[Route(path: '/%eccube_admin_route%/store/template/{id}/download', name: 'admin_store_template_download', requirements: ['id' => '\d+'], methods: ['GET'])]
public function download(Request $request, \Eccube\Entity\Template $Template): BinaryFileResponse
public function download(\Eccube\Entity\Template $Template): BinaryFileResponse

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[中] download() / delete() からの Request $request 引数削除は、本 PR の目的と無関係な public シグネチャ変更です。

download()(113 行目)と delete()(170 行目)から Request $request が削除されていますが、.env の反映可否検出とは無関係な変更です。app/Customize やプラグインでこのコントローラを継承してメソッドをオーバーライドしている場合、シグネチャ不一致で fatal になります。

PR 本文の互換性チェックリストは Service クラスについてしか言及していませんが、コントローラの public メソッドも同じく拡張点なので、

  • 別 PR に分ける(未使用引数の整理としてまとめて実施する)
  • あるいは本 PR に含めるなら、PR 本文に破壊的変更として明記する

のどちらかが良いと思いますが、いかがでしょうか。4.4 はメジャー更新なので削除自体が不可能とは思いませんが、レビュー単位としては分けたほうが追いやすいと感じました。

@@ -1429,6 +1429,10 @@ admin.setting.system.security.ip_limit_invalid_ip_and_submask: "%ip%はIPv4/ビ
admin.setting.system.security.ip_limit_invalid_https: "httpの場合には設定できません。"
admin.setting.system.security.admin_url_warning: 管理画面URLは、セキュリティのため推測されにくいものを設定してください。
admin.setting.system.security.not_found_env_file: .envファイルが見つかりません。.envを利用していない場合はセキュリティ設定を管理画面から変更できません。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[低] 参照されなくなった翻訳キーが残っています。

本 PR で SecurityController / TemplateController の警告が admin.system.env.ineffective.* に置き換わった結果、以下の 2 キーは参照ゼロになっています(rg で確認)。

  • admin.setting.system.security.not_found_env_file(この行 / en は messages.en.yaml:1431
  • admin.store.template.env_override_warningmessages.ja.yaml:1522 / messages.en.yaml:1522

削除するか、あるいは env_override_warning のほうは「どのキーが原因か名指しする」という良い先例なので、admin.system.env.ineffective.overridden の文言改善(別コメント参照)に取り込んで整理するのが良いと思います。

Comment on lines +87 to +98
#[Group(name: 'cache-clear')]
public function testDisplayWarningAndDisableButtonWhenEnvOverridden(): void
{
$key = 'ECCUBE_ADMIN_ROUTE';
$original = getenv($key);
putenv($key.'=admin');
try {
$crawler = $this->client->request(Request::METHOD_GET, $this->generateUrl('admin_setting_system_security'));
$this->assertTrue($this->client->getResponse()->isSuccessful());

// 登録ボタンが無効化されている
$this->assertGreaterThan(0, $crawler->filter('button[type="submit"][disabled]')->count());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[低] テストについて 3 点。

  1. #[Group(name: 'cache-clear')] はクラスレベルの #[Group('cache-clear')](22 行目)と重複しているので不要だと思います(TemplateControllerTest 側はクラスレベル指定がないので必要です)。

  2. assertGreaterThan(0, $crawler->filter('button[type="submit"][disabled]')->count()) は、対照となる正常系(反映可能な環境では無効化されないこと)が無いため、envWritable の条件が壊れて常に disabled が付くようになっても検出できません。既存の testDisplayList 相当の GET で ->count() === 0 を確認する対照アサーションを足しておくと、退行を検出できるようになります。

  3. これは実装側の指摘(EnvFileService.php のコメント参照)と関係しますが、このテストは putenv() で環境変数を立てているため getenv() 経路しか再現できていません。tests/bootstrap.php:21bootEnv()overrideExistingVars = false)を使うので、index.php:35boot_env(..., true) 経路(.env が OS 環境変数に勝つケース)はテストで踏めていません。EnvFileServiceTest 側であれば $_SERVER / $_ENV / SYMFONY_DOTENV_VARS を直接組み立てて両経路を単体で検証できるので、そちらに寄せるのが現実的だと思います。

if (file_exists($this->getParameter('kernel.project_dir').'/.env') === false) {
// .env への書き込みが反映されない状況では保存を拒否する
if (!$this->envFileService->isEffective(self::ENV_KEYS)) {
$this->addError('admin.common.save_error', 'admin');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[低] POST 拒否時のメッセージが汎用の admin.common.save_error だけなので、理由が分かりません。

GET 時は admin.system.env.ineffective.<reason> で理由を出しているので、POST 拒否時も同じ理由メッセージを併せて出すと、ボタン無効化を回避して直接 POST が飛んできたケース(あるいは表示後に環境が変わったケース)でも原因が伝わります。

$reasons = $this->envFileService->getIneffectiveReasons(self::ENV_KEYS);
if ([] !== $reasons) {
    $this->addError('admin.common.save_error', 'admin');
    foreach ($reasons as $reason) {
        $this->addError('admin.system.env.ineffective.'.$reason, 'admin');
    }

    return $this->redirectToRoute('admin_setting_system_security');
}

TemplateController.php:72-76 も同様です。

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.

.env を変更できない場合、使用していない場合は管理画面に警告を表示したい

2 participants