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.

Sign up for GitHub

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

Jump to bottom

Allow PHP8.3 #74

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Allow PHP8.3 #74

merged 5 commits into from
Nov 27, 2023

Conversation

heiglandreas
Copy link
Contributor

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

This allows usage of the package using PHP8.3

@Ocramius
Copy link
Member

Lock file needs a hash refresh

@heiglandreas
Copy link
Contributor Author

That's the least of all issues...

Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package php8.3-apcu
E: Couldn't find any package by glob 'php8.3-apcu'
E: Unable to locate package php8.3-memcached
E: Couldn't find any package by glob 'php8.3-memcached'
E: Unable to locate package php8.3-mongodb
E: Couldn't find any package by glob 'php8.3-mongodb'
E: Unable to locate package php8.3-redis
E: Couldn't find any package by glob 'php8.3-redis'

https://github.com/laminas/laminas-diagnostics/actions/runs/6954575541/job/18921670184?pr=74

@Ocramius
Copy link
Member

Ah yes, I think we solved that recently for laminas-session?

@froschdesign
Copy link
Member

See: https://github.com/laminas/laminas-session/blob/e733d986b30704831c6ec530d06688faf6f0856a/.laminas-ci/install-mongodb-extension-via-pecl.sh

@heiglandreas
Copy link
Contributor Author

Stupid question perhaps: Why are we going through all the hassle of setting up Memcached and Redis and Mongo et all to then just ignore those tests?

There were 10 skipped tests:

1) LaminasTest\Diagnostics\ChecksTest::testRabbitMQ
RabbitMQ tests are not enabled; enable them in phpunit.xml

/github/workspace/test/ChecksTest.php:100

2) LaminasTest\Diagnostics\ChecksTest::testRedis
Redis tests are not enabled; enable them in phpunit.xml

/github/workspace/test/ChecksTest.php:117

3) LaminasTest\Diagnostics\ChecksTest::testMemcache
Memcache tests are not enabled; enable them in phpunit.xml

/github/workspace/test/ChecksTest.php:135

4) LaminasTest\Diagnostics\ChecksTest::testMemcached
Memcached tests are not enabled; enable them in phpunit.xml

/github/workspace/test/ChecksTest.php:152

5) LaminasTest\Diagnostics\ChecksTest::testMongo
Mongo tests are not enabled; enable them in phpunit.xml

@heiglandreas
Allow PHP8.3
6dbc15b 
Signed-off-by: Andreas Heigl <[email protected]>
@heiglandreas
Allo platform requirements to be ignored on 8.3
6f7ae94 
At least for now...

Signed-off-by: Andreas Heigl <[email protected]>
@heiglandreas
Add installation instructions for pecl-extensions
2b035ab 
Otherwise those extensions would not be installed correctly on PHP8.3

Signed-off-by: Andreas Heigl <[email protected]>
@Xerkus
Copy link
Member

Xerkus commented Nov 22, 2023

That got left behind when moving to current common CI setup I would guess and noone paid enough attention to notice.

Ocramius
Ocramius reviewed Nov 22, 2023
View reviewed changes
src/Check/IniFile.php Outdated
@@ -22,7 +22,7 @@ class IniFile extends AbstractFileCheck
*/
protected function validateFile($file)
{
if (! is_array($ini = parse_ini_file($file)) || count($ini) < 1) {
if (! is_array($ini = @parse_ini_file($file)) || count($ini) < 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we use something like set_error_handler() + restore_error_handler() here, perhaps? 🤔

Using @ always bites us back later, so just wondering

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 was indeed thinking about that and decided to skip it here for sake of brevity. But using the error handler might actually allow us to add the reported message to the error-message.

So will add

@heiglandreas
Copy link
Contributor Author

OK. Running the actual tests with the external tools fails. But it at least fails in all tested PHP-versions, so it's not related to PHP8.3

I created an issue to reintroduce these tests at #75

@heiglandreas
Handle PHP8.3 parse-error of ini-files
8f0d43c 
PHP8.3 now emits a syntax error warning in addition to returning
`false` now.
Previously the error was silently ignored and the returned value was an
empty array.

The `@` silcences the warning. As this is done during the check
whether the ini-file can be parsed or not it should be safe here to not
output the content of the warning.

Signed-off-by: Andreas Heigl <[email protected]>
@Chris53897 Chris53897 mentioned this pull request Nov 24, 2023
Merged
3 tasks
@kbond kbond mentioned this pull request Nov 24, 2023
Merged
2 tasks
@heiglandreas
Copy link
Contributor Author

So shall I hit the "Merge" Button? Or is there anything missing?

@heiglandreas heiglandreas added this to the 1.25.0 milestone Nov 27, 2023
@heiglandreas
Update .laminas-ci.json
037ea19 
Remove 'ignore-platform-reqs'

Signed-off-by: Andreas Heigl <[email protected]>
@heiglandreas heiglandreas merged commit a73a087 into laminas:1.25.x Nov 27, 2023
13 checks passed
@Chris53897
Copy link

Thanks. Is there a release date planned?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ocramius Ocramius Ocramius left review comments

Assignees
No one assigned
Labels
None yet
Projects
No open projects
PHP 8.3
Status: Done
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.
5 participants
@heiglandreas @Ocramius @froschdesign @Xerkus @Chris53897