-
Notifications
You must be signed in to change notification settings - Fork 50
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
PHP 8.3 with Imagick from master #162
PHP 8.3 with Imagick from master #162
Conversation
3b6427c
to
a21faab
Compare
mkdir -p /usr/src/php/ext/imagick; \ | ||
# Locking on specific commit hash to provide consistent results, at the moment of writing this is the HEAD of master | ||
curl -fsSL https://github.com/Imagick/imagick/archive/28f27044e435a2b203e32675e942eb8de620ee58.tar.gz | tar xvz -C "/usr/src/php/ext/imagick" --strip 1; \ | ||
docker-php-ext-install imagick; \ | ||
# End install Imagick from source | ||
\ |
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.
LGTM, I was just thinking about making this part PHP 8.3 only, but since we do not touch stable releases (v3.2) it's fine for now as it is.
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.
If we would go that direction, for code it is probably the cleanest solution to make the download link dependent on the PHP version, so
PHP 8.3: 28f27044e435a2b203e32675e942eb8de620ee58
PHP 8.2: refs/tags/3.7.0
Alternative would be to have a somewhat larger if statement in your Dockerfile
[ "$PHP_VERSION" == "8.2 ] && \
pecl install -f imagick && \
docker-php-ext-enable imagick;
[ "$PHP_VERSION" != "8.2 ] && \
mkdir -p /usr/src/php/ext/imagick; \
# Locking on specific commit hash to provide consistent results, at the moment of writing this is the HEAD of master
curl -fsSL https://github.com/Imagick/imagick/archive/28f27044e435a2b203e32675e942eb8de620ee58.tar.gz | tar xvz -C "/usr/src/php/ext/imagick" --strip 1; \
docker-php-ext-install imagick;
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.
If it would make releasing a new version easier I'm happy to make that change, In that case please let me know which option you'd prefer
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.
@youwe-petervanderwal no, fine for me for now. We'll build only 3.x
images for PHP 8.3 for now, let's do some tests first and then do the v.3.3
release. If we see some issues with the current approach, we can change it anytime back to use the alternative approach (I'd prefer the second option then 😉).
Merging now. Images will be ready soon then.
@youwe-petervanderwal thanks a lot 😊 |
Image will be ready when https://github.com/pimcore/docker/actions/runs/9192158816/job/25280144763 is done. |
Thanks @brusch The development version works now 🚀
ps cross-post to issue #160 |
@youwe-petervanderwal glad to hear - thanks again for the PR. |
With Ubuntu 24.04 out now for a month which is shipped with PHP 8.3 and where Imagick can simply be installed via
apt install php-imagick
, I don't think we should longer lack behind.Imagick/imagick#640 has multiple mentions of installing Imagick from master and since that issue is open for half a year already I don't see any version being released soon (sorry for my pessimistic attitude).
I do see this as a temporary solution, of course the manual download should be reverted at the moment Imagic does release the new official version.
Test results:
pimcore/demo
with both PHP 8.2 and 8.3 (locally build Docker images and connected these in the docker-compose), results:PHP 8.2 test
Using Gaussian Blur on Thumbnail definition:
PHP 8.3 test
Using Gaussian Blur on Thumbnail definition: