Skip to content
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

Merged
merged 12 commits into from May 19, 2021

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented May 19, 2021

  • replace thecodingmachine/safe by azjezz/psl
  • replace webmozart/assert by azjezz/psl
  • replace symfony/process by azjezz/psl
  • upgraded psalm from v3 to v4
  • upgraded phpunit to 9.5 ( + migrated configurations accordingly )
  • replaced usage of PHP builtin array functions by Psl\Vec, and Psl\Dict components.

@azjezz azjezz force-pushed the migration/psl branch 2 times, most recently from ab943a3 to f57f802 Compare May 19, 2021 03:19
@azjezz
Copy link
Contributor Author

azjezz commented May 19, 2021

@Ocramius workflow passes in my branch ( https://github.com/azjezz/BackwardCompatibilityCheck/actions ), but needs approval here to run.

psalm.xml Show resolved Hide resolved
Copy link
Member

@Ocramius Ocramius left a 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?

phpunit.xml.dist Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
bin/roave-backward-compatibility-check.php Outdated Show resolved Hide resolved
src/CompareClasses.php Show resolved Hide resolved
src/Support/ArrayHelpers.php Show resolved Hide resolved
@Ocramius Ocramius added this to the 5.1.0 milestone May 19, 2021
@azjezz
Copy link
Contributor Author

azjezz commented May 19, 2021

acting exactly like PSL
mostly risk and overhead when instead we replace pure core constructs with Psl.

The difference as mentioned is consistency in error handling ( as in consistency between your code, and the standard library ), parameter order, and naming.

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?

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.

@Ocramius
Copy link
Member

@azjezz I marked all feedback related to restoring internal functions as "resolved", since you've convinced me to fully buy into Psl\ there :-)

3 comments remain, then it's mergeable, IMO.

@Ocramius
Copy link
Member

BTW, wanted to note that Psl\ is not really understood by infection/infection, and the number of mutants changed quite a lot (both a positive and a negative):

- 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

@azjezz
Copy link
Contributor Author

azjezz commented May 19, 2021

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 :(

@Ocramius
Copy link
Member

@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.

@azjezz
Copy link
Contributor Author

azjezz commented May 19, 2021

done :)

@azjezz azjezz requested a review from Ocramius May 19, 2021 12:21
@MGatner
Copy link

MGatner commented May 19, 2021

I'm just an observer here, but this is a super impressive PR so I'm compelled to give props!

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants