Eliminated Psalm baseline, migrated from webmozart/assert to azjezz/psl
#564
Conversation
This file contains 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
…z/psl` This change is quite massive, and rewrites most of the library so that it is more type-safe, especially when it comes to large JSON structure traversal. `composer/composer:^1` manifest parsing is now gone (not supported anyway, since we depend on `composer-runtime-api:^2`). We migrated from `webmozart/assert` to the incredibly more powerful `azjezz/psl` dependency, which gives us massive code reduction, and improved type guarantees. Most tests were also simplified a lot, and some (outdated, due to older PHP versions targeted) are gone. Test metadata has also been rewritten, and PHPUnit has been tuned to be much stricter. `symfony/console:^7.2.1` has also been bumped, meanwhile.
Ocramius
added
enhancement
dependencies
This leads to some minor tradeoffs between `non-empty-string` vs `string` declarations, and required us to remove an `@pure` declaration, but is otherwise a good improvement overall.
This type is relatively safe to keep inside this package (only), and its only responsibility is really behaving as anti-corruption layer with `composer.json`, `composer.lock` and `installed.json`, so this is a good fit.
These are minor dependency upgrades that are well worth having, for the reduced code complexity in our domain.
Ocramius
force-pushed
the
feature/stricter-types
branch
from
bfa36d6 to
2f87f49
Compare
These are slow and wasteful: not worth burning CPUs for no reason.
Many libraries, such as Symfony, don't understand semantic versioning well, and tend to release software that is incompatible with the next major version of a dependency (in this case `php`, which does not follow SemVer). This kind of botched architecture by Symfony leads to dependencies having broken behavior in oldstable releases. This is effectively a choice by the upstream maintainer (Symfony) to leave this sort of problem to the consumer (in this case, us), who then has to restrict ranges more aggressively. In this case, the failures are due to transitive dependencies leading to symfony, so introducing a `conflict` worsens the situation for our own consumers/
|
@maglnet since I'm touching a lot of the project, you might want to take a glance here, especially to see how I'm ripping out pieces to use |
fredden
reviewed
Feb 7, 2025
| $fileName = realpath($configFileArgument); | ||
| if ($fileName === false) { | ||
| $fileName = Filesystem\canonicalize($configFileArgument); | ||
| if ($fileName === null) { | ||
| throw new InvalidArgumentException( |
There was a problem hiding this comment.
Does this need to be listed in the @throws docblock?
There was a problem hiding this comment.
Possibly: I added @throws in an age when the IDE was really pedantic about it.
I don't believe them to be as important anymore, but if you believe so, we can tweak SA tools to check these
There was a problem hiding this comment.
I think we should either use the tag or not. I'm happy with the tags going away / being removed.
There was a problem hiding this comment.
I'd say that they are relevant for things where we explicitly also test for them
Comment on lines
62
to
83
| $fileContent = file_get_contents(__DIR__ . '/../../../data/config.dist.json'); | ||
| $this->assertNotFalse($fileContent); | ||
|
|
||
| $optionsFromFile = new Options( | ||
| json_decode($fileContent, true), | ||
| ); | ||
| $optionsFromFile = new Options([ | ||
| 'symbol-whitelist' => [], | ||
| 'php-core-extensions' => [ | ||
| 'Core', | ||
| 'date', | ||
| 'json', | ||
| 'hash', | ||
| 'pcre', | ||
| 'Phar', | ||
| 'Reflection', | ||
| 'SPL', | ||
| 'random', | ||
| 'standard', | ||
| ], | ||
| 'scan-files' => [], | ||
| ]); | ||
|
|
||
| $this->assertEquals($options, $optionsFromFile); | ||
| } |
There was a problem hiding this comment.
$fileContent is no longer used in this test. We are now comparing new Options() with new Options( $some_fixed_content ). The file isn't used at all here any longer. Perhaps this test should be renamed or rewritten?
There was a problem hiding this comment.
Yep, renaming is a good idea
...rRequireCheckerTest/DefinedSymbolsLocator/LocateDefinedSymbolsFromComposerRuntimeApiTest.php
Show resolved
Hide resolved
test/ComposerRequireCheckerTest/FileLocator/LocateAllFilesByExtensionTest.php
Show resolved
Hide resolved
test/ComposerRequireCheckerTest/FileLocator/LocateComposerPackageSourceFilesTest.php
Show resolved
Hide resolved
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.
This change is quite massive, and rewrites most of the library so that it is more type-safe, especially when it comes to large JSON structure traversal.
composer/composer:^1manifest parsing is now gone (not supported anyway, since we depend oncomposer-runtime-api:^2).We migrated from
webmozart/assertto the incredibly more powerfulazjezz/psldependency, which gives us massive code reduction, and improved type guarantees.Most tests were also simplified a lot, and some (outdated, due to older PHP versions targeted) are gone.
Test metadata has also been rewritten, and PHPUnit has been tuned to be much stricter.
symfony/console:^7.2.1has also been bumped, meanwhile.