Enabling psalm on php >= 7.1, fixing #125, #168, #169, partially fixing #177 #190
Conversation
|
Hi @SignpostMarv great PR, thanks. I'll take a deeper look into Psalm. However, I'm relying on Scrutinizer for static analysis and would like to know if it is worth to have both. |
|
I've been using a combination of phpstan, psalm, and scrutinizer for quite a while now- my experience is that scrutinizer doesn't catch everything and that running static analysis on your local machine leads to a more rapid development cycle as you're not waiting for CI to spin up multiple instances & finish all the matrices. This leaves scrutinizer's main purpose down to tracking overall code quality & code coverage. sebastian/phpcpd can be useful in identifying deduplication issues before they reach scrutinizer. povils/phpmnd can help identify integers that should probably be consts. maglnet/composer-require-checker checks if your code uses any php extensions or downstream code not explicitly required by the project. edited to update |
|
When do you plan to merge this? |
SignpostMarv
force-pushed
the
tinkering
branch
3 times, most recently
from
cf43861 to
601ca97
Compare
SignpostMarv
changed the title
There was a problem hiding this comment.
Thanks for such a work @SignpostMarv and thanks for your patience.
I understand the value that psalm brings, however I have to make up my mind on adding so many comments for the sake of a tool.
Meanwhile I have left some comments and will make Codacy go green.
| public function __construct() | ||
| { | ||
| if (!extension_loaded('intl')) { | ||
| if (!function_exists('idn_to_ascii')) { | ||
| throw new \LogicException(sprintf('The %s class requires the Intl extension.', __CLASS__)); |
There was a problem hiding this comment.
How does idn_to_ascii correlates to intl extension? Are they the same?
If so, please also update the error message.
There was a problem hiding this comment.
symfony/polyfill-intl-idn will take over when the extension isn't loaded, thus the condition needs to check for the function used.
composer.json
Outdated
| "repositories": [ | ||
| { | ||
| "type": "git", | ||
| "url": "https://github.com/dominicsayers/isemail" |
There was a problem hiding this comment.
Why are you removing this? it is needed for the tests comparision :)
There was a problem hiding this comment.
as mentioned in the commit message, the package is listed on packagist & directly listing the repo causes install issues on windows & git bash.
|
This PR does a lot of different things, which makes it hard to merge. I think it should be broken up into more atomic pieces. Not all of the changes are still relevant now too. |
SignpostMarv
force-pushed
the
tinkering
branch
from
601ca97 to
ada20f0
Compare
|
checking a rebase builds before splitting things off. |
|
@SignpostMarv apparently Psalm version you are requiring does not supports php 7.0 (https://travis-ci.org/egulias/EmailValidator/jobs/631128083?utm_medium=notification&utm_source=github_status) |
…fChecker does not seem to be polyfilled in symfony/polyfill-intl-*
…tEmailWithSpoofsIsInvalid fails if extension is not installed
SignpostMarv
force-pushed
the
tinkering
branch
from
ada20f0 to
520defd
Compare
no version constraint is applied, so this may occasionally fail (necessitating an adjustment to the matrix as in 079ac0e) |
SignpostMarv
changed the title
composer.json
Outdated
| }, | ||
| "require-dev": { | ||
| "ext-intl": "*", |
There was a problem hiding this comment.
Relates to #231 , since intl is still required for the spoof checker.
Maybe you can revert this change to avoid blocking this PR because of the INTL dependency fix.
There was a problem hiding this comment.
re: SpoofChecker, what're your thoughts regarding forking SpoofCheckValidation to it's own package that requires ext-intl ?
There was a problem hiding this comment.
Answered. However, we can still remove this changes from the current PR in order to merge it and solve the INTL issue there.
There was a problem hiding this comment.
removed in affd3dd
@psalm-suppressor two in there rather than a baseline file because the older versions of psalm required forphp < 7consider the baseline attribute invalid so that'd be best put off until you make the decision to drop support for php 5 in a given release.SpoofCheckerclass isn't polyfilled. The options there would be to find a polyfill just for SpoofChecker, or to fork out theSpoofCheckValidationclass into a package that explicitly requiresext-intl:*