Allow SortDirection enum in ObjectRepository::findBy() - #524
Conversation
Update the PHPDoc contract of findBy() to accept the PHP 8.6 SortDirection enum in addition to the 'asc', 'desc', 'ASC' and 'DESC' string literals, as requested in doctrine#523. The native signature is unchanged and strings remain accepted, so this is backward compatible for callers. Implementations that do not already handle the enum must accept it and map it to their underlying sort direction, otherwise a caller passing SortDirection will trigger a runtime error. doctrine/orm 3.7.0+ and doctrine/mongodb-odm 2.18+ already handle it. Add symfony/polyfill-php86 to require-dev so PHPStan can resolve the SortDirection symbol on PHP 8.1-8.5 without a production dependency. Refs doctrine#523
a5ae8e3 to
cf0ecac
Compare
| * @param array<string, string>|null $orderBy | ||
| * @phpstan-param array<string, 'asc'|'desc'|'ASC'|'DESC'>|null $orderBy | ||
| * @param array<string, mixed> $criteria | ||
| * @param array<string, string|SortDirection>|null $orderBy |
There was a problem hiding this comment.
Widening parameter type is a BC break for implementers (as implementers are not allowed to use a more restrictive type based on the Liskov Substitution Principle). So to me, this change must happen only in the 5.0.x branch.
There was a problem hiding this comment.
You're absolutely right. And even if it doesn't break the PHP code because the type isn't enforced by PHP, we're providing information about a supported type that isn't respected by all older versions of ORM/ODM. Adding a composer conflict rule with older ORM versions would be really dirty.
The solution, therefore, is to accept string|SortDirection in 5.0 and (possibly) remove support for string in 6.0.
There was a problem hiding this comment.
Adding a composer conflict rule with older ORM versions would be really dirty.
Indeed. An interface package that would be forced to add conflicts for older versions of implementation packages would be a clear signal that the interface package does not follow semver (assuming implementation packages don't use unbounded constraints of course but rely on semver constraints)
There was a problem hiding this comment.
Can this maybe be achieved with @method?
There was a problem hiding this comment.
@greg0ire no. We don't want to add a new method. We want to change the signature of an existing method.
There was a problem hiding this comment.
Ok, too bad there isn't an annotation to suggest this.
3 participants
Follows #523. Companion PR on 5.0.x: #525.
ObjectRepository::findBy()$orderBynow accepts the PHP 8.6SortDirectionenum in addition to the'asc','desc','ASC'and'DESC'string literals. The native signature is unchanged and strings remain accepted, so this is backward compatible for callers.Implementations that do not already handle the enum must accept it and map it to their underlying sort direction, otherwise a caller passing
SortDirectionwill trigger a runtime error.Downstream support:
doctrine/ormsupportsSortDirectioninfindBy()andfindOneBy()starting with 3.7.0. Older versions (3.6.x and below, including the 2.20.x/2.21.x LTS lines) only accept strings and will not receive this change; upgrade to 3.7.0 or later to use the enum.doctrine/mongodb-odmsupportsSortDirectioninfindBy()starting with 2.18. Older versions (2.17.x and below) only accept strings and will not receive this change; upgrade to 2.18 or later. Its query buildersort()already accepts the enum since 2.17.0, butfindBy()bypasses the query builder.symfony/polyfill-php86is added torequire-devso PHPStan can resolveSortDirectionon PHP 8.1-8.5 without a production dependency. The PHPDoc is validated by the static-analysis CI; the runtime test is a smoke test guarding against a future narrowing of the native type.