Rename andX() / orX() methods #3851

BenMorel · 2020-01-26T13:09:29Z

Q	A
Type	improvement
BC Break	no
Fixed issues	none

Summary

ExpressionBuilder::andX() and orX() have likely been named as such because before PHP 7 and its context sensitive lexer, methods could not be named and() or or().

I think it's time to change this for the next major version.

greg0ire · 2020-01-26T15:13:12Z

IMO it would make sense to target 2.10 and deprecate andX / orX instead of removing them.

BenMorel · 2020-01-26T15:22:00Z

@greg0ire I'm definitely in favour of adding and()/or() to 2.x and deprecate andX()/orX(). I can't do it in 2.10 though, it has to be in a minor version according to semver, not a patch version.

@morozov, is there a 2.11 version planned, and in this case, is there a branch for it?

Note that I would definitely remove them in 3.0, you don't want to carry BC stuff for too long. One minor version would be enough.

greg0ire · 2020-01-26T15:28:03Z

is there a 2.11 version planned, and in this case, is there a branch for it?

Also, why is there a 2.10 and 2.10.x branch?

morozov · 2020-01-26T16:56:16Z

@morozov, is there a 2.11 version planned, and in this case, is there a branch for it?

Yes. It is planned primarily to introduce all needed deprecations and provide a forward compatibility layer. I've just created 2.11.x from v2.10.1.

morozov · 2020-01-26T16:58:50Z

Note that I would definitely remove them in 3.0, you don't want to carry BC stuff for too long. One minor version would be enough.

The original purpose of 3.0 is to introduce the minimal needed BC break for compatibility with PHP 8 and other trivial to handle deprecations (e.g. removal of no longer supported DB platforms). We're going to postpone the remaining API changes for 4.0.

morozov · 2020-01-26T17:00:51Z

Also, why is there a 2.10 and 2.10.x branch?

2.10 is what we used historically for 2.x branches. 2.10.x is created by the @doctrinebot after each release in the 2.10.x series.

morozov · 2020-01-26T17:02:06Z

@BenMorel please retarget against 2.11.x.

BenMorel · 2020-01-26T17:21:08Z

The original purpose of 3.0 is to introduce the minimal needed BC break for compatibility with PHP 8 and other trivial to handle deprecations (e.g. removal of no longer supported DB platforms). We're going to postpone the remaining API changes for 4.0.

Oh,my. When is 4.0 expected? I was hoping all the work done here would be released in the coming months, for a project I wanted based on the latest DBAL.

morozov · 2020-01-26T17:36:02Z

The original purpose of 3.0 is […]

