fix: Rector の Attribute 引数順序で非推奨 Sensio クラスのパラメータ名を使わない (#6540)#6884
Open
nanasess wants to merge 5 commits into
Open
fix: Rector の Attribute 引数順序で非推奨 Sensio クラスのパラメータ名を使わない (#6540)#6884nanasess wants to merge 5 commits into
nanasess wants to merge 5 commits into
Conversation
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>
- 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>
6 tasks
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes非推奨クラスマッピング対応
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
🧹 Nitpick comments (1)
tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache / IsGranted / Security マッピングのテストが不足しています。
DEPRECATED_CLASS_MAPPINGにはTemplate以外にCache・IsGranted・Securityの3クラスも定義されていますが、fixture はdeprecated_template_attribute.php.incのみで、これら3クラスの変換結果を検証するテストがありません。将来的なリグレッションを検知できるよう、それぞれに対応する fixture の追加をご検討ください。🤖 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 `@tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php` around lines 29 - 32, The AttributeArgumentsOrderRectorTest data provider only covers the Template mapping, so add fixtures for the Cache, IsGranted, and Security entries from DEPRECATED_CLASS_MAPPING and make provideData() pick them up via yieldFilesFromDirectory. Use the existing test class and its Fixture directory to add one fixture per class so the rector’s conversion behavior is verified for all deprecated attribute mappings.
🤖 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.
Nitpick comments:
In `@tests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.php`:
- Around line 29-32: The AttributeArgumentsOrderRectorTest data provider only
covers the Template mapping, so add fixtures for the Cache, IsGranted, and
Security entries from DEPRECATED_CLASS_MAPPING and make provideData() pick them
up via yieldFilesFromDirectory. Use the existing test class and its Fixture
directory to add one fixture per class so the rector’s conversion behavior is
verified for all deprecated attribute mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beb3b334-3287-4633-b47d-311082008690
📒 Files selected for processing (5)
src/Eccube/Rector/CodingStyle/AttributeArgumentsOrderRector.phptests/Eccube/Tests/Rector/CodingStyle/AttributeArgumentsOrderRectorTest.phptests/Eccube/Tests/Rector/CodingStyle/Fixture/deprecated_template_attribute.php.inctests/Eccube/Tests/Rector/CodingStyle/Fixture/route_attribute_order.php.inctests/Eccube/Tests/Rector/CodingStyle/config/configured_rule.php
テスト env では Symfony DebugClassLoader が有効で、kernel 起動後にグローバル関数 bcround() から Symfony\Polyfill\Php84\Php84 を遅延 autoload すると Php84::bcround() が解決できず、PHP 8.2/8.3 で税計算(TaxRuleService::bcround)を通す 全テストが「Call to undefined method Php84::bcround()」で失敗していた (bootstrap 時点や standalone では bcround は正常に動作する)。 tests/bootstrap.php で kernel 起動前に Php84 クラスを class_exists で事前ロードし回避する。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AttributeArgumentsOrderRectorTest は Rector を実行するため rector 同梱の
nikic/php-parser を読み込む。pcov 有効のカバレッジ実行では本体の
nikic/php-parser と衝突し「Cannot redeclare class PhpParser\Node\Scalar\Float_」で
fatal になり、coverage1.xml が生成されず coverage / Upload coverage reports が
失敗していた。テストに #[Group('rector')] を付与し、coverage.yml の phpunit
実行に --exclude-group rector を追加して回避する。通常の unit-test では
引き続き実行される。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6884 +/- ##
=======================================
Coverage 75.32% 75.32%
=======================================
Files 519 519
Lines 25483 25484 +1
=======================================
+ Hits 19196 19197 +1
Misses 6287 6287
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:
|
ttokoro20240902
approved these changes
Jul 14, 2026
ttokoro20240902
approved these changes
Jul 14, 2026
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
2 participants
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)
Closes #6540
カスタム Rector ルール
AttributeArgumentsOrderRectorが、@Template("...")を#[Template(data: '...')]と誤った名前付き引数へ変換してしまう問題を修正します。原因
AnnotationToAttributeRectorがアノテーションを Attribute へ変換した時点では、まだuse 文が
Sensio\Bundle\FrameworkExtraBundle\Configuration\Template（非推奨）を指している。AttributeArgumentsOrderRectorがリフレクションで Sensio のTemplateを調べ、第一パラメータ$dataを読み取ってdata:を付与する。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 の提案に準拠）:
...\Configuration\TemplateSymfony\Bridge\Twig\Attribute\Template...\Configuration\CacheSymfony\Component\HttpKernel\Attribute\Cache...\Configuration\IsGrantedSymfony\Component\Security\Http\Attribute\IsGranted...\Configuration\SecuritySymfony\Component\Security\Http\Attribute\IsGranted実装に関する補足(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(...)]引数並べ替えが従来どおり動作する（リグレッションガード）マイナーバージョン互換性保持のための制限事項チェックリスト
🤖 Generated with Claude Code
Summary by CodeRabbit