Repository navigation
Analyse tests with Psalm without a baseline, upload to Shepherd, and test the provider docs - #108
Merged
Merged
Conversation
The services comment in config/swap.php told users to pass access_key to apilayer_fixer and apilayer_exchange_rates_data, which Exchanger rejects (they take api_key), showed 'api_key' => 'access_key' for currency_converter, and listed less than half of the registered services. List every service Exchanger registers with the options its processOptions() reads, and add the missing enterprise flags and the National Bank of Denmark to the docs.
Swap 4.6 pulls in Exchanger 2.11 with the provider fixes the config reference now documents. The 2.x-dev alias matches the branch policy of the sister packages.
Illuminate\Contracts\Cache\Repository extends Psr\SimpleCache\CacheInterface since Laravel 5.8, so on every Laravel version this package can install on (PHP 8.2+) the php-cache bridge fallback is unreachable. Drop it with its Laravel 5.7 docs, and test that the store named in swap.cache receives the cached rates.
Merge the package config in register() as the Laravel package docs recommend, publish it to the application's configPath() instead of a hand built base path, and register Swap\Swap as a container alias of the swap service rather than a second binding. The provider only registers the swap singleton, so implement DeferrableProvider and list Swap\Swap in provides(): it is now loaded when Swap is first resolved instead of on every request. Artisan's console kernel loads all deferred providers before running a command, so vendor:publish still sees the config path registered in boot(). Cover publishing, the alias, deferral, the custom HTTP client and request factory, services disabled with false and service order.
The provider and facade extend illuminate/support and use illuminate/contracts, but composer.json required neither, so nothing stopped installs on Laravel versions that were never tested. Require them for Laravel 12 and 13, the majors the test bench and CI cover. Laravel 11 is left out: every 11.x release has open security advisories, so Composer refuses to install it by default. Laravel 10 needs orchestra/testbench 8, which only runs on PHPUnit 10. CI now runs the tests for each PHP and Laravel pair; Laravel 13 needs PHP 8.3. Lumen, whose last release is built on Laravel 11, is dropped from the keywords.
The README promised Laravel 5.5+ and Lumen support. Name the Laravel versions CI tests instead, and keep the Lumen setup section with a note that it is no longer tested and only applies to earlier releases.
The composer cs:fix and cs:check scripts pointed at a php-cs-fixer binary that was never required. Require it and reuse Swap's config so the sister packages share one code style.
facade.php only checks with --lint; without it, it rewrites the docblock and exits 0, so the CI step always passed. The composer facade:fix and facade:lint scripts also lost their backslashes to the shell (ReflectionException for "SwapLaravelFacadesSwap") and passed a --write flag the tool does not know. Quote the class name, pass --lint to facade:lint and run that script in CI. Drop the @api tag that the documenter strips; Psalm's UnusedClass for the facade is suppressed in psalm.xml instead.
Give every workflow read-only contents permission, a timeout, and a concurrency group that cancels superseded pull request runs, and install dependencies with ramsey/composer-install for caching. Replace the Code Style workflow with Coding Standards, which runs composer cs:check next to the facade docblock lint and tells contributors which script fixes a failure.
Match Swap's PHPUnit settings so a warning or a PHPUnit deprecation fails CI instead of scrolling by, and print details for warnings, deprecations and skipped tests.
Run Psalm over tests/ with the PHPUnit plugin and fix what it reports instead of keeping a baseline. In the provider, read options through the config repository instead of the undeclared $app->config property, and only treat non-empty strings as cache, client or factory ids. The Laravel plugin infers config types from the shipped config/swap.php, so the options and services arrays are annotated with the types any user config can have. The two values taken from the container stay mixed; Builder rejects anything that is not a client or request factory, so those calls carry a suppression. The tests get void return types, final classes, typed helpers for the nullable Testbench application, and drop the HTTP client, request factory and cache tests that the recording tests already cover. With tests analysed, the facade is referenced, so its UnusedClass suppression in psalm.xml is no longer needed.
Report Psalm issues as GitHub annotations, and on pushes to master upload type coverage to shepherd.dev for the new README badge, as Swap and Exchanger do.
Fail when a service Exchanger registers is missing from the services reference in config/swap.php or from the README provider tables, with an explicit skip list (array, and fixer_apilayer in the README). The reference is also parsed and every documented entry is built through the provider: services listed as enabled with true must build without options, and services with options must build with them and refuse to build without the documented credential, so a wrong option name like access_key for apilayer_fixer fails the suite.
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
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.
Context
Psalm only analysed
src/and kept a baseline forSwapServiceProvider(mostly$app->config, an undeclared property on the application contract, and mixed values read from the config). The tests were not analysed at all. Nothing prevented the provider docs from drifting from Exchanger again either, which is howconfig/swap.phpended up telling users to passaccess_keytoapilayer_fixer(fixed in #104).Solution
Psalm on tests, no baseline.
psalm.xmlnow coverstests/withpsalm/plugin-phpunitnext topsalm/plugin-laravelat error level 1, andpsalm-baseline.xmlis gone. In the provider, the singleton closure reads options through$app->make('config'), and only non-empty strings count as cache store, HTTP client or request factory ids (before, any truthy value did). Two things are deliberate:config/swap.php(for exampleservicesasfastforex: array|false, european_central_bank: true), which is wrong for user configs, so theoptionsandservicesreads carry an@varwith the types any config can have.mixed.Builder::useHttpClient()anduseRequestFactory()already reject anything else, so those two calls have an@psalm-suppress MixedArgumentwith that reason instead of an extra runtime check.The tests are
finalwithvoidreturn types, use small typed helpers for Testbench's nullable$app, and drop the three earlier HTTP client, request factory and cache tests that only asserted an instance was built; the tests from #104 and #105 check the same settings are actually used. The facade'sUnusedClasssuppression from #107 goes away because the facade test now references the class.Shepherd. The Psalm workflow reports with
--output-format=githuband, on pushes to master, uploads type coverage with--shepherd; the README gets the type coverage badge.Docs sync test.
tests/DocumentationTest.phpfails when an identifier from Exchanger'sRegistryis missing from the services reference inconfig/swap.phpor from the README tables, with an explicit skip map:array(in memory service, not a provider) everywhere andfixer_apilayerin the README (as in Swap). It also parses the reference and builds every entry through the provider: services listed as enabled withtruemust build without options, services with options must build with the documented ones and must fail with "The "" option must be provided" once the first documented option is removed. Changingapilayer_fixerback toaccess_keyin the comment fails two tests. Optional flags (paid,enterprise) cannot be checked this way, because services ignore unknown options.