change collation in query to adapt to charset #337

danrot · 2017-01-27T09:55:42Z

This change is related to doctrine/DoctrinePHPCRBundle#270. If a connection to the RepositorySchema with a charset of utf8mb4 is passed it is not allowed to use utf8_bin in a select statement.

I simply changed it in here to utf8mb4_bin, which will work then. However, I know that this can't be the final solution. The question is how that one should look like. We could use the connection to get the charset and use an if in here, or use the charset and simply postfix it with _bin, maybe check before if this collation really exists (is that somehow possible? What should be done in case it isn't?).

dbu · 2017-01-27T10:27:34Z

where can do you configure the encoding? would it be possible to have that place also configure a binary encoding to go with that, which by default is just $encoding.'_bin' but can be customized, then read that value in the client for that query?

or alternatively, have the client check if the driver is on utf8 and if not not use any collate in the query? from what i understand, collate is only used as a performance improvement. or is it also about being case sensitive or a vs ä?

danrot · 2017-01-27T10:41:21Z

I've been looking for the commit in which this change is, and I've found it: 5273f18

Looks like it is about case-sensitivity. So I guess it it is not possible to simply remove it in case of UTF-8, right? Seems there was a problem at some point.

The current configuration is from the DoctrineBundle, so not really related to Jackalope. If we want to introduce a new configuration we would have to find a place for it first.

I have just checked the available collations on my system, and there is a *_bin-collation for every character set, with binary being the only exception. So should we maybe take the "adding _bin" option and just check for the binary character set?

dbu · 2017-01-27T11:19:38Z

