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

Update SoftDeleteable to work with Doctrine 3.1 #2801

Conversation

elfantome
Copy link
Contributor

Update Gedmo\SoftDeleteable\Filter\SoftDeleteableFilter::addFilterConstraint signature to be compatible with Doctrine\ORM\Query\Filter\SQLFilter::addFilterConstraint

…nstraint to make compatible with \Doctrine\ORM\Query\Filter\SQLFilter::addFilterConstraint
@mbabker
Copy link
Contributor

mbabker commented May 20, 2024

Unfortunately this change breaks compatibility with ORM 2.x.

Fatal error: Declaration of Gedmo\SoftDeleteable\Filter\SoftDeleteableFilter::addFilterConstraint(Doctrine\ORM\Mapping\ClassMetadata $targetEntity, string $targetTableAlias): string must be compatible with Doctrine\ORM\Query\Filter\SQLFilter::addFilterConstraint(Doctrine\ORM\Mapping\ClassMetadata $targetEntity, $targetTableAlias) in /src/SoftDeleteable/Filter/SoftDeleteableFilter.php on line 54

@elfantome
Copy link
Contributor Author

Unfortunately this change breaks compatibility with ORM 2.x.

Fatal error: Declaration of Gedmo\SoftDeleteable\Filter\SoftDeleteableFilter::addFilterConstraint(Doctrine\ORM\Mapping\ClassMetadata $targetEntity, string $targetTableAlias): string must be compatible with Doctrine\ORM\Query\Filter\SQLFilter::addFilterConstraint(Doctrine\ORM\Mapping\ClassMetadata $targetEntity, $targetTableAlias) in /src/SoftDeleteable/Filter/SoftDeleteableFilter.php on line 54

In this case, maybe bumping the version number with a new release can solve the problem?

@mbabker
Copy link
Contributor

mbabker commented May 21, 2024

I just looked at the install stats. It is way too early to consider dropping ORM 2.x support, and the volunteer resources don't exist to support two major version branches of this package.

There might be a way to change the signature in a way to support both versions (like only adding the return type without the param type), but based on numbers and resources only supporting ORM 3.x right now is not the right decision.

…date only the return type to keep backward compatibility
@elfantome
Copy link
Contributor Author

elfantome commented May 21, 2024

Makes sense.
I updated my PR to only add a return type to the method to keep compatibility with ORM 2.x

@elfantome elfantome changed the title Update SoftDeleteableFilter to work with Doctrine 3.1 Update SoftDeleteable to work with Doctrine 3.1 May 26, 2024
@@ -23,8 +23,6 @@
trait SoftDeleteableEntity
{
/**
* @ORM\Column(type="datetime", nullable=true)

Choose a reason for hiding this comment

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

Needs to be reverted

@mbabker mbabker mentioned this pull request Jun 9, 2024
Copy link

codecov bot commented Jun 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.40%. Comparing base (0632ab1) to head (18dd98c).
Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2801      +/-   ##
==========================================
- Coverage   78.75%   78.40%   -0.35%     
==========================================
  Files         163      167       +4     
  Lines        8593     8627      +34     
==========================================
- Hits         6767     6764       -3     
- Misses       1826     1863      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@franmomu franmomu merged commit 236c91c into doctrine-extensions:main Jun 9, 2024
19 of 20 checks passed
@franmomu
Copy link
Collaborator

franmomu commented Jun 9, 2024

thanks @elfantome!

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.

4 participants