Sorry, not the best choice of words. 4.0 will be released at the same time when the original 3.0 would be which is undetermined. In order to address compatibility with PHP 8 (#3803), we want to make a small 3.0 release and continue working on the rest of the planned breaking changes.

BenMorel · 2020-01-27T10:23:04Z

I've just created 2.11.x from v2.10.1.

Looks like you only created the branch in your fork, not upstream.

greg0ire · 2020-01-27T12:38:16Z

Indeed. I just created it. @morozov I don't know if there are settings to tweak but I didn't tweak any.

BenMorel · 2020-01-27T14:12:01Z

Retargeted against 2.11.x and deprecated methods.

morozov · 2020-01-27T14:35:16Z

Looks like you only created the branch in your fork, not upstream.

Indeed. Thanks, @greg0ire. I added the branch to Scrutinizer and that should be enough.

lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php

morozov · 2020-01-27T14:39:20Z

In light of doctrine/coding-standard#150, I'm not requesting the addition of @trigger_error().

greg0ire · 2020-01-27T14:40:46Z

In both cases, it should not be added until the deprecation is addressed in all dependent packages if any (and in the code of DBAL itself of course).

morozov · 2020-01-27T14:42:18Z

That's my problem with the approach. It is by definition deprecated as soon as there's another API to use for the same purpose. Documenting it one way or the other depending on the state of the world outside is BS.

BenMorel · 2020-01-27T15:12:06Z

lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php

@@ -52,7 +52,7 @@ public function __construct(Connection $connection)
     *
     * @return CompositeExpression
     */
-    public function andX($x = null)
+    public function and($x = null)


Because these are new methods, should we add variadics right now to align the definition of these methods with upcoming 3.0 or 4.0, and ease transition later?

Definitely. They should look as close to what the consumers will get in the new versions as possible.

Do we want the first argument to be optional here?

Definitely. They should look as close to what the consumers will get in the new versions as possible.

OK done!

Do we want the first argument to be optional here?

That's the job of #3845. I'm getting lost over what to do first, and on which branch.

morozov · 2020-01-27T16:30:51Z

lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php

@@ -39,41 +39,61 @@ public function __construct(Connection $connection)
    }

    /**
-     * Creates a conjunction of the given boolean expressions.
+     * Creates a conjunction of the given expressions.
     *
     * Example:
     *
     *     [php]
     *     // (u.type = ?) AND (u.role = ?)
     *     $expr->andX('u.type = ?', 'u.role = ?'));


Should we just get rid of this example? It suggests the usage of deprecated code. We're too used to being covered by static analysis to maintain code examples in comments.

Good point, totally missed that. Fixed in 2c69e55.

greg0ire · 2020-01-27T18:05:37Z

Documenting it one way or the other depending on the state of the world outside is BS.

Well you can document it with @deprecated from day one. @trigger_error is way more aggressive and can have implication on production systems, so it should be added when you're sure people are not going to be stuck with unfixable deprecations, because it is stronger than just "documenting".

Anyway, when I see the debate in the link above, I think what is going to happen is that there will be deprecations with @trigger_error, and packages without, and DBAL is probably going to be one of the packages without because you're clearly the owner of that package and if you're not fully on board with such a decision, it's not ok to push it in this package.

I think we all agree to use @deprecated in this PR, and that's great.

greg0ire · 2020-01-27T19:27:33Z

@BenMorel please update docs/en/reference/query-builder.rst as well.
@morozov should any mention of deprecated be removed entirely, or should we keep them in a dedicated section / use specific rst markup that will allow to document them while discouraging their usage?

morozov · 2020-01-27T19:36:50Z

[…] should any mention of deprecated be removed entirely, or should we keep them in a dedicated section / use specific rst markup that will allow to document them while discouraging their usage?

I haven’t made any documentation changes like this in the past. If it is possible to document both and mark the old API deprecated, that would be nice. Otherwise, we can replace the old examples with the new ones.

BenMorel · 2020-01-27T19:38:10Z

please update docs/en/reference/query-builder.rst as well.

Good point. Done in 625a70d.

morozov · 2020-01-27T19:43:13Z

@BenMorel, looks good. Please squash.

lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php

BenMorel · 2020-01-27T19:46:09Z

@morozov Squashed!

BenMorel · 2020-01-28T09:27:34Z

@morozov Is there anything blocking the merge?

morozov · 2020-01-28T14:14:22Z

Thanks for the reminder. Merged.

BenMorel · 2020-01-28T14:25:58Z

Thank you.

Do we want the first argument to be optional here?

Should we now make the first argument mandatory in 2.11?

morozov · 2020-01-28T14:28:24Z

Yes, the new API should look like in master to provide forward compatibility.

BenMorel · 2020-01-28T14:41:21Z

It's not done in master yet, but I guess you'll merge it into master later, removing the andX/orX?

morozov · 2020-01-28T16:09:20Z

Yeah. We’ll build FC and deprecate the old APIs first, then merge stuff up and remove the depreciated code.

@gjdanis

Release [2.11.0](https://github.com/doctrine/dbal/milestone/77) 2.11.0 ====== - Total issues resolved: **7** - Total pull requests resolved: **55** - Total contributors: **8** Improvement,Prepared Statements,SQL Server,Types,pdo_sqlsrv,sqlsrv ------------------------------------------------------------------ - [4274: Support ASCII parameter binding](doctrine#4274) thanks to @gjdanis Documentation ------------- - [4271: Add explanation about implicit indexes](doctrine#4271) thanks to @greg0ire Deprecation,Error Handling -------------------------- - [4253: Deprecate DBAL\DBALException in favor of DBAL\Exception](doctrine#4253) thanks to @morozov CI -- - [4251: Setup automatic release workflow](doctrine#4251) thanks to @greg0ire Deprecation,Schema Managers --------------------------- - [4230: Deprecate the functionality of dropping client connections when dropping a database](doctrine#4230) thanks to @morozov Deprecation,Platforms --------------------- - [4229: Deprecate more AbstractPlatform methods](doctrine#4229) thanks to @morozov - [4132: Deprecate AbstractPlatform::fixSchemaElementName()](doctrine#4132) thanks to @morozov Improvement,Test Suite ---------------------- - [4215: Remove test group configuration leftovers](doctrine#4215) thanks to @morozov - [4080: Update PHPUnit to 9.2](doctrine#4080) thanks to @morozov - [4079: Forward compatibility with PHPUnit 9.3](doctrine#4079) thanks to @morozov - [3923: Removed performance tests](doctrine#3923) thanks to @morozov Deprecation,Schema ------------------ - [4213: Deprecate the Synchronizer package](doctrine#4213) thanks to @morozov Blocker,Improvement,PHP,Test Suite ---------------------------------- - [4201: Update PHPUnit to 9.3](doctrine#4201) thanks to @morozov Blocker,PHP,Test Suite ---------------------- - [4196: The test suite uses the deprecated at() matcher](doctrine#4196) thanks to @morozov Connections,Deprecation,Documentation ------------------------------------- - [4175: Additional deprecation note for PrimaryReplicaConnection::query()](doctrine#4175) thanks to @morozov Connections,Deprecation,Prepared Statements ------------------------------------------- - [4165: Deprecated usage of wrapper components as implementations of driver-level interfaces](doctrine#4165) thanks to @morozov - [4020: Deprecated Connection::project(), Statement::errorCode() and errorInfo()](doctrine#4020) thanks to @morozov Connections,Deprecation ----------------------- - [4163: Deprecate duplicate and ambiguous wrapper connection methods](doctrine#4163) thanks to @morozov Error Handling,Improvement,Types -------------------------------- - [4145: Add TypeRegistry constructor](doctrine#4145) thanks to @morozov Deprecation,Drivers,Improvement,pdo_mysql,pdo_oci,pdo_pgsql,pdo_sqlite,pdo_sqlsrv --------------------------------------------------------------------------------- - [4144: Deprecate classes in Driver\PDO* namespaces](doctrine#4144) thanks to @morozov Connections,Documentation,Improvement ------------------------------------- - [4139: Mark connection constructors internal](doctrine#4139) thanks to @morozov Deprecation,Drivers,Error Handling ---------------------------------- - [4137: Deprecate driver exception conversion APIs](doctrine#4137) thanks to @morozov - [4112: Deprecate DriverException::getErrorCode()](doctrine#4112) thanks to @morozov Configuration,Connections,Deprecation,Error Handling ---------------------------------------------------- - [4134: Deprecate some DBALException factory methods](doctrine#4134) thanks to @morozov Code Style,Documentation ------------------------ - [4133: Fix more issues introduced by the deprecation of driver classes](doctrine#4133) thanks to @morozov BC Break,Drivers,Error Handling,pdo_sqlsrv,sqlsrv ------------------------------------------------- - [4131: Restore the PortWithoutHost exception parent class](doctrine#4131) thanks to @morozov Code Style,Improvement,Static Analysis -------------------------------------- - [4123: Remove the no longer needed error suppressions](doctrine#4123) thanks to @morozov Deprecation,Drivers,Error Handling,Improvement ---------------------------------------------- - [4118: Deprecate ExceptionConverterDriver](doctrine#4118) thanks to @morozov Bug,Connections,Improvement,Prepared Statements ----------------------------------------------- - [4117: Fixes for the recently introduced driver-level deprecations](doctrine#4117) thanks to @morozov Connections,Deprecation,Platform Detection ------------------------------------------ - [4114: Deprecate ServerInfoAwareConnection#requiresQueryForServerVersion() as an implementation detail](doctrine#4114) thanks to @morozov Deprecation,Prepared Statements,SQL Parser,oci8 ----------------------------------------------- - [4110: Mark non-interface OCI8 driver methods internal](doctrine#4110) thanks to @morozov Connections,Deprecation,Drivers,Improvement,Prepared Statements --------------------------------------------------------------- - [4100: Deprecate inconsistently and ambiguously named driver-level classes](doctrine#4100) thanks to @morozov Connections,Improvement ----------------------- - [4092: Remove Connection::$isConnected](doctrine#4092) thanks to @morozov Configuration,Connections ------------------------- - [4086: Mark Connection::getParams() internal](doctrine#4086) thanks to @morozov Bug,Drivers,ibm_db2 ------------------- - [4085: The IBM DB2 driver Exception class must implement the DriverException interface](doctrine#4085) thanks to @morozov PHP --- - [4078: Bump PHP requirement to 7.3 as of DBAL 2.11.0](doctrine#4078) thanks to @morozov Connections,Databases,Deprecation,Drivers ----------------------------------------- - [4068: Deprecate Driver::getDatabase()](doctrine#4068) thanks to @morozov Deprecation,Improvement,Portability ----------------------------------- - [4061: Deprecated platform-specific portability mode constants](doctrine#4061) thanks to @morozov - [4054: &doctrine#91;doctrineGH-4052&doctrine#93; Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection](doctrine#4054) thanks to @beberlei and @albe - [4017: Improve help of dbal:run-sql command](doctrine#4017) thanks to @ostrolucky Code Style,Improvement ---------------------- - [4050: Update doctrine/coding-standard to 8.0](doctrine#4050) thanks to @morozov Cache,Deprecation,Improvement,Prepared Statements ------------------------------------------------- - [4049: Replace forward-compatible ResultStatement interfaces with Result](doctrine#4049) thanks to @morozov Cache,Improvement,Prepared Statements ------------------------------------- - [4048: Make caching layer not rely on closeCursor()](doctrine#4048) thanks to @morozov Deprecation,Improvement,Prepared Statements ------------------------------------------- - [4037: Introduce Statement::fetchFirstColumn()](doctrine#4037) thanks to @morozov - [4019: Deprecated the concept of the fetch mode](doctrine#4019) thanks to @morozov Bug,Documentation,Improvement,Prepared Statements ------------------------------------------------- - [4034: Additional changes based on the discussion in doctrine#4007](doctrine#4034) thanks to @morozov Connections,Console,Improvement ------------------------------- - [3956: allow using multiple connections for CLI commands](doctrine#3956) thanks to @dmaicher Deprecation,Logging ------------------- - [3935: Deprecate EchoSQLLogger](doctrine#3935) thanks to @morozov Improvement,Packaging --------------------- - [3924: Actualize the content of the .gitattributes file](doctrine#3924) thanks to @morozov Azure,Deprecation,Drivers,Drizzle,MariaDB,Platforms,PostgreSQL,SQL Anywhere,SQL Server,Sharding,pdo_ibm ------------------------------------------------------------------------------------------------------- - [3905: Deprecate the usage of the legacy platforms and drivers](doctrine#3905) thanks to @morozov Deprecation,Query ----------------- - [3864: CompositeExpression and()/or() factory methods](doctrine#3864) thanks to @BenMorel - [3853: Deprecate calling QueryBuilder methods with an array argument](doctrine#3853) thanks to @BenMorel and @morozov Deprecation,Tools ----------------- - [3861: Deprecated the usage of the Version class](doctrine#3861) thanks to @morozov Improvement,Query ----------------- - [3852: First parameter of ExpressionBuilder::and/or() mandatory](doctrine#3852) thanks to @BenMorel Deprecation,Improvement,Query ----------------------------- - [3851: Rename andX() / orX() methods](doctrine#3851) thanks to @BenMorel - [3850: Prepare CompositeExpression for immutability](doctrine#3850) thanks to @BenMorel # gpg: Signature made Mon Sep 21 01:47:31 2020 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key # Conflicts: # README.md # lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php # lib/Doctrine/DBAL/Driver/OCI8/Driver.php # lib/Doctrine/DBAL/Version.php

BenMorel force-pushed the andx branch from ae8ed3f to 4434d39 Compare January 27, 2020 14:10

BenMorel changed the base branch from master to 2.11.x January 27, 2020 14:10

BenMorel requested a review from morozov January 27, 2020 14:12

morozov reviewed Jan 27, 2020

View reviewed changes

lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php Outdated Show resolved Hide resolved

BenMorel force-pushed the andx branch from 4434d39 to 773f8b9 Compare January 27, 2020 15:07

BenMorel commented Jan 27, 2020

View reviewed changes

BenMorel force-pushed the andx branch from 773f8b9 to 6b36973 Compare January 27, 2020 15:29

morozov added Deprecation Improvement QueryBuilder labels Jan 27, 2020

morozov reviewed Jan 27, 2020

View reviewed changes

greg0ire approved these changes Jan 27, 2020

View reviewed changes

morozov added this to the 2.11.0 milestone Jan 27, 2020

morozov requested changes Jan 27, 2020

View reviewed changes

lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php Outdated Show resolved Hide resolved

Replace andX/orX with and/or

84328cd

BenMorel force-pushed the andx branch from 625a70d to 84328cd Compare January 27, 2020 19:45

morozov approved these changes Jan 27, 2020

View reviewed changes

morozov merged commit 130e158 into doctrine:2.11.x Jan 28, 2020

BenMorel deleted the andx branch January 28, 2020 14:26

BenMorel mentioned this pull request Jan 28, 2020

First parameter of ExpressionBuilder::and/or() mandatory #3852

Merged

morozov self-assigned this Jan 28, 2020

BenMorel mentioned this pull request Jan 30, 2020

Remove ExpressionBuilder::andX()/orX() #3865

Merged

github-actions bot mentioned this pull request Sep 20, 2020

Merge release 2.11.0 into 3.0.x #4277

Merged

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021

[doctrineGH-3851] Trigger deprecation for ExpressionBuilder::andX/orX

e3fc3f5

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021

[doctrineGH-3851] Trigger deprecation for ExpressionBuilder::andX/orX

b80d236

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 11, 2021

[doctrineGH-3851] Trigger deprecation for ExpressionBuilder::andX/orX

07e4ec8

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 22, 2021

[doctrineGH-3851] Trigger deprecation for ExpressionBuilder::andX/orX

7b22861

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 27, 2021

[doctrineGH-3851] Trigger deprecation for ExpressionBuilder::andX/orX

d0b88d5

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename andX() / orX() methods #3851

Rename andX() / orX() methods #3851

BenMorel commented Jan 26, 2020 •

edited by morozov

Loading

greg0ire commented Jan 26, 2020

BenMorel commented Jan 26, 2020

greg0ire commented Jan 26, 2020

morozov commented Jan 26, 2020

morozov commented Jan 26, 2020

morozov commented Jan 26, 2020

morozov commented Jan 26, 2020

BenMorel commented Jan 26, 2020

morozov commented Jan 26, 2020

BenMorel commented Jan 27, 2020

greg0ire commented Jan 27, 2020

BenMorel commented Jan 27, 2020

morozov commented Jan 27, 2020

morozov commented Jan 27, 2020

greg0ire commented Jan 27, 2020

morozov commented Jan 27, 2020

BenMorel Jan 27, 2020

morozov Jan 27, 2020

morozov Jan 27, 2020

BenMorel Jan 27, 2020

morozov Jan 27, 2020 •

edited

Loading

BenMorel Jan 27, 2020

greg0ire commented Jan 27, 2020

greg0ire commented Jan 27, 2020 •

edited

Loading

morozov commented Jan 27, 2020

BenMorel commented Jan 27, 2020

morozov commented Jan 27, 2020

BenMorel commented Jan 27, 2020

BenMorel commented Jan 28, 2020

morozov commented Jan 28, 2020

BenMorel commented Jan 28, 2020

morozov commented Jan 28, 2020

BenMorel commented Jan 28, 2020

morozov commented Jan 28, 2020

Rename andX() / orX() methods #3851

Rename andX() / orX() methods #3851

Conversation

BenMorel commented Jan 26, 2020 • edited by morozov Loading

Summary

greg0ire commented Jan 26, 2020

BenMorel commented Jan 26, 2020

greg0ire commented Jan 26, 2020

morozov commented Jan 26, 2020

morozov commented Jan 26, 2020

morozov commented Jan 26, 2020

morozov commented Jan 26, 2020

BenMorel commented Jan 26, 2020

morozov commented Jan 26, 2020

BenMorel commented Jan 27, 2020

greg0ire commented Jan 27, 2020

BenMorel commented Jan 27, 2020

morozov commented Jan 27, 2020

morozov commented Jan 27, 2020

greg0ire commented Jan 27, 2020

morozov commented Jan 27, 2020

BenMorel Jan 27, 2020

Choose a reason for hiding this comment

morozov Jan 27, 2020

Choose a reason for hiding this comment

morozov Jan 27, 2020

Choose a reason for hiding this comment

BenMorel Jan 27, 2020

Choose a reason for hiding this comment

morozov Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

BenMorel Jan 27, 2020

Choose a reason for hiding this comment

greg0ire commented Jan 27, 2020

greg0ire commented Jan 27, 2020 • edited Loading

morozov commented Jan 27, 2020

BenMorel commented Jan 27, 2020

morozov commented Jan 27, 2020

BenMorel commented Jan 27, 2020

BenMorel commented Jan 28, 2020

morozov commented Jan 28, 2020

BenMorel commented Jan 28, 2020

morozov commented Jan 28, 2020

BenMorel commented Jan 28, 2020

morozov commented Jan 28, 2020

BenMorel commented Jan 26, 2020 •

edited by morozov

Loading

morozov Jan 27, 2020 •

edited

Loading

greg0ire commented Jan 27, 2020 •

edited

Loading