Release v3.0.0: Complete rewrite with PHP 8.1+ support #1
Conversation
Complete rewrite with modern architecture and comprehensive testing: BREAKING CHANGES: - Requires PHP 8.1+ (was PHP 7.1+) - New namespace: Calliostro\Discogs (was Discogs) - Ultra-lightweight 2-class architecture replaces complex service layer - ClientFactory::create() replaces service configuration - Magic method calls for all 65+ Discogs API endpoints - Modern PSR-4 autoloading with src/ directory structure Features: - 97.01% code coverage with comprehensive test suite (37 tests) - PHPStan Level 8 static analysis with full type safety - Reduced from 86 to 54 dependencies for minimal footprint - Direct API endpoint mapping with intuitive method names - OAuth 1.0a and Personal Access Token authentication - Complete Discogs API coverage: artists, releases, collections, marketplace - GitHub Actions CI testing across PHP 8.1-8.5 - Comprehensive migration guide and documentation Quality Assurance: - PSR-12 code standards via phpcs/phpcbf - Modern PHPUnit 10.5+ with latest testing practices - Clean git history with proper .gitignore - Professional CHANGELOG without marketing bloat - Zero technical debt and production-ready codebase Migration: See UPGRADE.md for detailed migration instructions from v2.x
- Change coverage from 'none' to 'xdebug' in test jobs - Fixes PHPUnit warning 'No code coverage driver available' - Prevents test failures due to failOnWarning=true setting
- Old package versions (PHPUnit 10.0.0, PHPStan 1.0.0) cause compatibility issues - 20+ deprecation warnings with modern PHP versions - Focus on stable dependencies across all PHP 8.1-8.5 versions - Cleaner, more reliable CI pipeline
- Replace squizlabs/php_codesniffer with friendsofphp/php-cs-fixer - Add .php-cs-fixer.php configuration with PSR-12 rules - Update composer scripts for cs and cs-fix commands - Achieve 100% code coverage (67/67 lines, 8/8 methods) - Add comprehensive test cases for all edge cases - Enhance CI workflow with PHP 8.1-8.5 matrix and stability testing - Add codecov.yml configuration for coverage reporting - Update README badges to remove branch-specific links - Add .php-cs-fixer.cache to .gitignore - Update CHANGELOG for v3.0.0 final release
- Ensure coverage metadata requirements are met for all test runs
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull Request Overview
This pull request represents a complete rewrite of the php-discogs-api library, transitioning from v2.x to v3.0 with significant architectural changes. The library is now ultra-lightweight with only 2 main classes, supports PHP 8.1+, and provides direct magic method API calls.
Key changes:
- Complete architectural rewrite from Guzzle Services to magic methods
- Modernization to PHP 8.1+ with strict typing and enhanced error handling
- Comprehensive testing with 100% coverage and improved CI/CD pipeline
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DiscogsApiClient.php | New main API client class with magic method calls and service-based operations |
| src/ClientFactory.php | Simplified factory for creating authenticated and unauthenticated clients |
| resources/service.php | Streamlined service configuration with 65+ API endpoints definitions |
| tests/ | Complete test suite rewrite with unit and integration tests |
| composer.json | Updated dependencies, PHP version requirement, and project metadata |
| README.md | Comprehensive documentation update reflecting new architecture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Test method call with null as parameters (edge case) | ||
| // @phpstan-ignore-next-line - Testing edge case | ||
| $result = $this->client->search(null); |
There was a problem hiding this comment.
Passing null as parameters could lead to unexpected behavior. The method should explicitly handle null parameters by either rejecting them with a clear error message or treating them as an empty array. Consider adding type validation in the __call method to ensure parameters are always arrays.
src/DiscogsApiClient.php
Outdated
| */ | ||
| public function __call(string $method, array $arguments): array | ||
| { | ||
| $params = $arguments[0] ?? []; |
There was a problem hiding this comment.
The method assumes the first argument is always an array but doesn't validate this assumption. If a non-array value is passed (like null in the test), it could cause issues downstream. Consider adding type validation: $params = is_array($arguments[0] ?? []) ? ($arguments[0] ?? []) : [];
| $params = $arguments[0] ?? []; | |
| $params = is_array($arguments[0] ?? null) ? $arguments[0] : []; |
tests/Unit/DiscogsApiClientTest.php
Outdated
| // Test with an extremely long method name to potentially trigger edge cases | ||
| $longMethodName = str_repeat('A', 1000) . 'Get'; |
There was a problem hiding this comment.
Creating a 1000-character string in a test may be excessive and could impact test performance. Consider using a smaller but still meaningful size (e.g., 100 characters) that would still test the edge case effectively without unnecessary memory allocation.
| // Test with an extremely long method name to potentially trigger edge cases | |
| $longMethodName = str_repeat('A', 1000) . 'Get'; | |
| // Test with a long method name (100 characters) to potentially trigger edge cases | |
| $longMethodName = str_repeat('A', 100) . 'Get'; |
src/DiscogsApiClient.php
Outdated
| private function convertMethodToOperation(string $method): string | ||
| { | ||
| // Split a camelCase into parts | ||
| $parts = preg_split('/(?=[A-Z])/', $method, -1, PREG_SPLIT_NO_EMPTY); |
There was a problem hiding this comment.
The preg_split could potentially return false on error, which isn't being handled. While unlikely with this pattern, consider using $parts = preg_split('/(?=[A-Z])/', $method, -1, PREG_SPLIT_NO_EMPTY) ?: [] to ensure $parts is always an array.
| $parts = preg_split('/(?=[A-Z])/', $method, -1, PREG_SPLIT_NO_EMPTY); | |
| $parts = preg_split('/(?=[A-Z])/', $method, -1, PREG_SPLIT_NO_EMPTY) ?: []; |
| if (!$parts) { | ||
| return $method; | ||
| } |
There was a problem hiding this comment.
The preg_split could potentially return false on error, which isn't being handled. While unlikely with this pattern, consider using $parts = preg_split('/(?=[A-Z])/', $method, -1, PREG_SPLIT_NO_EMPTY) ?: [] to ensure $parts is always an array.
- Improve parameter validation in __call method to handle null/non-array inputs - Add error handling for preg_split operations using null coalescing operator - Optimize test performance by reducing test string length from 1000 to 100 chars - Simplify PHPUnit configuration by removing coverage requirements from default config - Remove xdebug warning for users running composer test without coverage setup
Breaking Changes
Calliostro\Discogs\*(wasDiscogs\*)Changes
ClientFactoryandDiscogsApiClient$client->artistGet(['id' => '123'])See CHANGELOG.md for migration details.