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

Audit dependencies #31

Closed
asgrim opened this issue Sep 7, 2024 · 4 comments · Fixed by #42
Closed

Audit dependencies #31

asgrim opened this issue Sep 7, 2024 · 4 comments · Fixed by #42
Assignees
Labels
enhancement New feature or request

Comments

@asgrim
Copy link
Collaborator

asgrim commented Sep 7, 2024

We are currently using some dependencies which MAY be problematic for maintenance.

Whilst not a big issue, if we want to support newer versions of PHP (e.g. 8.4 at the time of writing, is unreleased), we have to ensure we are compatible with the newer dependencies. For example, the constraint we have for illuminate/container is ^10.47, which would not allow version 11 (which I believe has fixed the PHP 8.4 issues). This is somewhat mitigated by our root php constraint being explicitly 8.1.*||8.2.*||8.3.* (i.e., we do not yet support PHP 8.4), and this issue only surfaces when --ignore-platform-req=php is used (e.g. for testing). The more third-party dependencies we have, the more we are constrained by upstream support. We should evaluate the dependencies we are using, and ensure it makes sense to continue using them or not.

PSL - https://packagist.org/packages/azjezz/psl - was mentioned as problematic, since it relies on some extensions which not everyone always has installed (e..g bcmath, sodium, etc.). This creates a chicken/egg problem if the extension someone wants to install is bcmath! Whilst I love the functionality we're using in azjezz/psl we may need to find another way; we're not using the azjezz/psl parts that use those extensions (we're ONLY using the nicely-typed JSON parsing, and only in a couple of places), and I don't think it's reasonable at all to ask the author that these parts are split off into a separate package.

ref: mongodb/mongo-php-driver#1624 (comment)

@asgrim asgrim added the enhancement New feature or request label Sep 7, 2024
@kynx
Copy link

kynx commented Sep 7, 2024

@asgrim I’ve only just started following this repo and applaud your efforts. And yes, the fewer dependencies the better for a project that is going to bootstrap installs. I’d go further and say symfony/console is overkill for the simple CLI proposed - getopts ain’t much fun, but it works and is available on every install.

That said, the most important thing upfront is to get a functioning extension manager out the door and extension authors buying in. Don’t let dependency worries slow this down!

OT, but the big thing for me is to have tooling like renovate recognise when extensions need updating.

@asgrim
Copy link
Collaborator Author

asgrim commented Sep 8, 2024

to have tooling like renovate recognise when extensions need updating.

being able to identify which extensions are installed is definitely in the works (pie show), but I've added your suggestion to our design doc issue tracker - see ThePHPF/pie-design#24 - thank you @kynx !

@asgrim
Copy link
Collaborator Author

asgrim commented Sep 16, 2024

Discovered - if the target PHP install does not have ext-json, then \Php\Pie\Platform\TargetPhp\PhpBinaryPath::extensions causes a crash (since it does json_decode). This was just an ease of use, and can be refactored fairly easily to not depend on ext-json.

Maybe something like:

-echo json_encode(array_combine($exts, $extVersions));
+echo '{' . implode(',', array_map(fn($k, $v) => sprintf('"%s":"%s"', $k, $v), $exts, $extVersions)) . '}';

although prob needs escaping >.< will have a think about other potential ways. I'm trying to avoid using serialize/unserialize but that might actually be a good enough tool for the job.

@asgrim
Copy link
Collaborator Author

asgrim commented Sep 17, 2024

Dependencies assessment:

Dependency Remark Remove?
ext-zip Used to unzip downloaded archives. Non-trivial to replace. No
azjezz/psl QOL for development; limited use and can be replaced with individual assertions. Yes
composer/composer Required. No
guzzlehttp/guzzle Used for downloading and API requests. Potentially could be replaced, but Guzzle is very common so probably not worth it. Not at this time
guzzlehttp/psr7 See Guzzle. Not at this time
illuminate/container QOL for development; potentially could be replaced, but limited ROI, plus Laravel fairly quick at updating for PHP support. Not at this time
psr/http-message See Guzzle. Not at this time
symfony/console Required. No
symfony/process QOL for executing subprocesses; potentially could be replaced but this solves many problems! Not at this time
webmozart/assert QOL for assertions, but fairly low risk Not at this time

@asgrim asgrim self-assigned this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants