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

doctrine/dbal ^4.0.0 #84

Merged
merged 14 commits into from
Mar 8, 2024
Merged

doctrine/dbal ^4.0.0 #84

merged 14 commits into from
Mar 8, 2024

Conversation

oleg-andreyev
Copy link
Contributor

@oleg-andreyev oleg-andreyev commented Mar 7, 2024

This should be a major version because:

  • dbal is using union type which are from PHP8.0

  • dbal require php8.1

  • fixed also psalm in ci (it was not running)

  • increased mutation score

@oleg-andreyev
Copy link
Contributor Author

@Jean85 could you please take a look?

src/ConnectionTrait.php Outdated Show resolved Hide resolved
Copy link
Member

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, I'll try to work with you to make viable and release it!

I agree that this will require a new major, I'll later change it to target a new major branch; in the meantime, I'll backport whatever makes sense (like the Psalm fixes) from this PR to the existing major.

I've left some comments; also, for Infection, to fix it you have to add a test that would fail when editing the code in the mutated cases that it highlights.

Comment on lines 37 to 45

- php-version: "8.3"
mysql-version: "5.7"
custom-entrypoint: ~
job: Tests
- php-version: "8.2"

- php-version: "8.3"
mysql-version: "5.7"
job: Tests
Copy link
Member

Choose a reason for hiding this comment

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

Here you created a duplicate. You should test MySQL 8 against PHP 8.1

@@ -109,7 +111,7 @@ jobs:
command: vendor/bin/php-cs-fixer fix --dry-run --ansi --verbose
- name: Psalm
commandName: Run Psalm analysis
command: vendor/bin/php-cs-fixer fix --dry-run --ansi --verbose
command: vendor/bin/psalm --show-info=true
Copy link
Member

Choose a reason for hiding this comment

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

Wow nice catch!

composer.json Outdated
"php": "^7.4 || ^8.0",
"doctrine/dbal": "^3.6.0"
"php": "^8.1",
"doctrine/common": "^3.4",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need doctrine/common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was required because some class was missing from common namespace

Comment on lines 20 to 27
# zsh / OhMyZsh
RUN apk --no-cache add git zsh

ARG COMPOSER_VERSION=2.5.5
RUN curl -sS https://getcomposer.org/installer | php -- \
--install-dir=/usr/local/bin --filename=composer --version=$COMPOSER_VERSION

USER facile
RUN sh -c "$(curl -fsSLv https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.sh)"
Copy link
Member

Choose a reason for hiding this comment

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

Please do not remove this custom shell.

docker/Dockerfile Show resolved Hide resolved
@Jean85 Jean85 changed the base branch from master to 3.x March 8, 2024 15:56
@Jean85
Copy link
Member

Jean85 commented Mar 8, 2024

I've backported some of your fixes with #84, and retargeted this PR to the new 3.x branch, which includes that; in fact, now you have conflicts to fix.

@Jean85 Jean85 mentioned this pull request Mar 8, 2024
@Jean85 Jean85 added this to the 3.0 milestone Mar 8, 2024
@Jean85 Jean85 merged commit dc5846a into facile-it:3.x Mar 8, 2024
8 checks passed
@oleg-andreyev
Copy link
Contributor Author

@Jean85 Thanks for help and so fast response! This will allow use to use ORM4 and Symfony 7 ❤️

@Jean85
Copy link
Member

Jean85 commented Mar 9, 2024

Thank you for the help!

Also, keep in mind that ORM 3 is still usable with DBAL 3, you're not forced to upgrade them together, you can split the upgrade.

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