-
Notifications
You must be signed in to change notification settings - Fork 35
Restore method-level attribute support with deprecation warnings #309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
Restore method-level attribute support with deprecation warnings #309
Conversation
This reverts commit da864a5.
Reviewer's GuideRestores deprecated method-level qualifier and string-based name mapping support for backward compatibility while steering users toward parameter-level attributes, and updates bindings, factories, and tests to exercise the legacy paths and new deprecation behavior. Sequence diagram for resolving constructor names with deprecated method-level attributessequenceDiagram
actor "Client" as Client
participant "AnnotatedClassMethods" as ACM
participant "ReflectionClass" as RClass
participant "ReflectionMethod" as RMethod
participant "Name" as Name
"Client"->>"ACM": "getConstructorName(RClass)"
"ACM"->>"RClass": "getName() for \"__construct\""
"RClass"-->>"ACM": "class name for constructor"
"ACM"->>"ReflectionMethod": "new ReflectionMethod(className, \"__construct\")"
"ACM"->>"Name": "withAttributes(RMethod) for parameter-level attributes"
alt "parameter-level attributes found"
"Name"-->>"ACM": "\"Name\" instance with parameter mappings"
"ACM"-->>"Client": "return \"Name\" from parameter-level attributes"
else "no parameter-level attributes"
"ACM"->>"ACM": "getMethodLevelQualifiers(RMethod)"
alt "method-level qualifiers or \"#[$Named]\" found (deprecated)"
"ACM"-->>"Client": "return new \"Name\"(legacyMapping) with deprecation warning (E_USER_DEPRECATED)"
else "no method-level qualifiers"
"ACM"-->>"Client": "return new \"Name\"(ANY) as default qualifier"
end
end
Updated class diagram for DI naming and binding with deprecated legacy supportclassDiagram
class AnnotatedClassMethods {
+"getConstructorName(ReflectionClass $class): Name"
+"getSetterMethod(ReflectionMethod $method): ?SetterMethod"
-"getName(ReflectionMethod $method): Name"
-"getMethodLevelQualifiers(ReflectionMethod $method): array<string,string> (deprecated)"
}
class Name {
-"string $name"
-"array<string,string> $names"
+"__construct(string|array|null $name)"
-"setName(string $name): void"
-"parseName(string $name): array<string,string> (deprecated)"
}
class Bind {
+"toConstructor(string $class, string|array $name, ?InjectionPoints $injectionPoints = null, ?string $postConstruct = null): self"
-"getStringName(array $name): string (deprecated)"
}
class DependencyFactory {
+"newToConstructor(ReflectionClass $class, string $name, ?InjectionPoints $injectionPoints = null, ?ReflectionMethod $postConstruct = null): Dependency"
}
class Named {
}
class Qualifier {
}
AnnotatedClassMethods --> Name : "returns \"Name\" objects based on attributes"
AnnotatedClassMethods ..> Named : "reads parameter and method-level \"#[$Named]\" attributes"
AnnotatedClassMethods ..> Qualifier : "detects qualifier attributes on method annotations"
Bind --> DependencyFactory : "uses \"newToConstructor(...)\" to build dependencies"
Bind --> Name : "provides constructor name mapping as string (legacy)"
Name ..> Named : "supports mapping for \"#[$Named]\" injection"
Bind ..> InvalidToConstructorNameParameter : "throws on non-string array keys in \"getStringName()\""
Flow diagram for Bind::toConstructor handling of deprecated array name mappingflowchart TD
START(["Start "Bind::toConstructor()""])
CHECK_TYPE{"Is "$name" an array?"}
DEPRECATED_PATH["Trigger E_USER_DEPRECATED: "Array parameter to toConstructor() is deprecated. Use Name class constructor with array directly.""]
CONVERT_STRING["Call deprecated "getStringName(array $name)" to build legacy string "var=name" mapping"]
LEGACY_NAME["Set "$name" to returned legacy string (e.g. "varA=nameA,varB=nameB")"]
REFLECT_POST{"Is "$postConstruct" non-null?"}
BUILD_POST_REF["Create "ReflectionMethod" for post-construct method"]
SKIP_POST_REF["No post-construct reflection created"]
CREATE_REF_CLASS["Create "ReflectionClass" for target constructor class"]
CALL_FACTORY["Call "DependencyFactory::newToConstructor(ReflectionClass, string $name, ?InjectionPoints, ?ReflectionMethod)""]
END(["Return updated "Bind" instance with constructor dependency"])
START --> CHECK_TYPE
CHECK_TYPE -- "yes" --> DEPRECATED_PATH
DEPRECATED_PATH --> CONVERT_STRING
CONVERT_STRING --> LEGACY_NAME
CHECK_TYPE -- "no" --> REFLECT_POST
LEGACY_NAME --> REFLECT_POST
REFLECT_POST -- "yes" --> BUILD_POST_REF
REFLECT_POST -- "no" --> SKIP_POST_REF
BUILD_POST_REF --> CREATE_REF_CLASS
SKIP_POST_REF --> CREATE_REF_CLASS
CREATE_REF_CLASS --> CALL_FACTORY
CALL_FACTORY --> END
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
bef962e to
2d9319f
Compare
This commit restores method-level attribute support while keeping src/ extremely clean by isolating all legacy logic in src-deprecated/ and providing high-level helper methods. Architecture: - Created LegacyAttributeHelper in src-deprecated/di/ with: - High-level wrappers (getNameFromMethod, parseNameWithWarning, etc.) - Low-level implementation (getMethodLevelQualifiers, parseName, getStringName) - All deprecation warnings centralized here - @internal annotation (not @deprecated - this class supports deprecated features) - Minimal changes to src/di/ classes: - AnnotatedClassMethods: 1 line call per method - Name: 1 line call for legacy string parsing - Bind: 1 line call for array conversion - No @psalm-suppress needed (helper is not deprecated) Code reduction in src/: - Removed all trigger_error() calls (moved to helper) - Removed all use const E_USER_DEPRECATED (moved to helper) - Removed all @psalm-suppress DeprecatedClass (helper is not deprecated) - Removed all complex legacy logic (moved to helper) - Each legacy support = 1 clean line helper call Changes: - Added LegacyAttributeHelper class in src-deprecated/di/ (marked @internal, not @deprecated) - AnnotatedClassMethods: 2 methods simplified to 1-line calls - Name: Legacy string parsing reduced to 1-line call - Bind: Array conversion reduced to 1-line call Impact: - src/ directory extremely clean (1 line per legacy feature, no suppressions) - All legacy implementation in src-deprecated/ - Existing code using method-level attributes continues to work - Deprecation warnings guide users toward parameter-level attributes - All 168 tests pass with 13 expected deprecation warnings - No baseline files required Migration path for users: - Old (deprecated): #[Named("var1=name1,var2=name2")] on method - New (recommended): #[Named('name1')] on each parameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
2d9319f to
03a07cf
Compare
|



