[5.6] Fix postgres blueprint compileComment to proper escape its content

[mfn]

Only affects 5.6, code is new

The implementation in #21855 suffers from a slight error when trying to escape the columns comment.

By default, single quotes in Postgres aren't escaped with a backslash but with doubling the single quote.

Instead of … 'test '\ quote' it has to be … 'test '' quote'. Couldn't think of a better way than to use str_replace

/cc @andersondanilo

References:

• https://stackoverflow.com/a/12320729/47573
• http://php.net/manual/en/function.pg-escape-string.php

  addslashes() must not be used with PostgreSQL.

[taylorotwell]

Will removing addslashes cause other problems? Why was it removed?

[mfn]

Will removing addslashes cause other problems? Why was it removed?

Because it will transform a string like foo ' bar into foo \' bar and postgres does not escape single quotes with \ but ' i.e. to escape a single quote ' you have to use two single quotes => ''.

It was introduced with #21855 recently but it's just wrong to use it. If you have comments with single quotes and try it in postgres it will 💥

And, as the PHP manual too states:

addslashes() must not be used with PostgreSQL.

The logic of that method simply does nothing useful in the world of postgres (it's all in the PRs description).

[Miguel-Serejo]

Are there cases where you might want to keep an unescaped single quote or where a single quote is already escaped in the input string?

[mfn]

where a single quote is already escaped in the input string?

Only if you it manuall…? 🤷‍♀️

An unescaped single quote in a single-quoted string in a pgsql statement is an error.

Bueprints ->comment() is properly escaped with this PR, whereas before it breaks when single quotes are contained in a comment.

If you talk about having two literal single quotes: then yes, each of them gets escaped by another single quotes. There postgres rules, not mine 😄

Again, this PR fixes this PR which was recently introduced.

The unit it tests in the Laravel framework don't have integration directly but I verified locally that before this PR it breaks with an SQL error, afterwards it works.

[Miguel-Serejo]

Thanks for clarifying. I don't have much experience with postgres but have been looking into it.

So, before this PR, if a user tries to manually escape their quotes, the code just fails. With this PR, their quotes get doubled. I guess you wouldn't be expected to escape the quotes yourself so that's probably fine?

Are double-quoted identifiers allowed in comments, and if so, would single quotes need to be escaped in them? For example, if my comment includes the identifier "test o'clock", this would turn that into "test o''clock". Would that still be fine?

[mfn]

So, before this PR, if a user tries to manually escape their quotes, the code just fails. With this PR, their quotes get doubled. I guess you wouldn't be expected to escape the quotes yourself so that's probably fine?

Exactly!

Are double-quoted identifiers allowed in comments, and if so, would single quotes need to be escaped in them? For example, if my comment includes the identifier "test o'clock", this would turn that into "test o''clock". Would that still be fine?

Please make yourself familiar with postgres first, double quotes are only applicable to "identifiers" in postgres. Strings can be only quoted with single quotes. There's also an extended form of quoting using E'…' but this IMHO isn't relevant here.

This is usually never an issue with any code per se, as everything is using prepared statements/bound parameters.
But this is the Blueprints, different context.

[Miguel-Serejo]

double quotes are only applicable to "identifiers" in postgres.

I am aware of that, I'm asking if identifiers are allowed as a part of comments.
For example, adapting an example from the psql documentation:

COMMENT ON TABLE mytable IS 'This is my table and the column "test o'clock" is really important.';

And now that I've actually typed that out, I realize it's invalid as the single quote in the identifier would have to be escaped, which this PR would do.

Thanks for the additional clarification and sorry if it sounded like I was trying to question the validity of this PR, I just wanted to understand this properly.

[mfn]

Thanks for the additional clarification and sorry if it sounded like I was trying to question the validity of this PR, I just wanted to understand this properly.

It sounded that way, but I should not have assumed it! Apologies for that and thanks for the feedback!