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

Rewrite lib/pecl #341

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Rewrite lib/pecl #341

wants to merge 14 commits into from

Conversation

Frzk
Copy link
Contributor

@Frzk Frzk commented Jul 11, 2023

This is a complete rewrite of lib/pecl which originally contains code to install any PECL extension.

While trying to reproduce #236, I dove into this code and thought it would greatly benefit from a rewrite. So, this PR:

  • Splits the code into multiple, small and understandable functions, making it easier to debug/maintain/extend,
  • Introduces some kind of namespace to better identify the functions and where they come from,
  • Ditches the set +e that was put at some point,
  • Should fail early when compiling an extension fails, unlike currently,
  • Should fail early when trying to install a non-existing extension, unlike currently,
  • Has better error and success messages,
  • Includes built-in support for some extensions (oci8, yaml and scoutapm). This part is maybe the most arguable (see the php::pecl::enable_pecl_extension and php::pecl::configure_extension functions).

Fixes #236

@Frzk Frzk self-assigned this Jul 11, 2023
@Frzk Frzk force-pushed the chore/php-buildpack/pecl_extensions branch from af89ee9 to 4527a82 Compare July 11, 2023 09:01
@Frzk Frzk marked this pull request as ready for review July 11, 2023 09:18
@Frzk Frzk requested a review from Soulou July 11, 2023 09:18
Copy link
Member

@Soulou Soulou left a comment

Choose a reason for hiding this comment

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

The code looks great, it's tricky to validate if it works correctly because of the lack of tests.

What do you think about using this PR to introduce some testing in the buildpack to prevent regressions?

Or maybe merge it and then prepare another PR with the ability to test the behavior?

What do you think would be best?

@Frzk
Copy link
Contributor Author

Frzk commented Jul 17, 2023

I'd be keen to:

  1. write the tests,
  2. make them pass with the current code,
  3. make them pass with the new code (from the PR).

WDYT?

@Soulou
Copy link
Member

Soulou commented Jul 21, 2023

@Frzk Look good to me, but do you prefer this PR being merged or wait later?

@ipfaze ipfaze mentioned this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment of an unknown extension shouldn't have a match from the cache
2 participants