Introduce domain type aliases and PHP 8.5 support #38
Conversation
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
Reviewer's GuideThis PR adds a centralized domain type alias file, migrates metadata classes to use these aliases with proper PHPDoc and Psalm imports, enforces runtime type assertions for full Psalm inference, applies minor readability tweaks, marks exceptions as final, and updates the CI matrix to support PHP 8.5. Class diagram for new domain type aliases in Types.phpclassDiagram
class Types {
<<final>>
+AppName: non-empty-string
+Context: non-empty-string
+AppDir: non-empty-string
+TmpDir: non-empty-string
+LogDir: non-empty-string
+UriPath: non-empty-string
+FilePath: non-empty-string
+Scheme: non-empty-string
}
Updated class diagram for AppMeta metadata classesclassDiagram
class AbstractAppMeta {
+AppName name
+AppDir appDir
+TmpDir tmpDir
+LogDir logDir
+getResourceListGenerator(): Generator
+getGenerator(scheme: Scheme): Generator<ResMeta>
-camel2kebab(str: non-empty-string): void
}
class Meta {
<<final>>
+Meta(name: AppName, context: Context, appDir: string)
-getAppDir(name: AppName): AppDir
}
AbstractAppMeta <|-- Meta
class ResMeta {
<<final>>
+ResMeta(uriPath: UriPath, class: class-string<ResourceObject>, filePath: FilePath)
+UriPath uriPath
+class-string<ResourceObject> class
+FilePath filePath
}
Class diagram for updated exception classesclassDiagram
class AppNameException {
<<final>>
AppNameException extends LogicException
}
class NotWritableException {
<<final>>
NotWritableException extends LogicException
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #38 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 19 19
===========================================
Files 3 3
Lines 40 45 +5
===========================================
+ Hits 40 45 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Relying on PHP’s assert() for enforcing non-empty-string invariants can be disabled at runtime; consider using explicit runtime checks or lightweight value objects to guarantee these domain types.
- In the Meta constructor docblock the
$appDirparameter is still marked asstringrather than using theAppDiralias—import and apply@param AppDirthere for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on PHP’s assert() for enforcing non-empty-string invariants can be disabled at runtime; consider using explicit runtime checks or lightweight value objects to guarantee these domain types.
- In the Meta constructor docblock the `$appDir` parameter is still marked as `string` rather than using the `AppDir` alias—import and apply `@param AppDir` there for consistency.
## Individual Comments
### Comment 1
<location> `src/Meta.php:59-64` </location>
<code_context>
}
- return dirname((string) (new ReflectionClass($module))->getFileName(), 3);
+ $fileName = (new ReflectionClass($module))->getFileName();
+ assert($fileName !== false);
+
+ /** @var AppDir $dir */
</code_context>
<issue_to_address>
**suggestion:** ReflectionClass::getFileName() can return false for internal classes.
If $module is an internal class, this will fail. Consider throwing a more descriptive exception for easier debugging.
```suggestion
$fileName = (new ReflectionClass($module))->getFileName();
if ($fileName === false) {
throw new \RuntimeException(
sprintf(
'Cannot determine file name for module "%s". The class may be internal and does not have a file name.',
$module
)
);
}
/** @var AppDir $dir */
$dir = dirname($fileName, 3);
assert($dir !== '.');
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces semantic type definitions via Psalm and strengthens type safety across the codebase. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
This commit modernizes the codebase with better type safety following BEAR.Package conventions. Type System Improvements: - Add Types.php with domain type aliases (AppName, Context, AppDir, TmpDir, LogDir, UriPath, FilePath, Scheme) - Remove duplicate @psalm-* annotations in favor of standard PHPDoc annotations - Use @psalm-import-type to import type aliases across classes - Add proper type assertions to achieve 100% Psalm type inference Code Quality: - Mark exception classes as final (AppNameException, NotWritableException) - Split long lines for better readability (AbstractAppMeta:64) - Remove unnecessary string casts (AbstractAppMeta:90) - Use promoted properties annotations correctly in ResMeta 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add PHP 8.5 to CI test matrix - Move PHP 8.4 to old_stable versions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Meta.php (1)
60-67: Good assert message; resolves prior concern.The explicit message on getFileName() false improves debuggability during development.
🧹 Nitpick comments (6)
.github/workflows/continuous-integration.yml (1)
12-13: Quote 8.5 to avoid YAML float coercion.Pass it as a string for consistency with old_stable and many reusable workflow inputs.
- current_stable: 8.5 + current_stable: "8.5"If the reusable workflow expects a string, this prevents type surprises.
src/Types.php (1)
7-23: Harden the types container and narrow Scheme.
- Prevent accidental instantiation of Types with a private constructor.
- Scheme is only 'app' | 'page' | '*'; reflect that for stronger checks.
/** * Domain Types * @psalm-type AppName = non-empty-string * @psalm-type Context = non-empty-string @@ - * @psalm-type Scheme = non-empty-string + * @psalm-type Scheme = "app" | "page" | "*" */ final class Types { + private function __construct() + { + } }src/ResMeta.php (1)
15-23: Make promoted properties readonly.They are value metadata and not mutated; readonly communicates intent and prevents accidental writes.
public function __construct( - public string $uriPath, - public string $class, - public string $filePath, + public readonly string $uriPath, + /** @var class-string<ResourceObject> */ + public readonly string $class, + public readonly string $filePath, ) { }src/Meta.php (1)
37-45: Consider removing error suppression on mkdir or capturing context.The pattern works but hides diagnostics. Optionally capture warnings or retry on race without @ to aid debugging.
- if (! file_exists($this->tmpDir) && ! @mkdir($this->tmpDir, 0777, true) && ! is_dir($this->tmpDir)) { + if (! file_exists($this->tmpDir) && ! mkdir($this->tmpDir, 0777, true) && ! is_dir($this->tmpDir)) { throw new NotWritableException($this->tmpDir); }If noisy in CI, keep @ but log last error when throwing.
src/AbstractAppMeta.php (2)
76-87: Guard against empty $path before indexing.Rare, but defensive check avoids undefined offset if a class unexpectedly lacks the expected segments.
foreach ($this->getResourceListGenerator() as [$class, $file]) { /** @var array<non-empty-string> $paths */ $paths = explode('\\', $class); $path = array_slice($paths, 3); array_walk($path, [$this, 'camel2kebab']); + if ($path === []) { + continue; // defensive: unexpected class structure + } if ($scheme === '*') { /** @var non-empty-string $schemeValue */ $schemeValue = $path[0];
38-49: Optional: consider typed or readonly properties in next major.Typing (and possibly readonly) for $name, $appDir, $tmpDir, $logDir would encode invariants at runtime, but could be BC-impacting in 1.x. Consider for 2.x.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/continuous-integration.yml(1 hunks)src/AbstractAppMeta.php(3 hunks)src/Exception/AppNameException.php(1 hunks)src/Exception/NotWritableException.php(1 hunks)src/Meta.php(2 hunks)src/ResMeta.php(1 hunks)src/Types.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
src/Exception/NotWritableException.php (1)
9-11: Finalizing exception class looks good.Clearer API surface; prevents unintended inheritance.
src/Exception/AppNameException.php (1)
9-11: LGTM: finalized exception.Consistent with the exception hierarchy tightening.
src/AbstractAppMeta.php (1)
110-113: camel2kebab: LGTM.Asserting non-empty result is a nice safety net.
- Upgrade squizlabs/php_codesniffer from ^3.7 to ^4.0 - Upgrade doctrine/coding-standard from 12.0.0 to 14.0.0 - Upgrade slevomat/coding-standard from 8.22.1 to 8.24.0 This removes deprecated sniff warnings: - SlevomatCodingStandard.TypeHints.UnionTypeHintFormat (replaced by DNFTypeHintFormat) - Squiz.WhiteSpace.LanguageConstructSpacing (replaced by Generic.WhiteSpace.LanguageConstructSpacing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add explicit configuration for bamarni-bin plugin to suppress deprecation warnings: - bin-links: true (maintain current behavior) - forward-command: false (maintain current behavior) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Update minimum PHP version from 8.0 to 8.1 (8.0 is EOL) - Update phpcs.xml to use PHP 8.1 compatibility - Update rector.php to use UP_TO_PHP_81 level set - Add ReadOnlyPropertyRector for PHP 8.1 features 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Use PHP 8.1 readonly properties for ResMeta's promoted constructor properties to ensure immutability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update bear/resource from ^1.0 to ^1.26 to fix PHP 8.1+ deprecation warnings related to ArrayAccess return type declarations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove phpmetrics as it depends on an old version of nikic/php-parser that doesn't support PHP 8.1 readonly keyword syntax. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
PHPUnit 9.5 uses an old nikic/php-parser that doesn't support the readonly keyword. Update to ^9.6 which includes php-parser with PHP 8.1 support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
vendor-bin tools are not needed for CI tests, which run with --no-plugins. Removing these scripts prevents Psalm PHP 8.5 compatibility issues in CI.
Normalize file paths from getResourceListGenerator() to ensure cross-platform compatibility. Remove unused var_dump import.
This reverts commit 642499d.
Partial revert of cd4ea86 to restore original psr4list version requirement while keeping PHP 8.1 and other necessary updates.
- Add private constructor to prevent instantiation - Narrow Scheme type from non-empty-string to literal union 'app'|'page'|'*'
The private constructor prevents instantiation and is not meant to be tested.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR modernizes BEAR.AppMeta with domain type aliases following BEAR.Package conventions, upgrades to PHP 8.1 with readonly properties, and adds full PHP 8.5 CI support.
Major Changes
Type System Improvements
Types.phpwith domain type aliases (AppName, Context, AppDir, TmpDir, LogDir, UriPath, FilePath, Scheme)@psalm-import-typepattern across all classes@psalm-*annotations in favor of standard PHPDocPHP 8.1 Upgrade
readonlyproperties to ResMeta value objectDependency Management
bear/resourceto^1.0for maximum compatibility (was^1.26)koriym/psr4listto^1.0.2for maximum compatibility (was^1.4)post-install-cmdandpost-update-cmdfor CI compatibilityforward-command: truein bamarni-bin configPHP 8.5 Support
Code Quality
Testing
Backward Compatibility
^1.0instead of^1.26) improve compatibility with older projects🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]