i guess that is about as save as the current code using hardcoded utf8_bin. maybe we can add a `setCaseSensitiveEncoding method on the client class, as a last resort for people that have a special case with some requirements? something like:

private $caseSensitiveEncoding;
private function getCaseSensitiveEncoding()
{
 if (!$this->caseSensitiveEncoding) {
   $this->caseSensitiveEncoding = get driver collation .'_bin';
 }

 return $this->caseSensitiveEncoding;
}

(and only call this in the mysql case.)

danrot · 2017-01-27T12:07:44Z

@dbu But that's only useful if it is at least protected, right? If we make it private nobody can change it, so there would be no reason to introduce that. Or am I overseeing something now? 🙈

danrot · 2017-01-27T12:46:38Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

-            if ($this->getConnection()->getDriver() instanceof \Doctrine\DBAL\Driver\PDOMySql\Driver) {
-                $query = 'SELECT id FROM phpcr_nodes WHERE path COLLATE utf8_bin = ? AND workspace_name = ?';
+            $params = $this->conn->getParams();
+            $charset = isset($params['charset']) ? $params['charset'] : 'utf8';


There's a problem with that solution... The charset option and the default charset with which a table is created can be different... How should we handle that? Couldn't find another way to get that from the SchemaManager in Doctrine for the specific table 😕

Just to be clear, I am talking about this situation in the config:

doctrine: dbal: charset: utf8mb4 default_table_options: charset: utf8 collate: utf8_unicode_ci

oh well. but in that case lets expect people to set the case sensitive encoding manually?

i think we really must avoid doing a database call in every request to know this, so it must come from the connection / driver if thats possible at all. worst case we just add this new method and have a compiler pass in the phcpr-bundle that reads the dbal configuration and does something sensible?

@lsmith77 can you help us here on where to get the right information?

hmm would need to dig in here too .. but I agree we should do as little work as possible at run time ..
so what we can do at install time or at config compile time the better.

Just to be clear: The current state doesn't involve any database call at all, or am I missing something?

no, currently not. i was just saying that we must not end with something that does.

And you are also saying that the current solution is not satisfying, right? Would it be enough to add the setCaseSensitiveEncoding method?

yes, i would say so. have a setter, and a get method that does the code you added above and below, so that this code can just say if mysql and not binary, do collate with $this->getCaseSensitiveEncoding()

dbu · 2017-01-27T16:34:28Z

@dbu <https://github.com/dbu> But that's only useful if it is at least protected, right? If we make it private nobody can change it, so there would be no reason to introduce that. Or am I overseeing something now? 🙈

i missed to write the setter in my illustration... i would have a /** * Set table encoding to use for case sensitive lookups in MySQL. */ public function setCaseSensitiveEncoding($encoding)

danrot · 2017-01-30T11:03:21Z

@dbu Added these methods, is it ok now?

dbu

cool! added a few comments, otherwise looks good.

can you please also add an entry to the changelog to mention this fix and the method in case somebody runs into problems?

dbu · 2017-01-30T12:34:30Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

+            $params = $this->conn->getParams();
+            $charset = isset($params['charset']) ? $params['charset'] : 'utf8';
+
+            return $charset === 'binary' ? $charset : $charset . '_bin';


please do $this->caseSensitiveEncoding = $charset === 'binary' ? $charset : $charset.'_bin'; instead of return, so its only done once

dbu · 2017-01-30T12:35:41Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

@@ -439,6 +444,23 @@ public function setCheckLoginOnServer($bool)
        $this->checkLoginOnServer = $bool;
    }

+    public function setCaseSensitiveEncoding($encoding)


please add a docblock explaining that this is to control what collate should be used when querying for nodes. and mention that its only relevant for mysql and usually the autodetection of appending _bin to the charset is good enough.

danrot · 2017-01-30T13:51:54Z

@dbu Fixed your comments.

dbu · 2017-01-30T15:34:47Z

CHANGELOG.md

@@ -1,6 +1,13 @@
 Changelog
 =========

+1.2.8


if we merge this change into master, it will become 1.3, as master is for 1.3 and there are other changes in it. do you urgently need a fixed release? vondrasekpavel is busy doing lots of cleanups and fixes, we would release sometimes in february.

for an urgent fix, i would have to update the 1.2 branch to have everything that is in 1.2 (it currently looks massively outdated: 1.2...1.2.8). if you need a fix urgently, i can do that and then merge your branch into 1.2 and tag 1.2.9 instead.

Ah, I thought master is the next hotfix release... Actually @alexander-schranz has to tell how fast he needs that fix, for me the next minor release is also good 😃

Have talked to @alexander-schranz, it's not that important. Changed it to 1.3

dbu · 2017-01-31T08:29:44Z

great, then it will go into 1.3. i guess we should release that soonish, quite a few fixes and improvements going on in the last weeks! thanks daniel!

danrot · 2017-01-31T10:10:36Z

Cool, thank you! Now only doctrine/DoctrinePHPCRBundle#270 is missing in order for it to work in a symfony environment :-)

dbu · 2017-01-31T10:16:44Z

thanks for the reminder, merged that as well.

danrot mentioned this pull request Jan 27, 2017

Ckeditor special character crash page sulu/sulu#3106

Closed

danrot changed the title ~~changed collate to utf8mb4~~ change collation in query to adapt to charset Jan 27, 2017

danrot commented Jan 27, 2017

View reviewed changes

dbu requested changes Jan 30, 2017

View reviewed changes

dbu approved these changes Jan 30, 2017

View reviewed changes

dbu reviewed Jan 30, 2017

View reviewed changes

change collation in query to adapt to charset

9b7a165

dbu merged commit ad1284c into jackalope:master Jan 31, 2017

danrot deleted the bugfix/utf8mb4 branch January 31, 2017 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change collation in query to adapt to charset #337

change collation in query to adapt to charset #337

danrot commented Jan 27, 2017

dbu commented Jan 27, 2017

danrot commented Jan 27, 2017

dbu commented Jan 27, 2017 •

edited

Loading

danrot commented Jan 27, 2017

danrot Jan 27, 2017 •

edited

Loading

dbu Jan 27, 2017 •

edited

Loading

lsmith77 Jan 27, 2017

danrot Jan 29, 2017

dbu Jan 29, 2017

danrot Jan 30, 2017

dbu Jan 30, 2017

dbu commented Jan 27, 2017 via email

danrot commented Jan 30, 2017

dbu left a comment

dbu Jan 30, 2017

dbu Jan 30, 2017

danrot commented Jan 30, 2017

dbu Jan 30, 2017

danrot Jan 31, 2017

danrot Jan 31, 2017

dbu commented Jan 31, 2017

danrot commented Jan 31, 2017

dbu commented Jan 31, 2017 via email

change collation in query to adapt to charset #337

change collation in query to adapt to charset #337

Conversation

danrot commented Jan 27, 2017

dbu commented Jan 27, 2017

danrot commented Jan 27, 2017

dbu commented Jan 27, 2017 • edited Loading

danrot commented Jan 27, 2017

danrot Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

dbu Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Jan 27, 2017 via email

danrot commented Jan 30, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Jan 31, 2017

danrot commented Jan 31, 2017

dbu commented Jan 31, 2017 via email

dbu commented Jan 27, 2017 •

edited

Loading

danrot Jan 27, 2017 •

edited

Loading

dbu Jan 27, 2017 •

edited

Loading