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

Detection for vips 1 not longer works #46

Merged
merged 12 commits into from
Dec 11, 2024

Conversation

alexander-schranz
Copy link
Contributor

The implementation previously was f159d24

Not sure when but if (\method_exists(Config::class, 'ffi')) { was replaced by class_exists(FFI::class) which seems now not longer fallback then to check for v1 version as FFI class always exist im basic PHP core installations.

@chregu
Copy link
Contributor

chregu commented Nov 22, 2024

Thanks. Looks like we need to check the tests also.. Something is not working on them

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Nov 22, 2024

@chregu looks like ffi.enable is not activate on the CI which is required to vips to work. Default is 'preload' but vips requires 'true'

Comment on lines +33 to +42
'no_superfluous_phpdoc_tags' => false,
'fully_qualified_strict_types' => false,
'native_function_invocation' => false,
'no_null_property_initialization' => false,
'phpdoc_trim' => false,
'blank_line_before_statement' => false,
'phpdoc_add_missing_param_annotation' => false,
'modernize_strpos' => false,
'trailing_comma_in_multiline' => false,
'no_whitespace_in_blank_line' => false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to reduce too much changes inside this PR but still allow to test against PHP 8.4

Copy link
Contributor

Choose a reason for hiding this comment

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

Linter changes are totally fine for me. That doesn't change functionality

@alexander-schranz
Copy link
Contributor Author

I hope I got it running now all let me know If I should change anything else.

@chregu
Copy link
Contributor

chregu commented Nov 22, 2024

BTW, it's also fine to get rid of support for non supported PHP versions. If that makes life easier
https://www.php.net/supported-versions.php
So that would mean, everything below 8.1 we could get rid of

Comment on lines +74 to +77
- name: Debug PHP settings
run: |
php -r 'echo "Has VIPS Extension: " . (extension_loaded("vips") ? "true" : "false") . PHP_EOL; echo "Has FFI Extension: " . (extension_loaded("ffi") ? "true" : "false") . PHP_EOL; echo "Has FFI Class: " . (class_exists(FFI::class) ? "true" : "false") . PHP_EOL; echo "Has FFI Enabled: " . (ini_get("ffi.enable") === "1" ? "true" : "false") . PHP_EOL; echo "Has zend.max_allowed_stack_size correct: " . (ini_get("zend.max_allowed_stack_size") === "-1" ? "true" : "false") . PHP_EOL;'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will keep this may helpful in future?

@alexander-schranz
Copy link
Contributor Author

@chregu currently see no need yet to drop them. Seems to still work fine on older versions :)

@chregu
Copy link
Contributor

chregu commented Nov 22, 2024

@chregu currently see no need yet to drop them. Seems to still work fine on older versions :)

Even better ;)

@chregu
Copy link
Contributor

chregu commented Nov 22, 2024

Since all tests go through now, I think we can easily merge this, right?

@alexander-schranz
Copy link
Contributor Author

@chregu all fine from my side to merge it.

@chregu chregu merged commit 59ecbca into rokka-io:master Dec 11, 2024
24 checks passed
@chregu
Copy link
Contributor

chregu commented Dec 11, 2024

Thanks ;) and sorry for the delay

@alexander-schranz alexander-schranz deleted the patch-1 branch December 11, 2024 11:08
@alexander-schranz
Copy link
Contributor Author

@chregu no problem is not urgent :)

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.

2 participants