Skip to content

fix: Rector の Attribute 引数順序で非推奨 Sensio クラスのパラメータ名を使わない (#6540)#6877

Closed
nanasess wants to merge 5 commits into
EC-CUBE:4.4from
nanasess:fix/6540-rector-deprecated-class-param-name
Closed

fix: Rector の Attribute 引数順序で非推奨 Sensio クラスのパラメータ名を使わない (#6540)#6877
nanasess wants to merge 5 commits into
EC-CUBE:4.4from
nanasess:fix/6540-rector-deprecated-class-param-name

Conversation

@nanasess

@nanasess nanasess commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

概要(Overview・Refs Issue)

Closes #6540

カスタム Rector ルール AttributeArgumentsOrderRector が、@Template("...")
#[Template(data: '...')]誤った名前付き引数へ変換してしまう問題を修正します。

原因

  1. AnnotationToAttributeRector がアノテーションを Attribute へ変換した時点では、まだ
    use 文が Sensio\Bundle\FrameworkExtraBundle\Configuration\Template(非推奨)を指している。
  2. AttributeArgumentsOrderRector がリフレクションで Sensio の Template を調べ、第一パラメータ
    $data を読み取って data: を付与する。
  3. その後 RenameClassRector が use 文を Symfony\Bridge\Twig\Attribute\Template へ rename するが、
    名前付き引数 data: はそのまま残る。

Symfony\Bridge\Twig\Attribute\Template の第一パラメータは $template$data は存在しないため、
#[Template(data: '...')] は実行時に Unknown named parameter $data となる実害があります。

方針(Policy)

AttributeArgumentsOrderRector に「非推奨クラス → 新しい Attribute クラス」のマッピング
DEPRECATED_CLASS_MAPPING)を追加し、リフレクション対象のクラス名を新クラスへ事前差し替えします。
これにより置換後クラスの正しいパラメータ順序(Template$template 等)が使われます。
差し替えるのはリフレクション対象のみで、出力される Attribute 名ノードには手を加えません。

対象マッピング(#6540 の提案に準拠):

非推奨(Sensio) 新クラス
...\Configuration\Template Symfony\Bridge\Twig\Attribute\Template
...\Configuration\Cache Symfony\Component\HttpKernel\Attribute\Cache
...\Configuration\IsGranted Symfony\Component\Security\Http\Attribute\IsGranted
...\Configuration\Security Symfony\Component\Security\Http\Attribute\IsGranted

新クラスがオートロードできない場合は従来どおり ReflectionException で空配列を返してスキップするため、安全側にフォールバックします。

実装に関する補足(Appendix)

  • src/Eccube/Rector/CodingStyle/AttributeArgumentsOrderRector.php: DEPRECATED_CLASS_MAPPING 定数を追加し、getConstructorParameterOrder() のリフレクション前にクラス名を差し替え。
  • tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php(+ config / Fixture)を追加。

テスト(Test)

AbstractRectorTestCase ベースのルール単体テストを追加し、ローカルで確認済み:

  • use Sensio\...\Template; 配下の #[Template('@admin/Content/cache.twig')]
    #[Template(template: '@admin/Content/cache.twig')] へ変換される(修正の本丸)
  • 通常の #[Route(...)] 引数並べ替えが従来どおり動作する(リグレッションガード)
  • マッピングを無効化すると本テストが赤くなり、有効化で緑になることを確認(修正がテストで守られている)
vendor/bin/phpunit tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php
OK (2 tests)

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

  • 既存機能の仕様変更はありません(開発時のみ使う Rector ルールの修正)
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

🤖 Generated with Claude Code

Summary by CodeRabbit

  • 新機能
    • 非推奨のアノテーション表記を対応する属性へ置き換えつつ、引数の並び順をより正確に整える変換に対応しました。
    • テンプレート指定・ルート指定で名前付き引数形式に対応しました。
  • バグ修正
    • 変換時に誤ったパラメータ名が使われるケースを抑え、結果の安定性を改善しました。
  • テスト
    • 変換内容を検証する自動テストとフィクスチャを追加しました。
  • Chores
    • Composerキャッシュの保存手順を最適化しました。

AnnotationToAttributeRector が Sensio FrameworkExtraBundle のアノテーションを
Attribute 化した直後、まだ use 文が Sensio クラスを指す段階で
AttributeArgumentsOrderRector が走ると、リフレクションが Sensio 側の
コンストラクタ第一パラメータ(Template の $data 等)を読み取り、後段の
RenameClassRector でクラスだけ置換されても誤った名前付き引数
(#[Template(data: '...')])が残っていた。Symfony Bridge の Template には
data 引数が存在しないため、これは実害(Unknown named parameter)になる。

リフレクション対象のクラス名を新しい Attribute クラスへ事前に差し替える
DEPRECATED_CLASS_MAPPING を追加し、置換後クラスの正しいパラメータ順序
(Template の $template 等)を使うようにした。出力される Attribute 名ノードには
手を加えない。

テスト(AttributeArgumentsOrderRectorTest)を追加し、Sensio Template の
@template が #[Template(template: '...')] へ変換されること、および通常の
Route 引数並べ替えが従来どおり動作することを検証。

Refs EC-CUBE#6540

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

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5ea89162-486f-4d23-9992-a73419b30bf4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

AttributeArgumentsOrderRector で非推奨Sensioクラス名をSymfony Attributeへ差し替える処理を追加し、専用テストとフィクスチャを整備しました。あわせて、Composerキャッシュの参照先とキーを更新しています。

Changes

AttributeArgumentsOrderRector 非推奨クラスマッピング修正とテスト追加

Layer / File(s) Summary
DEPRECATED_CLASS_MAPPINGの追加とリフレクション対象クラス名の差し替え
src/Eccube/Rector/CodingStyle/AttributeArgumentsOrderRector.php
Sensio→Symfony Attributeへのマッピング定数を追加し、getConstructorParameterOrder でリフレクション前にクラス名を変換するよう変更。
テスト・フィクスチャ・設定ファイルの追加
tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php, tests/Eccube/Tests/Rector/CodingStyle/config/configured_rule.php, tests/Eccube/Tests/Rector/CodingStyle/Fixture/deprecated_template_attribute.php.inc, tests/Eccube/Tests/Rector/CodingStyle/Fixture/route_attribute_order.php.inc
Fixtureディレクトリのファイルをデータプロバイダで網羅するテストクラスと、非推奨Templateアトリビュート・Routeアトリビュートのフィクスチャ、AttributeArgumentsOrderRectorのみを有効化する設定ファイルを追加。

Composerキャッシュ設定の更新

Layer / File(s) Summary
Composerキャッシュの参照先とキー更新
.github/actions/composer/action.yml
Composerキャッシュディレクトリ取得ステップを追加し、キャッシュの参照先とキー、restore-keysを更新。

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

🐇 古いSensioの名札を外して
template: がぴょこんと前に出た
キャッシュもきゅっと整って
うさぎは鼻をひくひく、満足げ
ぴょん、っと直っていい感じ!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Composer キャッシュ設定の変更は Rector の属性引数順序修正と無関係で、リンクされた要件の範囲外です。 この変更は別PRに分離し、AttributeArgumentsOrderRector の修正とテストだけを残してください。
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed タイトルは AttributeArgumentsOrderRector で Sensio の非推奨クラスのパラメータ名を使わない修正を簡潔に表しており、変更内容と一致しています。
Linked Issues check ✅ Passed 非推奨 Sensio クラスを Symfony Attribute にマッピングして反射前に解決し、Template の template: 化と既存の順序検証テストも追加されています。
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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: 1

🤖 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 `@tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php`:
- Around line 1-12: The test file header order is inconsistent with the
convention used under tests/, so add declare(strict_types=1); immediately after
the opening PHP tag and before the EC-CUBE license comment block. Update the
top-of-file bootstrap in AttributeArgumentsOrderRectorTest so it follows the
standard <?php → declare(strict_types=1); → license header sequence used by
other test classes.
🪄 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: f9c6be38-c142-4ba0-b645-943ad12f530b

📥 Commits

Reviewing files that changed from the base of the PR and between 0067e77 and d019c4b.

📒 Files selected for processing (5)
  • src/Eccube/Rector/CodingStyle/AttributeArgumentsOrderRector.php
  • tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php
  • tests/Eccube/Tests/Rector/CodingStyle/Fixture/deprecated_template_attribute.php.inc
  • tests/Eccube/Tests/Rector/CodingStyle/Fixture/route_attribute_order.php.inc
  • tests/Eccube/Tests/Rector/CodingStyle/config/configured_rule.php

- DEPRECATED_CLASS_MAPPING の @var を array<string, class-string> に修正
  (キーは削除済み Sensio クラスの文字列リテラルで class-string 判定不可)
- Rector テストの @dataProvider を #[DataProvider] 属性へ移行し
  declare(strict_types=1) を付与

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nanasess
nanasess force-pushed the fix/6540-rector-deprecated-class-param-name branch from 5eb82d8 to 3d95cce Compare July 1, 2026 07:40
@nanasess
nanasess force-pushed the fix/6540-rector-deprecated-class-param-name branch from 163ee69 to 606c520 Compare July 3, 2026 04:31
nanasess and others added 2 commits July 3, 2026 13:33
composer アクションは vendor/ を prefix restore-keys でキャッシュしていたため、
別の composer.lock で作られた vendor/ が復元され、composer install が
「Nothing to install」で古い・不整合な vendor をそのまま使う状態が発生していた。
その結果、PHP 8.2/8.3 で polyfill-php84 の Php84::bcround() が未解決となり
(Php84.php が古い版のまま bootstrap.php だけ新しい等の混在)、税計算(bcround)を
通る全テストが 600 件エラーになっていた。lock を変えてもキーが変わるだけで
restore-keys が別 vendor を引き continue するため解消できなかった。

ステップ名(Get Composer Cache Directory)本来の意図どおり、キャッシュ対象を
composer のダウンロードキャッシュ(composer config cache-files-dir)へ変更。
vendor/ は毎回 composer install でクリーンに構築されるため不整合が根絶される。
キー接頭辞も composer- → composer-cache- に変え、旧 vendor キャッシュを引かない。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
polyfill-php84 1.37.0 は PHP 8.2/8.3 で bootstrap80.php 経由の bcround() が
Php84::bcround() 未解決になる壊れた実装で、fresh install でも
「Call to undefined method Symfony\Polyfill\Php84\Php84::bcround()」で
税計算(bcround)を通る全テストが失敗する(CI の fresh install で実証)。
4.4 本体が緑なのは古い working キャッシュで延命しているだけの潜在バグ。

1.38.1 は bootstrap80.php を廃し PHP>=8.2 を bootstrap82.php に振り分け
bcround を正しく提供する。4.4 が既に php85=1.38.1 なのに php84 だけ 1.37.0 で
取り残されていた不整合の是正でもある(PHP 8.3 で bcround 動作を確認済み)。
先行の composer アクション修正(fresh install 化)と併せて根絶する。

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

nanasess commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

最新の 4.4 からブランチを作り直した #6884 に置き換えます。本 PR はクローズします。

@nanasess nanasess closed this Jul 3, 2026
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.

[4.3-symfony7] AttributeArgumentsOrderRector で非推奨クラスのパラメータ名が使われる問題

1 participant