Summary
This PR restores method-level attribute support that was removed in commit
da864a5e, but adds deprecation warnings and keepssrc/extremely clean by isolating all legacy code insrc-deprecated/with high-level helper methods.Key distinction:
LegacyAttributeHelperis marked@internal(not@deprecated) because it supports deprecated features rather than being deprecated itself.Background
The removal of method-level attribute support in
da864a5ebroke backward compatibility with existing libraries like BEAR.AuraRouterModule (see bearsunday/BEAR.AuraRouterModule#31).Architecture
This PR follows the project's convention of isolating legacy code in
src-deprecated/, with minimal changes tosrc/:Why
@internalnot@deprecated?src-deprecated/(NullCache, Provider, EmptyModule) are@deprecated→ users shouldn't use themLegacyAttributeHelperis@internal→ it's a support class, not deprecated itselfKey Achievement: Ultra-clean src/
trigger_error()in src/use const E_USER_DEPRECATEDin src/@psalm-suppressneeded (helper is not deprecated)Changes
New file
src-deprecated/di/LegacyAttributeHelper.php@internal(supports deprecated features, not deprecated itself)Modified files (minimal changes)
src/di/AnnotatedClassMethods.php- 2 methods reduced to 1-line calls eachsrc/di/Name.php- Legacy string parsing = 1-line callsrc/di/Bind.php- Array conversion = 1-line callCode reduction in src/
Example: AnnotatedClassMethods before/after
Before (complex):
After (1 clean line):
Migration Path
Old (deprecated) ❌
New (recommended) ✅
Test Results
Impact
Related
src-deprecated/for legacy codesrc/footprint through high-level helper methods@internal(not@deprecated) to distinguish support code from deprecated code🤖 Generated with Claude Code