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

Allow %s_bin in SQL field definitions #1286

Closed
ghost opened this issue Jan 4, 2018 · 14 comments
Closed

Allow %s_bin in SQL field definitions #1286

ghost opened this issue Jan 4, 2018 · 14 comments
Assignees
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Jan 4, 2018

Comment by @fritzmg
January 4th, 2018, 09:29 GMT

Btw. why do all the alias fields use a hardcoded utf8mb4_bin collation now anyway? Even if you set

doctrine:
    dbal:
        connections:
            default:
                default_table_options:
                    charset: utf8
                    collate: utf8_unicode_ci

the collation of these fields would still be utf8mb4_bin.

@leofeyer
Copy link
Member

leofeyer commented Jan 4, 2018

Because those fields are not case-insensitive. But I agree that we should allow %s_bin and replace %s with the configured charset.

@leofeyer leofeyer added the bug label Jan 4, 2018
@leofeyer leofeyer added this to the 4.5.2 milestone Jan 4, 2018
@aschempp
Copy link
Member

aschempp commented Jan 5, 2018 via email

@leofeyer
Copy link
Member

leofeyer commented Jan 5, 2018

Really? How is this supposed to work?

@fritzmg
Copy link
Contributor

fritzmg commented Jan 5, 2018

Please use the SQL definition as array, that should probably automatically fix it ;-)

Even if you use the array, you'd still have to hard code the collation, wouldn't you?

@leofeyer what about using

varchar(128) BINARY NOT NULL default ''

instead of

varchar(128) COLLATE utf8mb4_bin NOT NULL default ''

@aschempp
Copy link
Member

aschempp commented Jan 5, 2018

Really? How is this supposed to work?

http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/schema-representation.html#column

'sql' => ['type' => 'binary_string', 'length' => 16, 'fixed' => true]

@leofeyer
Copy link
Member

leofeyer commented Jan 5, 2018

Nope, the schema update tries to generate a varbinary() field then instead of a varchar() binary field.

@leofeyer
Copy link
Member

leofeyer commented Jan 5, 2018

@fritzmg's suggestion varchar(128) BINARY works, however the schema tool does not explicitly handle this case it seems.

@aschempp
Copy link
Member

aschempp commented Jan 7, 2018

Is there a difference between the two? According to Stackoverflow I would say we actually do want varbinary?

@leofeyer
Copy link
Member

leofeyer commented Jan 7, 2018

No, according to exactly the post you have linked to, we do not want varbinary. We want varchar(255) binary, which

causes the binary collation for the column character set to be used

@leofeyer
Copy link
Member

leofeyer commented Jan 7, 2018

Why you should not map varchar() binary to varbinary(): https://bugs.mysql.com/bug.php?id=16668

@leofeyer leofeyer self-assigned this Jan 7, 2018
@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2018

Since we cannot make Doctrine handle the BINARY flag, there are two possible solutions:

  1. Continue to use COLLATE utf8mb4_bin, which is potentially incompatible if someone changes the DB charset. (They would have to adjust the DCA files, too.)

  2. Allow to use the BINARY flag in DCA files and handle it in the DcaSchemaProvider. This works fine for DCA files but not for ORM entities. (So it would be a new known limitation.)

@contao/developers What is your favorite solution?

@aschempp
Copy link
Member

aschempp commented Jan 9, 2018

But we are not storing a binary string, we are storing binary data, are we not? According to that link:

VARCHAR BINARY
When BINARY values are stored, they are right-padded with the pad value to the specified length.

vs.

VARBINARY
There is no padding on insert, and no bytes are stripped on select. All bytes are significant in comparisons.

VARBINARY sounds more reasonable for me?

@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2018

We are storing a binary string (e.g. tl_page.alias). We are also storing binary data (e.g. tl_content.singleSRC) but that has nothing to do with this issue.

@leofeyer
Copy link
Member

See #1300.

leofeyer added a commit that referenced this issue Jan 11, 2018
Description
-----------

This PR fixes #1286 as discussed with @aschempp. It handles the following cases:

* `'sql' => "varchar(128) BINARY NOT NULL default ''"`
* `'sql' => ['type' => 'string', 'length' => 128, 'not_null' => false, 'default' => '', 'customSchemaOptions' => ['case_sensitive' => true]]`

Commits
-------

3a02e11 Use the `BINARY` flag instead of `COLLATE utf8mb4_bin` (see #1286).
8de2862 Throw an exception if both a collation and the binary flag are set.
9fb1db5 Revert the exception changes.
@leofeyer leofeyer modified the milestones: 4.5.2, 4.5 May 14, 2019
leofeyer added a commit that referenced this issue Feb 7, 2020
Description
-----------

Fixes #1224

Commits
-------

15d5ba33 Fix the front end preview URLs in the back end (see #1224)
7dfad9c2 Add a unit test for the missing case (page and alias given)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants