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

IBX-1489: Provided integration test for Installers for PHP8 #264

Merged
merged 21 commits into from
Dec 6, 2021
Merged

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented Nov 23, 2021

Question Answer
JIRA issue IBX-1489
Type improvement
Target Ibexa version v3.3
BC breaks no

Provides integration tests & fixes for PHP8 compatibility.

Notable changes:

Disabling transactions during schema import / install.

Due to a change in PHP's MySQL driver php/php-src@990bb34 for PHP8 it becomes necessary to handle a case where Connection::commit() throws an exception that there is no transaction (which happens after DDL SQL commands).

DDL statements on mysql result in an implicit commit (see mysql documentation). This causes the explicit commit in transactional mode to fail. But only starting with php 8.0 an error is raised (probably due to php/php-src@990bb34).

Removal of PHP8 deprecation warnings

This includes:

  • Reordering of some test methods, so that optional argument are at the end of the list.
  • Converting values to string, where other data types are possible. Especially null or int.

Issues that will require addressing in the near future

  • One test fails specifically on PHP 8.1 due to change in DateTime interval behavior. It is therefore disabled on this platform.
  • MySQL 8.0 tests have issues with collation conversion (which are probably silently ignored on earlier versions).

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@Steveb-p Steveb-p marked this pull request as ready for review November 29, 2021 13:13
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@konradoboza konradoboza self-assigned this Nov 29, 2021
@Steveb-p Steveb-p mentioned this pull request Nov 29, 2021
6 tasks
@Steveb-p Steveb-p requested a review from a team November 29, 2021 15:12
@konradoboza konradoboza requested review from konradoboza and removed request for konradoboza November 29, 2021 15:15
@konradoboza
Copy link
Member

The failing test case seems to be random, given it passes on all other PHP versions. Perhaps tests order comes into play.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

It's a bit unclear to me why some changes were made and how they're related to what the PR is supposed to be fixing. Unfortunately the PR description is rather empty.

needs: tests
services:
mysql:
image: mysql:5.7
Copy link
Contributor Author

@Steveb-p Steveb-p Nov 30, 2021

Choose a reason for hiding this comment

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

Note: A follow up draft PR with mysql:8.0 is necessary to show that there are tests that will fail on MySQL 8.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

+1 but Steve has some valid remarks: #264 (review) ;)

@konradoboza konradoboza requested a review from adamwojs November 30, 2021 13:55
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Steveb-p Steveb-p requested a review from a team November 30, 2021 14:01
@konradoboza konradoboza removed their assignment Dec 1, 2021
@micszo micszo self-assigned this Dec 1, 2021
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Tested with sanities on Ibexa Commerce 3.3.12-dev with PHP8.1.

QA Approved.

@adamwojs adamwojs merged commit 68e8ee8 into 1.3 Dec 6, 2021
@adamwojs adamwojs deleted the fix-php8 branch December 6, 2021 14:46
@micszo micszo removed their assignment Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Development

Successfully merging this pull request may close these issues.

5 participants