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

Reworked AbstractPlatform::get*Expression() methods #3498

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

morozov
Copy link
Member

@morozov morozov commented Mar 27, 2019

Q A
Type improvement
BC Break yes

This pull request contains the breaking changes in AbstractPlatform::get*Expression() methods from #3348.

The key changes are:

  1. The arguments of the changed methods which previously could accept an SQL expression but weren't annotated so are now annotated.
  2. The arguments which were declared as int but de-facto accepted an SQL expression are now type-hinted as string.
  3. As a side effect of the above, with a slight rework in the implementation, the $interval argument of the getDate(Add|Sub)*Expression() methods can now accept an SQL expression which makes it possible to define the interval as a numeric literal (former behavior), prepared statement parameter, column name or technically any valid SQL expression 🔥
  4. The natively unsupported date intervals (e.g. weeks in SQLite) are now converted in SQL instead of the client side. Hence some changes in assertions against generated SQL on some platforms.
  5. DataAccessTest::testDateArithmetics() has been reworked into a series of independent tests each of which represents the interval in the query in three modes: numeric literal, prepared statement parameter and an SQL expression.
  6. As a side effect of the rework of SqlitePlatform::getDateArithmeticIntervalExpression(), it no longer truncates the time part of the result of addition or subtraction of days, weeks, months, quarters or years. This issue can be filed and fixed additionally in master since it impacts the code portability.
  7. The names of the arguments were renamed closer to their SQL analogs.
  8. The behavior implemented in Allow dynamic intervals in DATE_ADD & DATE_SUB for SQLite #3019 is now by design.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM (besides nits)

@Ocramius Ocramius added this to the 3.0.0 milestone Mar 27, 2019
@Ocramius Ocramius merged commit 8cb2fb6 into doctrine:develop Mar 27, 2019
@Ocramius Ocramius assigned Ocramius and unassigned morozov Mar 27, 2019
@morozov morozov deleted the get-other-expr branch March 27, 2019 14:57
morozov pushed a commit that referenced this pull request Apr 16, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request May 6, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request May 23, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit to morozov/dbal that referenced this pull request May 31, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request Jun 13, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request Jun 27, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request Jun 27, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request Jun 27, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Reworked AbstractPlatform::get*Expression() methods
morozov pushed a commit that referenced this pull request Nov 2, 2019
Reworked AbstractPlatform::get*Expression() methods
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants