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

[2.x] Prefix FrankenPHP version with v for version_compare() #776

Closed
wants to merge 1 commit into from

Conversation

cwhite92
Copy link

@cwhite92 cwhite92 commented Dec 20, 2023

I was experimenting with creating a Docker image to run a FrankenPHP web server with Octane. I made the following Dockerfile:

FROM dunglas/frankenphp

RUN install-php-extensions \
    pcntl

COPY . /app

ENTRYPOINT ["php", "artisan", "octane:frankenphp"]

When I started the container it immediately exited, and when inspecting the logs I found that Octane was attempting to download the FrankenPHP binary despite it already existing in the Docker image at /usr/local/bin/frankenphp:

   WARN  Your FrankenPHP binary version (v1.0.1) may be incompatible with Octane.

 Should Octane download the latest FrankenPHP binary version for your operating system? (yes/no) [yes]:
 > 
   ERROR  FrankenPHP binaries are currently only available for Linux (x86_64) and macOS. Other systems should use the Docker images or compile FrankenPHP manually.

   INFO  Server running…

  Local: http://127.0.0.1:8000 

  Press Ctrl+C to stop the server


   INFO  sh: 1: exec: /usr/local/bin/frankenphp: not found

After some debugging I found that the version_compare() on this line might not be working the way we think. The frankenphp binary returns v1.0.1 as its version and it's being compared to 1.0.0 without the v. As you can see here v1.0.1 >= 1.0.0 returns false, and causes Octane to re-download the binary: https://3v4l.org/uWjgX

After changing $requiredFrankenPhpVersion to v1.0.1 in my local files and starting the container again, Octane doesn't try to re-download the frankenphp binary and the container runs successfully.

@driesvints
Copy link
Member

We don't want to make the adjustment in the property here. We always define the version in source code for our packages without it: https://github.com/laravel/cashier-paddle/blob/c3b70784918ebecb12b28cda1b04aabe39e0ca8b/src/Cashier.php#L16

The correct fix is to adjust this in the place where it's used.

@driesvints driesvints changed the title Prefix FrankenPHP version with v for version_compare() [2.x] Prefix FrankenPHP version with v for version_compare() Dec 20, 2023
@taylorotwell taylorotwell marked this pull request as draft December 20, 2023 14:32
@nunomaduro
Copy link
Member

Currently re-working on this. Thank you for this report.

@nunomaduro nunomaduro closed this Dec 20, 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.

None yet

3 participants