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

Replace || by concat on postgreSQLPlatform #3887

Closed
wants to merge 2 commits into from

Conversation

NiniGeek
Copy link

@NiniGeek NiniGeek commented Mar 4, 2020

Q A
Type improvement
BC Break yes
Fixed issues doctrine/orm#3938

Summary

For PostGreSQL, the CONCAT DQL function is translated in concatenation with || operator (which is the default behavior in AbstractPlatform class).

The concatenation with || operator return null if one of the part is null, whereas CONCAT function will simply ignore null values.

@SenseException
Copy link
Member

The changes are looking good, but I'm not sure if this should be part of 2.10 or for the next major release. If CONCAT wasn't used in DBAL because of PostgreSQL's legacy (|| was there before the SQL function was added), this won't be a bug fix, but a BC break.

I currently would like to see this to be in the next major release if || was preferred for compatibility reasons in the past. I also would like to hear what @doctrine/team-dbal have to say about it.

@morozov
Copy link
Member

morozov commented Mar 4, 2020

Agree with the above.

@oleg-andreyev
Copy link

It seems that CONCAT function was added only in PostgreSQL 9.1 (https://www.postgresql.org/docs/9.1/functions-string.html#FUNCTIONS-STRING-OTHER) so without checking Platform Version I think it cannot be merged.

@morozov
Copy link
Member

morozov commented Apr 1, 2020

Since the different implementations CONCAT() and || have different semantics, before choosing what's right and what's wrong, I'd like to have a cross-platform integration test that covers other platforms. We cannot change the semantics of the API just because the new implementation looks better.

@oleg-andreyev
Copy link

I've decided to check Travis configuration and found that DBAL is tested against PostgreSQL 9.4

env: DB=pgsql POSTGRESQL_VERSION=9.4 COVERAGE=yes

so can we say that 9.4 is the lowest version DBAL is supporting?

@morozov
Copy link
Member

morozov commented Apr 1, 2020

@oleg-andreyev DBAL 2 supports at least PostgreSQL 9.1 as the PostgreSQL91Platform class implies. We don't test the versions older than 9.4 on Travis because they are not supported by the community and that would require extra effort from the team. But we don't accept any changes that would knowingly break the support.

The support for PostgreSQL versions lower than 9.4 will be dropped in DBAL 3.0 (#3912).

@morozov morozov changed the base branch from 2.10.x to 2.12.x November 29, 2020 21:34
@morozov morozov closed this Apr 21, 2021
@morozov morozov deleted the branch doctrine:2.12.x April 21, 2021 16:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants