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

[AllBundles] Postgres compatibility fixes #1778

Merged
merged 7 commits into from
Jan 30, 2018

Conversation

acrobat
Copy link
Member

@acrobat acrobat commented Jan 5, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets
Version used 5.0.0-RC2

I've started a new project with a postgres database and I got a lot of errors to start with. These fixes are "needed" to be able to use postgres as the backend database. I have tested these fixes with a clean install on mysql and postgres.

The biggest problem was the custom query for the acl check. Other queries don't use db specific stuff of use orm/dbal which is compatible by default.

Summary of the fixes:

  • Postgres strict compares fields so varying char = bigint with work in a where. So cast the field to the correct type (acl query)
  • In the sql standard single quotes are used for strings
  • In the sql standard boolean fields should be compared to false/true instead of 0/1
  • Postgres does not have an IF function, instead it has an COALESCE function

@acrobat acrobat changed the title Postgres compatibility fixes [AllBundles] Postgres compatibility fixes Jan 5, 2018
@acrobat
Copy link
Member Author

acrobat commented Jan 18, 2018

@Devolicious any chance this can still make it in 5.0? Or will it be moved to the first bugfix on 5.0?

@Devolicious
Copy link
Contributor

@acrobat I think that since this is a bugfix that there are 2 options. First option is like you said, wait until V5 has been release and than apply it as a bugfix for 5.0.1. Second option is that we rebase this and submit it to the 4.1 branch. From a safety point of view I like the first option better since we are moving away from V4 and that would mean that if we later found out that something is broken, that 4.1 stays broken since we don't support it anymore than. On the other hand you tested it, tests succeeded, so it would be nice if we could add this to 4.1 as well.

@acrobat acrobat force-pushed the postgres-compatibility-fixes branch from e0ab8de to 183e948 Compare January 18, 2018 13:20
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR needs some changes

  • your should rebase your PR so that it only contains 1 commit
  • A core contributor should set the correct milestone for this PR

@acrobat acrobat changed the base branch from 5.0 to 4.1 January 18, 2018 13:20
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR needs some changes

  • your should rebase your PR so that it only contains 1 commit
  • A core contributor should set the correct milestone for this PR

@acrobat acrobat force-pushed the postgres-compatibility-fixes branch from 183e948 to 54f4770 Compare January 18, 2018 13:23
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR needs some changes

  • your should rebase your PR so that it only contains 1 commit
  • A core contributor should set the correct milestone for this PR

@acrobat
Copy link
Member Author

acrobat commented Jan 18, 2018

@Devolicious I've rebased the PR against 4.1, surprisingly no merge conflicts 🎉 💪 So this should be good to go for 4.1. I don't have any bundles 4 projects so only tested on a 5 project but as there were no merge conflicts this should work on all versions.

@Devolicious Devolicious added this to the 4.1.1 milestone Jan 30, 2018
@Devolicious Devolicious merged commit 2027349 into Kunstmaan:4.1 Jan 30, 2018
@acrobat acrobat deleted the postgres-compatibility-fixes branch January 30, 2018 19:09
Devolicious added a commit that referenced this pull request Jan 31, 2018
* 4.1:
  update changelog
  [AllBundles] Postgres compatibility fixes (#1778)
Devolicious added a commit that referenced this pull request Jan 31, 2018
* master:
  fix command
  update changelog
  [GeneratorBundle] Fix missing browser sync option for generate commands (#1798)
  update changelog
  [AllBundles] Postgres compatibility fixes (#1778)
sandergo90 added a commit to sandergo90/KunstmaanBundlesCMS that referenced this pull request Feb 6, 2018
@acrobat acrobat restored the postgres-compatibility-fixes branch February 6, 2018 15:25
Devolicious pushed a commit that referenced this pull request Feb 6, 2018
[AllBundles] Revert "Postgres compatibility fixes (#1778)"
Devolicious added a commit that referenced this pull request Feb 6, 2018
* 4.1:
  update changelog
  Revert "[AllBundles] Postgres compatibility fixes (#1778)" (#1810)
Devolicious added a commit that referenced this pull request Feb 6, 2018
* 4.2:
  update changelog
  Revert "[AllBundles] Postgres compatibility fixes (#1778)" (#1810)
Devolicious added a commit that referenced this pull request Feb 6, 2018
* 5.0:
  update changelog
  update changelog
  [KunstmaanAdminBundle] change php7 to php5 code (#1807)
  Revert "[AllBundles] Postgres compatibility fixes (#1778)" (#1810)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants