-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
replace thecodingmachine/safe
, webmozart/assert
, and symfony/process
by azjezz/psl
#306
Conversation
ab943a3
to
f57f802
Compare
@Ocramius workflow passes in my branch ( https://github.com/azjezz/BackwardCompatibilityCheck/actions ), but needs approval here to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall massive improvements, but I would suggest rolling back all changes in which the core API is:
- safe
- pure
- acting exactly like PSL
There is tremendous value in replacing Safe\
with Psl\
, but mostly risk and overhead when instead we replace pure core constructs with Psl\
.
The reason there is risk is that many of these core functions are correctly inferred by psalm, as well as tooling like infection/infection
, and have near-zero overhead, while Psl\
constructs are not the same there.
Functions that IMO should be rolled back:
array_intersect_*
array_values
array_filter
array_map
strtolower
array_keys
array_combine
implode
- ... perhaps more? ...
As for Shell\execute()
calls, before we move on and phase out symfony/process
, what will happen if a security issue occurs on azjezz/psl
on old branches? Will it be fixed in all past branches?
The difference as mentioned is consistency in error handling ( as in consistency between your code, and the standard library ), parameter order, and naming.
Yes, security fixes will apply to past branches, as well as bug fixes. I will probably add a document later to PSL explaining which versions will receive bug fixes/security patches and for how long, there's already a security policy in place to report vulnerabilities. |
@azjezz I marked all feedback related to restoring internal functions as "resolved", since you've convinced me to fully buy into 3 comments remain, then it's mergeable, IMO. |
BTW, wanted to note that - 372 mutations were generated:
+ 263 mutations were generated:
- 371 mutants were killed
+ 262 mutants were killed
0 mutants were not covered by tests
0 covered mutants were not detected
1 errors were encountered
0 time outs were encountered |
hmm, I can probably create an adapter under @php-standard-library org, i didn't get to play with infection much since infection/infection#1483 is still WIP :( |
@azjezz don't think it's really needed, just wanted to highlight that many mutations are possible because the libraries "know" how to deal with internal symbols, while aren't able to understand custom symbols. This is normal, and it's just part of the considerations. |
done :) |
I'm just an observer here, but this is a super impressive PR so I'm compelled to give props! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks!
thecodingmachine/safe
byazjezz/psl
webmozart/assert
byazjezz/psl
symfony/process
byazjezz/psl
Psl\Vec
, andPsl\Dict
components.