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

feat: allow doctrine/dbal v4 #247

Merged
merged 1 commit into from
May 26, 2024
Merged

feat: allow doctrine/dbal v4 #247

merged 1 commit into from
May 26, 2024

Conversation

kochen
Copy link
Contributor

@kochen kochen commented Feb 7, 2024 •
edited

Description

Support doctrine/dbal:^4.0

Closes: #246

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR checklist

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.

@kochen kochen requested a review from ramsey as a code owner February 7, 2024 09:17
This was referenced Feb 10, 2024
Closed
Closed
@kochen
Copy link
Contributor Author

kochen commented Mar 8, 2024

@ramsey could you please take a look?

greg0ire
greg0ire reviewed Mar 12, 2024
View reviewed changes
composer.json Outdated Show resolved Hide resolved
@greg0ire
Copy link

@kochen I've created #253 to show how it could be possible to avoid forcing users to upgrade both packages at once. Feel free to incorporate the changes into your PR.

@greg0ire greg0ire mentioned this pull request Mar 12, 2024
Closed
@kochen
Copy link
Contributor Author

kochen commented Mar 13, 2024

Thanks @greg0ire - on it...

@kochen
Copy link
Contributor Author

kochen commented Mar 13, 2024

@greg0ire @ramsey now supporting both dbal v3 (& v2.8) as well as v4!

@kochen kochen changed the title feat: allow doctrine/dbal v4 and remove older versions feat: allow doctrine/dbal v4 Mar 13, 2024
@kochen kochen requested a review from greg0ire March 15, 2024 09:55
greg0ire
greg0ire approved these changes Mar 15, 2024
View reviewed changes
Copy link

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Looks great. I think some parts of your PR have nothing to do directly with doctrine/dbal 4 compat and you could split them out in a separate commit or even pull request for clarity.

@kochen
Copy link
Contributor Author

kochen commented Mar 18, 2024

Hi @greg0ire,
I tried reverting each of the changes I made and they all seem to be required for dbal v4.

Do you have one specific suspect you could point out?

greg0ire
greg0ire reviewed Mar 18, 2024
View reviewed changes
composer.json Outdated Show resolved Hide resolved
@tasselchof
Copy link

What need to be done here to get this pull request approved @greg0ire @ramsey?

@greg0ire
Copy link

I don't know, I'm not a maintainer of this repository

@tasselchof
Copy link

I don't know, I'm not a maintainer of this repository

Oh, I didn't realize you were just helping with this commit. Let's wait for the maintainer.

@greg0ire
Copy link

I'm a Doctrine maintainer, and I want DBAL 4 to get more adoption, hence my review :)

@tasselchof tasselchof mentioned this pull request Apr 17, 2024
Open
@tasselchof

This comment was marked as off-topic.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as off-topic.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as off-topic.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as off-topic.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as off-topic.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as outdated.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@tasselchof

This comment was marked as off-topic.

Sign in to view
@greg0ire

This comment was marked as off-topic.

Sign in to view
@ramsey
Copy link
Owner

ramsey commented Apr 27, 2024

This looks good, but it's failing the static analysis checks, and I don't have time to investigate. Can you look into it?

@kochen
Copy link
Contributor Author

kochen commented Apr 29, 2024

This looks good, but it's failing the static analysis checks, and I don't have time to investigate. Can you look into it?

@ramsey all the issues are related to the unsupported PHP 7.4
Is there any plan to drop support for it? doctrine/dbal:4 expects PHP > 8.1 anyway...

@kochen
Copy link
Contributor Author

kochen commented Apr 29, 2024

@ramsey could you approve the workflow again?

@kingjia90 kingjia90 mentioned this pull request Apr 29, 2024
Closed
@ToshY
Copy link

ToshY commented May 15, 2024

@ramsey Could you spare some time to take a look at this? Thanks in advance.

@codecov Codecov
Copy link

codecov bot commented May 18, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.25%. Comparing base (ee0b1ef) to head (6b9d3d6).
Report is 17 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #247      +/-   ##
============================================
- Coverage     99.27%   90.25%   -9.02%     
- Complexity       68       75       +7     
============================================
  Files             6        7       +1     
  Lines           138      154      +16     
============================================
+ Hits            137      139       +2     
- Misses            1       15      +14
Files Coverage Δ
src/UuidBinaryOrderedTimeType.php 94.28% <100.00%> (-5.72%) ⬇️
src/UuidBinaryType.php 87.09% <100.00%> (-12.91%) ⬇️
src/UuidType.php 85.71% <100.00%> (-14.29%) ⬇️
src/UuidV7Generator.php 85.71% <ø> (ø)
src/GetBindingTypeImplementation.php 50.00% <50.00%> (ø)

... and 1 file with indirect coverage changes

greg0ire
greg0ire reviewed May 18, 2024
View reviewed changes
tests/UuidBinaryOrderedTimeTypeTest.php
@@ -46,7 +47,7 @@ private function getType(): Type

public function testGetName(): void
{
$this->assertSame('uuid_binary_ordered_time', $this->getType()->getName());
$this->assertSame('uuid_binary_ordered_time', $this->getType()::lookupName($this->getType()));
Copy link

@greg0ire greg0ire May 18, 2024 •
edited

Choose a reason for hiding this comment

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

lookupName() was added in dbal 3.7.0: doctrine/dbal#6130

It's not available at all with DBAL 2. Maybe we should drop support for DBAL 2 in a separate PR first, or use lookupName() only when that method exists.

@ramsey ramsey merged commit 6b9d3d6 into ramsey:main May 26, 2024
9 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

@ramsey ramsey ramsey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Doctrine/dbal v4 is out!
5 participants
@kochen @greg0ire @tasselchof @ramsey @ToshY