Skip to content

Allow dynamic intervals in DATE_ADD & DATE_SUB for SQLite#3019

Merged
morozov merged 2 commits intodoctrine:masterfrom
fogs:date_add
Mar 29, 2018
Merged

Allow dynamic intervals in DATE_ADD & DATE_SUB for SQLite#3019
morozov merged 2 commits intodoctrine:masterfrom
fogs:date_add

Conversation

@fogs
Copy link
Copy Markdown
Contributor

@fogs fogs commented Feb 11, 2018

Fixing #2983
Replacing #2984

@fogs
Copy link
Copy Markdown
Contributor Author

fogs commented Feb 14, 2018

Hi @Ocramius,

This is the replacement for #2984 now including some unit tests. Let me know if that works for you or if you are missing something else.

The Scrutinizer came back with "Tests: passed" but is complaining about "Failure condition met". Is that something I need to fix and if so, how?

Our project is using doctrine/dbal in an older version. Will my patch be backported or can I do something to get it there? Would love to get rid of using my fork of doctrine in our project's composer.json.

Thanks!

@Ocramius
Copy link
Copy Markdown
Member

Heya! Please do ignore the Scrutinizer-CI failure here :-)

As for the backport: no, this is an addition, and it will land in DBAL 2.7

}

if (! is_numeric($interval)) {
$interval = "' || " . $interval . " || '";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The semantics depending on the value is a bit fishy. Do other platforms support something similar?

@morozov morozov requested a review from Ocramius March 29, 2018 03:24
@morozov
Copy link
Copy Markdown
Member

morozov commented Mar 29, 2018

Someone needs to approve this before it gets into 2.7.0.

@Ocramius Ocramius assigned morozov and unassigned Ocramius Mar 29, 2018
@morozov morozov merged commit a4015c8 into doctrine:master Mar 29, 2018
@fogs fogs deleted the date_add branch April 29, 2018 12:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022
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.

3 participants