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

Test all extensions with PHP8 #4361

Merged
merged 3 commits into from
Oct 20, 2020

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Oct 19, 2020

Jobs that use other versions of PHP are temporarily removed so that it's
easier to focus and iterate.

Failures:

@greg0ire
Copy link
Member Author

The releases page for sqlsrv shows PHP8 support in version 5.9.0-preview1, let's try that one.

@greg0ire
Copy link
Member Author

Also, let's remove all the successful jobs.

@morozov
Copy link
Member

morozov commented Oct 19, 2020

Message was: "oci_execute(): Argument #​1 ($statement) must be of type resource, null given"

It looks like php_oci8 just enforced its argument types as well as many other extensions.

@greg0ire
Copy link
Member Author

The solution for sqlsrv works. We will just have to tweak the configuration so that it picks the right version of sqlsrv for each version of PHP, or maybe we can use 5.9.0 everywhere, dunno…

@morozov
Copy link
Member

morozov commented Oct 19, 2020

maybe we can use 5.9.0 everywhere

This is fine.

@greg0ire
Copy link
Member Author

greg0ire commented Oct 19, 2020

Link to the faulty call:

$ret = @oci_execute($this->_sth, $this->_conn->getExecuteMode());
if (! $ret) {
throw OCI8Exception::fromErrorInfo($this->errorInfo());
}

Note the error suppression operator lol

@greg0ire
Copy link
Member Author

Message was: "oci_execute(): Argument #​1 ($statement) must be of type resource, null given"

Love how it tells you the name of the argument to avoid confusion because 0 vs 1 numbering 🤩

@morozov
Copy link
Member

morozov commented Oct 19, 2020

Note the error suppression operator lol

It's not a problem per se if the API always returns false in the case of an error and we implement proper handling. Although it is a problem in cases like this (PHP-level error). Not having the error suppressed makes it reported twice (warning and exception) and harder to test (PHPUnit will handle the error before catching the exception).

@greg0ire
Copy link
Member Author

Yeah, it would be great to have an exception instead of an error…

@greg0ire
Copy link
Member Author

supplied resource is not a valid oci8 statement resource

I tried fooling oci_execute, but it looks like the type checks are pretty thorough now…

@morozov
Copy link
Member

morozov commented Oct 19, 2020

I tried fooling oci_execute, but it looks like the type checks are pretty thorough now…

I'd rather have this test refactored. IIRC, that's the one that had to be reworked many times just because of how creepy it is.

@greg0ire
Copy link
Member Author

I just looked at the comment and that part of the test is irrelevant anyway, asserting the type of the exception is not relevant anyway.

@morozov
Copy link
Member

morozov commented Oct 19, 2020

See where it ended up: #3820.

@greg0ire greg0ire force-pushed the test-all-extensions-with-php8 branch from 86d3aa6 to 50585e2 Compare October 19, 2020 20:41
@greg0ire
Copy link
Member Author

Oh right, RIP OCI8StatementTest.php.

I'd rather have this test refactored.

I pushed a commit, but I don't think that's what you mean by "refactored" :P

@morozov
Copy link
Member

morozov commented Oct 19, 2020

I don't think that's what you mean by "refactored" :P

No, just skip it on PHP 8.

@greg0ire greg0ire force-pushed the test-all-extensions-with-php8 branch from 50585e2 to 90c35d7 Compare October 19, 2020 20:53
It has support for PHP8.
That test will get remove in next major release, and requires too much
maintenance.
@greg0ire greg0ire force-pushed the test-all-extensions-with-php8 branch from 90c35d7 to 921f04b Compare October 19, 2020 21:37
@greg0ire greg0ire marked this pull request as ready for review October 19, 2020 21:52
@greg0ire
Copy link
Member Author

The OCI8 job seems stuck :/

@greg0ire greg0ire closed this Oct 19, 2020
@greg0ire greg0ire reopened this Oct 19, 2020
@greg0ire
Copy link
Member Author

Ok so now we have 78 checks 😅

@morozov
Copy link
Member

morozov commented Oct 19, 2020

Ok so now we have 78 checks 😅

This is overkill. We just need to test each driver with the latest platform version on PHP 8 and other non-primary PHP versions. There's no much point in testing all platform versions with any PHP version than the primary (currently 7.4).

@morozov
Copy link
Member

morozov commented Oct 20, 2020

Just in case, I'm not concerned about the number of jobs as such (as long as GA is fine with it). But I am concerned about the build stability: there's a higher chance that one of the 78 jobs will decide to fail for no reason, and right now (unlike Travis and AppVeyor), GA only allows to restart all jobs (not just the failed ones.)

@morozov morozov added this to the 2.12.0 milestone Oct 20, 2020
@morozov morozov added the CI label Oct 20, 2020
@morozov morozov added the PHP label Oct 20, 2020
Jobs that use other versions of PHP are temporarily removed so that it's
easier to focus and iterate.
@greg0ire greg0ire force-pushed the test-all-extensions-with-php8 branch from 921f04b to 94c02cc Compare October 20, 2020 05:57
@greg0ire
Copy link
Member Author

Oh I forgot that there were already tests defined under include for some platforms. Fixed.

@greg0ire
Copy link
Member Author

Down to 59, we can apply your ideas on 2.11.x to reduce that number even further.

@greg0ire greg0ire requested a review from morozov October 20, 2020 06:24
@greg0ire greg0ire merged commit b85809c into doctrine:2.12.x Oct 20, 2020
@greg0ire greg0ire deleted the test-all-extensions-with-php8 branch October 20, 2020 14:40
@morozov
Copy link
Member

morozov commented Oct 20, 2020

By the way, @BenMorel put together a script some time ago that I believe still can be used as a reference for the algorithm to generate the matrix.

rgrellmann added a commit to Rossmann-IT/dbal that referenced this pull request Mar 7, 2021
Release [2.12.0](https://github.com/doctrine/dbal/milestone/82)

2.12.0
======

- Total issues resolved: **1**
- Total pull requests resolved: **7**
- Total contributors: **5**

Documentation,Static Analysis
-----------------------------

 - [4376: Configuration should not be internal](doctrine#4376) thanks to @BenMorel

CI
--

 - [4374: Reduce number of build jobs](doctrine#4374) thanks to @greg0ire
 - [4365: Fail on extension / tool installation failure](doctrine#4365) thanks to @greg0ire

Bug,Static Analysis
-------------------

 - [4373: Psalm fails on release commits](doctrine#4373) thanks to @morozov

Documentation,Error Handling
----------------------------

 - [4362: Adds exception thrown by execute() method](doctrine#4362) thanks to @toby-griffiths

CI,PHP
------

 - [4361: Test all extensions with PHP8](doctrine#4361) thanks to @greg0ire

PHP
---

 - [4347: &doctrine#91;2.12&doctrine#93; PHP 8 compatibility](doctrine#4347) thanks to @derrabus

# gpg: Signature made Thu Oct 22 20:29:24 2020
# gpg:                using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132
# gpg: Can't check signature: No public key
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants