Drupal's core schema coming with NULL NULL text fields using '' as default value. This is a critical problem for cross database implementation, which Moodle XMLDB also face before (http://docs.moodle.org/en/Development:XMLDB_problems#NOT_NULL_fields_usi...):
Under Moodle 1.6 there are tons of char/varchar/text/blob columns that are defined as NOT NULL and contains one DEFAULT clause with the value '' (empty string).
There are two important problems with this type of definitions:
1. They haven't to much sense from a design perspective. If one field is defined as NOT NULL it shouldn't contain empty values (yep, agree that NULL != everything, included empty, but in a logical world I cannot find any reason to fill fields with empty values).
2. Oracle considers empty strings as NULL values the DEFAULT application will crash in that RDBMS. (http://www.techonthenet.com/oracle/questions/empty_null.php)The proper way to solve this situation should be to annihilate all those inconsistent/wrong combinations from the whole DB schema (NOT NULL + DEFAULT '')...
This issue hope to standardize Drupal's CHAR, VARCHAR, TEXT and BLOB value as: 1. valid value, or 2. NULL only.
Patch attached demonstrate how this logic can be implemented with. It pass "expert" profile install, and all simpletest relate to modules installed for "expert" profile. Schema update with update.php is pass, too.
Should we have some discussion for this idea, and also how to solve this hidden problem?
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | nullable-schema-1227760399.patch | 122.28 KB | hswong3i |
| #10 | nullable-username-userpass-1227765376.patch | 4.39 KB | hswong3i |
| #10 | nullable-language-1227765197.patch | 20.28 KB | hswong3i |
| #10 | nullable-actions-parameters-1227764119.patch | 4.2 KB | hswong3i |
| #7 | nullable-schema-1227546071.patch | 122.25 KB | hswong3i |
Comments
Comment #1
hswong3i commentedPath module need some more consideration, since language is now using '' (empty string) as undefined default value. This issue is splitting into #336293: Use 'default' instead of '' as undefined default language for independent handling.
For patch attached here, it is integrate with #336293: Use 'default' instead of '' as undefined default language and so pass all simpletest test case with MySQL.
Comment #2
hswong3i commentedAdd missing node module upgrade function and 2 missing fields. Some other minor fix, too.
For review 2 additional files are attached (split version):
Comment #3
chx commentedIf Oracle can't make a difference between NULL and empty strings, bad for Oracle but this won't fly, you will never get contrib authors to make IS NULL instead of = '' and I told you that there is a reason why NULL and the empty strings are different things. Let the Oracle driver alter the schema, fix INSERTs and str_replace the queries as necessary but dont try to force this on everyone else.
Comment #4
chx commentedHere is the full chat transcript we had about this:
[Sat Nov 15 2008] [21:01:59] [hswong3i] hi chx, sorry for pm but i would like to have your idea before coming on development
[Sat Nov 15 2008] [21:02:09] [hswong3i] may i have your review for this issue? http://drupal.org/node/334687
[Sat Nov 15 2008] [21:02:36] [hswong3i] i hope to standardize text handling as valid value and NULL only
[Sat Nov 15 2008] [21:03:15] [chx] empty strings are not NULL
[Sat Nov 15 2008] [21:03:59] [hswong3i] ypuyup i understand
[Sat Nov 15 2008] [21:04:08] [hswong3i] but as like as moodle gay idea
[Sat Nov 15 2008] [21:04:14] [chx] we already took care to have no DEFAULT set at all for TEXT and BLOB
[Sat Nov 15 2008] [21:04:21] [hswong3i] it is not too useful and make a little bit confluse
[Sat Nov 15 2008] [21:04:48] [hswong3i] so i would like to extend this for char and varchar too
[Sat Nov 15 2008] [21:04:58] [hswong3i] and try not to use ''
[Sat Nov 15 2008] [21:05:37] [chx] you will meet considerable resistance
[Sat Nov 15 2008] [21:05:57] [hswong3i] i think so too so i would like to have idea before pushing it
[Sat Nov 15 2008] [21:06:03] [chx] not to mention that this is by far not enough
[Sat Nov 15 2008] [21:06:16] [chx] what you will do when a user supplied field is an empty string?
[Sat Nov 15 2008] [21:06:34] [chx] are you going to carefully change all those to NULL
[Sat Nov 15 2008] [21:06:37] [hswong3i] the driver will translate it as NULL internally
[Sat Nov 15 2008] [21:06:50] [chx] and then how do you make a difference between "not entered" and "empty"?
[Sat Nov 15 2008] [21:07:00] [chx] because NULL has a meaning and a role
[Sat Nov 15 2008] [21:07:24] [hswong3i] Hmm... this is truth, but for daily usage, we don't really use NULL much
[Sat Nov 15 2008] [21:07:34] [chx] core does not
[Sat Nov 15 2008] [21:07:34] [hswong3i] the patch attached also show this
[Sat Nov 15 2008] [21:07:46] [hswong3i] we only need to change very few lines for working
[Sat Nov 15 2008] [21:07:59] [chx] also, NULLable tables are IMO slower in MySQL
[Sat Nov 15 2008] [21:08:12] [hswong3i] !? isn't it?
[Sat Nov 15 2008] [21:08:20] [chx] i cant see this patch flying
[Sat Nov 15 2008] [21:08:42] [hswong3i] i need to agree that it is originally about oracle
[Sat Nov 15 2008] [21:09:08] [hswong3i] but having text "status" only with: 1. valid value, or else 2. NULL
[Sat Nov 15 2008] [21:09:12] [hswong3i] seems more simple
[Sat Nov 15 2008] [21:09:28] [chx] you need to study how you can avoid bending core to your own needs
[Sat Nov 15 2008] [21:09:33] [chx] look at the SQLite patch
[Sat Nov 15 2008] [21:09:42] [chx] we suffered a lot but the core changes are absolutely minimal.
[Sat Nov 15 2008] [21:10:01] [hswong3i] wait let me check...
[Sat Nov 15 2008] [21:10:02] [chx] actually, we only had real bugfixes.
[Sat Nov 15 2008] [21:10:07] [hswong3i] i means check my patch...
[Sat Nov 15 2008] [21:10:42] [chx] also
[Sat Nov 15 2008] [21:10:47] [chx] there is a problem with contrib, see
[Sat Nov 15 2008] [21:10:55] [chx] just because you change core queries to this effect
[Sat Nov 15 2008] [21:11:02] [chx] contrib wont follow just because
[Sat Nov 15 2008] [21:11:28] [chx] so your efforts are in vain
[Sat Nov 15 2008] [21:12:05] [hswong3i] for making core working with nullable only text, we only need to change 3 file additionally: http://rafb.net/p/em9sr785.html
[Sat Nov 15 2008] [21:12:29] [hswong3i] before i really code for this patch
[Sat Nov 15 2008] [21:12:36] [hswong3i] i guess we need to change core much too
[Sat Nov 15 2008] [21:12:51] [hswong3i] but after work it out, it is not too complicated and unacceptable
[Sat Nov 15 2008] [21:12:56] [hswong3i] that i never think before
[Sat Nov 15 2008] [21:13:34] [chx] and how will you get contrib to follow.
[Sat Nov 15 2008] [21:13:36] [hswong3i] only very few inline SQL = '' need to convert as IS NULL
[Sat Nov 15 2008] [21:13:50] [hswong3i] change inline SQL = '' into IS NULL
[Sat Nov 15 2008] [21:13:57] [hswong3i] and that's almost done
[Sat Nov 15 2008] [21:14:05] [chx] str_replace in Oracle driver that
[Sat Nov 15 2008] [21:14:14] [hswong3i] DBTNG and schemaAPI can handle the rest
[Sat Nov 15 2008] [21:14:19] [chx] and change the schema in the Oracle driver as well.
[Sat Nov 15 2008] [21:14:27] [hswong3i] i try that too for a year
[Sat Nov 15 2008] [21:14:34] [hswong3i] i think this work too
[Sat Nov 15 2008] [21:14:36] [chx] dont touch core, it's a) not nice b) wont fly 'cos of contrib
[Sat Nov 15 2008] [21:15:14] [hswong3i] Hmmm... i would like to tell you some background information for oracle dev
[Sat Nov 15 2008] [21:15:16] [hswong3i] may i?
[Sat Nov 15 2008] [21:15:19] [chx] i think you should really study how we did the SQLite patch -- fix core bugs (real bugs like a merge query in aggregator had no keys) -- abstract DBTNG enough -- submit driver.
[Sat Nov 15 2008] [21:16:05] [hswong3i] i try this too
[Sat Nov 15 2008] [21:16:12] [hswong3i] but it is not work as we are now using PDO
[Sat Nov 15 2008] [21:16:28] [hswong3i] for my old implementation, i can restrict this within oracle driver only
[Sat Nov 15 2008] [21:16:39] [hswong3i] but for now PDO don't give any hook for me...
[Sat Nov 15 2008] [21:16:45] [chx] hook?
[Sat Nov 15 2008] [21:17:02] [chx] where do you need to step in and you cant with PDO?
[Sat Nov 15 2008] [21:17:16] [hswong3i] the foreach loop
[Sat Nov 15 2008] [21:17:31] [chx] sure you can
[Sat Nov 15 2008] [21:17:45] [hswong3i] how?
[Sat Nov 15 2008] [21:18:09] [chx] well, i have two ideas both implementing your DatabaseStatement
[Sat Nov 15 2008] [21:18:10] [hswong3i] i guess that is STL (is that this name) function
[Sat Nov 15 2008] [21:18:22] [chx] one, extend PDOStatement and implement the iterator functions
[Sat Nov 15 2008] [21:18:25] [chx] it either lets you or not
[Sat Nov 15 2008] [21:18:27] [chx] two
[Sat Nov 15 2008] [21:18:34] [chx] i told you to study the SQLite process
[Sat Nov 15 2008] [21:18:41] [chx] we created prefetch.inc
[Sat Nov 15 2008] [21:18:44] [chx] it's already in core
[Sat Nov 15 2008] [21:18:52] [chx] look at that
[Sat Nov 15 2008] [21:19:25] [hswong3i] prefetch.inc? from your patch or already in core?
[Sat Nov 15 2008] [21:20:07] [hswong3i] anyway i will check it :)
[Sat Nov 15 2008] [21:20:11] [hswong3i] and study it
[Sat Nov 15 2008] [21:20:28] [chx] already in core
[Sat Nov 15 2008] [21:20:40] [chx] it gives you absolute control over everything , beyond imagination
[Sat Nov 15 2008] [21:21:57] [hswong3i] but i can't find it from CVS HEAD checkout :S
[Sat Nov 15 2008] [21:22:15] [hswong3i] but i get it from your SQLite patch
[Sat Nov 15 2008] [21:22:15] [hswong3i] now reading
[Sat Nov 15 2008] [21:23:03] [chx] it should be in already
[Sat Nov 15 2008] [21:23:09] [hswong3i] Hmm... just one more minor question: may i have your idea about the wordings of moodle gay?
[Sat Nov 15 2008] [21:23:17] [chx] moodle guy?
[Sat Nov 15 2008] [21:23:18] [hswong3i] They haven't to much sense from a design perspective. If one field is defined as NOT NULL it shouldn't contain empty values (yep, agree that NULL != everything, included empty, but in a logical world I cannot find any reason to fill fields with empty values).
[Sat Nov 15 2008] [21:23:31] [chx] hmmm really prefetch.inc not yet in
[Sat Nov 15 2008] [21:23:41] [chx] yeah, only the underlining interface is.
[Sat Nov 15 2008] [21:23:41] [chx] fine.
[Sat Nov 15 2008] [21:23:51] [hswong3i] nvm about prefetch.inc, i am now reading it from your patch
[Sat Nov 15 2008] [21:23:55] [chx] yeah
[Sat Nov 15 2008] [21:24:15] [chx] friendly advice: learning more English would make your life easier
[Sat Nov 15 2008] [21:24:30] [chx] sometimes it's extremely hard to find out what do you want
[Sat Nov 15 2008] [21:24:30] [hswong3i] ..... i think so tooo :S
[Sat Nov 15 2008] [21:24:33] [chx] I know it's hard
[Sat Nov 15 2008] [21:24:42] [chx] I spent like 15 years getting to this level
[Sat Nov 15 2008] [21:25:12] [hswong3i] i will take a course for english soon :(
[Sat Nov 15 2008] [21:25:17] [hswong3i] i really need it
[Sat Nov 15 2008] [21:25:45] [hswong3i] anyway, any idea about above wordings?
[Sat Nov 15 2008] [21:25:49] [hswong3i] They haven't to much sense from a design perspective. If one field is defined as NOT NULL it shouldn't contain empty values (yep, agree that NULL != everything, included empty, but in a logical world I cannot find any reason to fill fields with empty values).
[Sat Nov 15 2008] [21:26:00] [chx] thats nonsense
[Sat Nov 15 2008] [21:26:00] [hswong3i] i think they are not totally wrong at all
[Sat Nov 15 2008] [21:26:02] [chx] simply.
[Sat Nov 15 2008] [21:26:10] [hswong3i] err...
[Sat Nov 15 2008] [21:26:18] [chx] i told you above already that NULL and empty string are two rather different things
[Sat Nov 15 2008] [21:26:59] [chx] if you have a multi page form situation where the pages of the form can be filled in through a long time
[Sat Nov 15 2008] [21:27:14] [chx] then entering NULL into the fields which need answering is a rather valid use case
[Sat Nov 15 2008] [21:27:24] [chx] empty string is a valid user input in this case
[Sat Nov 15 2008] [21:27:26] [chx] and so is NULL
[Sat Nov 15 2008] [21:27:44] [chx] if you force my hand to enter some invalid user input in those cases then i will yell Year 2000 :)
[Sat Nov 15 2008] [21:28:30] [hswong3i] a question: how can formAPI classify NULL and empty string user input?
[Sat Nov 15 2008] [21:28:59] [chx] it of course can't but re-read what i said above
[Sat Nov 15 2008] [21:29:13] [chx] imagine you have page 1, 2 and 3
[Sat Nov 15 2008] [21:29:21] [chx] the user can any time navigate away and continue later
[Sat Nov 15 2008] [21:29:45] [chx] the information from all three pages are entered into one database row
[Sat Nov 15 2008] [21:29:52] [hswong3i] i get your idea too
[Sat Nov 15 2008] [21:29:54] [chx] now, what should i do after page 1 is filled in?
[Sat Nov 15 2008] [21:30:01] [hswong3i] but when user move from page 1 to page 2
[Sat Nov 15 2008] [21:30:06] [chx] i want to fill in NULLs for page 2 and 3 values
[Sat Nov 15 2008] [21:30:38] [chx] so that when the user revisits the for I can immediately see, ah yes, page 1 fields are filled in , page 2 and page 3 not yet
[Sat Nov 15 2008] [21:31:22] [hswong3i] ok i get your idea
[Sat Nov 15 2008] [21:31:29] [hswong3i] hope to have more idea about formapi
[Sat Nov 15 2008] [21:31:40] [hswong3i] so how we handle multi page form right now?
[Sat Nov 15 2008] [21:31:45] [hswong3i] i have not much idea about that
[Sat Nov 15 2008] [21:32:06] [chx] this has nothing to do with form API
[Sat Nov 15 2008] [21:32:12] [chx] page 2 is simply not submitted
[Sat Nov 15 2008] [21:32:33] [hswong3i] so why we need to care about NULL for page 2 and 3?
[Sat Nov 15 2008] [21:32:39] [chx] so when I save the database row that will eventually contain data from three pages but for now I only have page 1, i fill in NULLs for the rest.
[Sat Nov 15 2008] [21:32:44] [hswong3i] i have some confluse...
[Sat Nov 15 2008] [21:32:49] [chx] you go to page 2
[Sat Nov 15 2008] [21:32:51] [chx] you go to page 1
[Sat Nov 15 2008] [21:32:52] [chx] sorry
[Sat Nov 15 2008] [21:32:53] [chx] you go to page 1
[Sat Nov 15 2008] [21:32:55] [chx] you submit page 1
[Sat Nov 15 2008] [21:33:02] [chx] now you need to save these fields
[Sat Nov 15 2008] [21:33:10] [chx] because , the user might go away and might come back later
[Sat Nov 15 2008] [21:33:23] [chx] Now, I save the database row that will eventually contain data from three pages but for now I only have page 1, so i fill in NULLs for the rest.
[Sat Nov 15 2008] [21:33:36] [chx] filling in empty is not adequate. it's not empty. it's NULL.
[Sat Nov 15 2008] [21:33:48] [chx] the user did not yet submit page 2 or 3
[Sat Nov 15 2008] [21:33:53] [chx] they are nonexisting values
[Sat Nov 15 2008] [21:33:56] [chx] which is NULL
[Sat Nov 15 2008] [21:34:15] [hswong3i] ok i get it
[Sat Nov 15 2008] [21:34:28] [hswong3i] so you would like to have page control tough NULL value
[Sat Nov 15 2008] [21:34:37] [hswong3i] if page not yet submit
[Sat Nov 15 2008] [21:34:40] [hswong3i] we fill in NULL
[Sat Nov 15 2008] [21:34:45] [chx] it's data
[Sat Nov 15 2008] [21:34:46] [hswong3i] so when user come back
[Sat Nov 15 2008] [21:34:48] [chx] yeah
[Sat Nov 15 2008] [21:34:57] [chx] also dont forget: this is a simplified example
[Sat Nov 15 2008] [21:35:11] [chx] it very well can be that such information is collected throughout the site
[Sat Nov 15 2008] [21:35:12] [hswong3i] we get that "ah! those value are missing, so he should go to page 2/3!!"
[Sat Nov 15 2008] [21:35:18] [chx] so that if i see NULL then I put out the field
[Sat Nov 15 2008] [21:35:24] [chx] soemwhere
[Sat Nov 15 2008] [21:35:30] [chx] but if I see empty then i dont
[Sat Nov 15 2008] [21:35:34] [chx] yeah
[Sat Nov 15 2008] [21:35:40] [chx] in the simple example, we do that
[Sat Nov 15 2008] [21:36:11] [chx] but as i said, in real life ( i am quoting a real, damned complicated social network app here) the user information might be collected through a lot of forms and a lot of pages
[Sat Nov 15 2008] [21:36:29] [chx] and then it's quite crucial not to display fields where the user already entered empty.
[Sat Nov 15 2008] [21:37:05] [hswong3i] yes i totally agree with your idea
[Sat Nov 15 2008] [21:37:15] [hswong3i] and that's why i don't propose this patch before
[Sat Nov 15 2008] [21:37:27] [hswong3i] as i hope to classify '' null and value value too
[Sat Nov 15 2008] [21:37:33] [hswong3i] i think it is very useful
[Sat Nov 15 2008] [21:37:57] [hswong3i] Hmmm... some other idea
[Sat Nov 15 2008] [21:38:22] [hswong3i] i think the most resistance for this patch is: 1. it change core, and 2 it change contribute module
[Sat Nov 15 2008] [21:38:27] [hswong3i] isn't it?
[Sat Nov 15 2008] [21:39:20] [chx] it does something fundamentally wrong by mashing NUL and empty string together
[Sat Nov 15 2008] [21:39:32] [chx] and while core is always changeable , contrib wont follow
[Sat Nov 15 2008] [21:39:41] [chx] people will still do foo = '' in their sql
[Sat Nov 15 2008] [21:40:13] [hswong3i] i think if we really change '' into NULL, we need to document the change for = '' too
[Sat Nov 15 2008] [21:40:30] [chx] this is why you db_strlen patch is kinda broken -- who will use that? MySQL coders will use STRLEN. You need to have that in the query builder or you are doomed.
[Sat Nov 15 2008] [21:40:48] [chx] contrib people only do things they are kinda forced to do
[Sat Nov 15 2008] [21:41:01] [chx] if something mysql specific works that's what they will use
[Sat Nov 15 2008] [21:41:03] [hswong3i] so if they do so
[Sat Nov 15 2008] [21:41:09] [hswong3i] i will let it be
[Sat Nov 15 2008] [21:41:22] [hswong3i] and whenever we need that module working with oracle
[Sat Nov 15 2008] [21:41:38] [hswong3i] ppl will understand how to revamp it as functioning
[Sat Nov 15 2008] [21:41:49] [hswong3i] e.g. even we have db_query_range
[Sat Nov 15 2008] [21:42:05] [hswong3i] if ppl only use LIMIT directly, code will still not funtion for pgsql
[Sat Nov 15 2008] [21:42:11] [hswong3i] it is just the same idea
[Sat Nov 15 2008] [21:42:20] [chx] you do not get the whole point of this show
[Sat Nov 15 2008] [21:42:49] [chx] we want to provide nice functionality which people will use instead of MySQLisms
[Sat Nov 15 2008] [21:43:14] [chx] the alterable , described , easy to write schema driven everyone out from writing CREATE TABLE statements by hand
[Sat Nov 15 2008] [21:43:57] [chx] similarly, we will have a nice query builder that lets you do more so people will use that
[Sat Nov 15 2008] [21:44:22] [chx] there needs to be advantages for every Drupalism compared to what one would write based on "PHP for dummies"
[Sat Nov 15 2008] [21:44:27] [chx] and there always are
[Sat Nov 15 2008] [21:44:40] [chx] the advantage must be something that people care of
[Sat Nov 15 2008] [21:45:04] [chx] you cant possibly expect people to care of pgsql and oracle or even sqlite
[Sat Nov 15 2008] [21:45:09] [chx] that's the wrong way to go
[Sat Nov 15 2008] [21:47:19] [hswong3i] so we should standardize something, abstract something, also with documentation, and so simplify users life
[Sat Nov 15 2008] [21:48:16] [chx] yeah
[Sat Nov 15 2008] [21:48:24] [chx] people will use what's convinient to use
[Sat Nov 15 2008] [21:48:35] [hswong3i] ok i will always keep this in mind :)
[Sat Nov 15 2008] [21:49:07] [hswong3i] anyway, just some sharing
[Sat Nov 15 2008] [21:49:17] [hswong3i] i will give some more effort for the nullable patch
[Sat Nov 15 2008] [21:49:23] [hswong3i] and see how it should look for
[Sat Nov 15 2008] [21:49:37] [hswong3i] something that we can never sure before we did it
[Sat Nov 15 2008] [21:49:43] [hswong3i] i can't even sure too
[Sat Nov 15 2008] [21:49:50] [hswong3i] maybe it need to change core much
[Sat Nov 15 2008] [21:49:54] [hswong3i] maybe it don't
[Sat Nov 15 2008] [21:50:00] [hswong3i] at least it will give a try for that
[Sat Nov 15 2008] [21:50:12] [hswong3i] e.g. at least pass all simpletest
[Sat Nov 15 2008] [21:50:28] [chx] well if oracle does not support empty and null as two different things then oracle is seriously broken.
[Sat Nov 15 2008] [21:50:33] [hswong3i] and so let's review again at that moment?
[Sat Nov 15 2008] [21:50:53] [hswong3i] we all say "oracle is buggy" for null and empty string
[Sat Nov 15 2008] [21:51:02] [chx] i do not have the ansi standard at hand
[Sat Nov 15 2008] [21:51:04] [hswong3i] every oracle gay agree for that
[Sat Nov 15 2008] [21:51:07] [chx] but i seriously doubt that's ANSI
[Sat Nov 15 2008] [21:51:10] [chx] guy!
[Sat Nov 15 2008] [21:51:16] [hswong3i] that guy...
[Sat Nov 15 2008] [21:51:19] [hswong3i] sorry...
[Sat Nov 15 2008] [21:51:28] [hswong3i] Oops...
[Sat Nov 15 2008] [21:51:31] [chx] listen, gay means someone who loves his own sex
[Sat Nov 15 2008] [21:51:40] [chx] like a male a male or a woman another woman
[Sat Nov 15 2008] [21:51:46] [hswong3i] ... i mix that...
[Sat Nov 15 2008] [21:51:51] [hswong3i] sorry ...
[Sat Nov 15 2008] [21:51:57] [chx] nothing is wrong being gay, mind you
[Sat Nov 15 2008] [21:52:06] [hswong3i] ;S
[Sat Nov 15 2008] [21:52:08] [chx] but throwing that word in place of guy is at least strange
[Sat Nov 15 2008] [21:52:50] [hswong3i] ok again: oracle NULL handling is buggy, it is not ansi standard
[Sat Nov 15 2008] [21:53:04] [hswong3i] because they work this out even before ansi 92
[Sat Nov 15 2008] [21:53:14] [hswong3i] i guess, around 80's
[Sat Nov 15 2008] [21:53:23] [chx] then no matter what you do, this will be broken
[Sat Nov 15 2008] [21:53:43] [hswong3i] oracle also document that it should be change, since 8i
[Sat Nov 15 2008] [21:53:57] [hswong3i] but even now, 11g, they did nothing
[Sat Nov 15 2008] [21:53:58] [chx] it should be changed , how?
[Sat Nov 15 2008] [21:53:59] [chx] ah
[Sat Nov 15 2008] [21:54:03] [chx] so they should change
[Sat Nov 15 2008] [21:54:14] [hswong3i] but before they really change
[Sat Nov 15 2008] [21:54:19] [hswong3i] we can do nothing for that
[Sat Nov 15 2008] [21:54:25] [chx] well, how can they expect every Drupal developer to work around an Oracle bug?
[Sat Nov 15 2008] [21:55:04] [hswong3i] ooo that;s why my patch works: it don't change every drupal developers daily life
[Sat Nov 15 2008] [21:55:14] [hswong3i] only need to change some inline SQL checking
[Sat Nov 15 2008] [21:55:23] [hswong3i] and if they don't change it for oracle
[Sat Nov 15 2008] [21:55:30] [hswong3i] it just don't working for oracle
[Sat Nov 15 2008] [21:56:02] [hswong3i] Hmmm... i still need some more time to prove this idea
[Sat Nov 15 2008] [21:56:16] [hswong3i] at least i will give some more try for that
[Sat Nov 15 2008] [21:56:25] [hswong3i] i don't know the result yet...
[Sat Nov 15 2008] [21:57:32] [hswong3i] i would like to have your comment once again, when the patch is ready for production
[Sat Nov 15 2008] [21:57:39] [hswong3i] anyway, thanks for discuss with me
[Sat Nov 15 2008] [21:57:47] [hswong3i] i love this discussion :)
[Sat Nov 15 2008] [21:58:39] [chx] again
[Sat Nov 15 2008] [21:58:56] [chx] if foo = '' wont work (returns NULL rows) then stuff is broken
[Sat Nov 15 2008] [22:00:41] [hswong3i] i agree for this too, but sometime from point of view of oracle developer, i have no choice
[Sat Nov 15 2008] [22:00:54] [hswong3i] so i will give a try and see how it works
[Sat Nov 15 2008] [22:01:14] [hswong3i] this shouldn't be the best solution
[Sat Nov 15 2008] [22:01:26] [hswong3i] oracle should be the one who change
[Sat Nov 15 2008] [22:06:41] [chx] you always have a choice
[Sat Nov 15 2008] [22:07:00] [chx] you can always try to drive empty strings to the query builder and have fun with it
[Sat Nov 15 2008] [22:07:11] [chx] but what is going to happen when you have foo = :something ?
[Sat Nov 15 2008] [22:07:25] [chx] and then :something gets an empty string
[Sat Nov 15 2008] [22:07:49] [hswong3i] you may can't believe that
[Sat Nov 15 2008] [22:08:00] [hswong3i] the patch don't break core simpletest
[Sat Nov 15 2008] [22:08:06] [hswong3i] i can't believe this too
[Sat Nov 15 2008] [22:08:16] [hswong3i] i guess we will ahve a lot of error message :)
[Sat Nov 15 2008] [22:08:19] [chx] select * from suppliers
[Sat Nov 15 2008] [22:08:19] [chx] where supplier_name = '';
[Sat Nov 15 2008] [22:08:19] [chx] When you run this statement, you'd expect to retrieve the row that you inserted above. But instead, this statement will not retrieve any records at all.
[Sat Nov 15 2008] [22:08:35] [chx] this will NEVER EVER work in Drupal!
[Sat Nov 15 2008] [22:08:38] [hswong3i] so for this SQL, we need to change as IS NULL
[Sat Nov 15 2008] [22:08:43] [chx] we insert empty strings all the empty
[Sat Nov 15 2008] [22:08:45] [chx] we insert empty strings all the time
[Sat Nov 15 2008] [22:08:47] [chx] sorry
[Sat Nov 15 2008] [22:09:05] [chx] so yeah, we need to change the empty strings to NULL in the insert builder
[Sat Nov 15 2008] [22:09:10] [hswong3i] so for my patch, this empty string will change as NULL, too
[Sat Nov 15 2008] [22:09:19] [hswong3i] yupyup ou get it
[Sat Nov 15 2008] [22:09:21] [hswong3i] you
[Sat Nov 15 2008] [22:09:28] [chx] so what you do is:
[Sat Nov 15 2008] [22:09:38] [chx] a) change schema in the Oracle to allow NULLs.
[Sat Nov 15 2008] [22:09:53] [chx] b) change in the INSERT query builder all empty strings to NULLs.
[Sat Nov 15 2008] [22:10:07] [chx] after that, if I am not mistaken, no core or contrib change is necessary.
[Sat Nov 15 2008] [22:10:24] [hswong3i] c) = '' need to revamp as IS NULL
[Sat Nov 15 2008] [22:10:29] [hswong3i] nothing else
[Sat Nov 15 2008] [22:11:20] [hswong3i] please have a look for
[Sat Nov 15 2008] [22:11:22] [hswong3i] http://rafb.net/p/em9sr785.html
[Sat Nov 15 2008] [22:11:29] [hswong3i] this is the change fore core
[Sat Nov 15 2008] [22:11:42] [hswong3i] in order to make it work for expert profile install
[Sat Nov 15 2008] [22:11:49] [hswong3i] the changes is not much
[Sat Nov 15 2008] [22:12:26] [chx] ah true
[Sat Nov 15 2008] [22:12:34] [chx] but just str_replace = '' to IS NULL in the Oracle driver
[Sat Nov 15 2008] [22:12:42] [chx] and in the query builder
[Sat Nov 15 2008] [22:12:48] [chx] dont touch core
[Sat Nov 15 2008] [22:12:55] [chx] because, as I said, contrib wont follow.
[Sat Nov 15 2008] [22:14:45] [hswong3i] well... i am a man who will work it out before conclusion. before reach the target, i will not judge if it is a good idea :D
[Sat Nov 15 2008] [22:14:58] [hswong3i] because i am not sure too
[Sat Nov 15 2008] [22:15:10] [chx] I am absolute sure contrib will not follow.
[Sat Nov 15 2008] [22:15:20] [chx] because of the variable case i mentioned
[Sat Nov 15 2008] [22:15:26] [hswong3i] i think we can teach them though documentation
[Sat Nov 15 2008] [22:15:37] [chx] when you run SELECT * FROM foo WHERE bar = :bar
[Sat Nov 15 2008] [22:15:40] [hswong3i] but again, i am not sure
[Sat Nov 15 2008] [22:15:43] [chx] and :bar happens to be the empty string
[Sat Nov 15 2008] [22:16:00] [chx] you want every time to get poor developers check for that or run through the query builder?
[Sat Nov 15 2008] [22:16:06] [chx] that wont happen.
[Sat Nov 15 2008] [22:16:11] [hswong3i] but it is now working within my patch
[Sat Nov 15 2008] [22:16:24] [hswong3i] even we don't change the WHERE bar = :bar
[Sat Nov 15 2008] [22:16:35] [hswong3i] all core simpletest are pass, too
[Sat Nov 15 2008] [22:16:41] [hswong3i] i don't even believe this too
[Sat Nov 15 2008] [22:16:45] [hswong3i] but this is the truth
[Sat Nov 15 2008] [22:17:16] [chx] on Oracle?
[Sat Nov 15 2008] [22:17:43] [hswong3i] i not yet check with oracle, but this is for sure funcitoning for oracle
[Sat Nov 15 2008] [22:17:49] [hswong3i] i check it with mysql simpletest
[Sat Nov 15 2008] [22:18:06] [hswong3i] i am now developing on top of mysql
[Sat Nov 15 2008] [22:18:34] [chx] that does not matter
[Sat Nov 15 2008] [22:18:48] [chx] you acn make a lot of changes and yet not break mysql
[Sat Nov 15 2008] [22:19:04] [chx] that does mean it's going to get in core just because of that :)
[Sat Nov 15 2008] [22:20:24] [hswong3i] i understand that, i need to cooperate with other
[Sat Nov 15 2008] [22:20:37] [hswong3i] that's why i ask for your comment
[Sat Nov 15 2008] [22:20:57] [hswong3i] you can always give me some good idea :D
[Sat Nov 15 2008] [22:21:02] [chx] it's not just cooperation, you need to submit patches that make life better for everyone else :)
[Sat Nov 15 2008] [22:21:16] [hswong3i] yes, i agree
[Sat Nov 15 2008] [22:21:31] [hswong3i] and this is my target too
[Sat Nov 15 2008] [22:22:11] [hswong3i] anyway, thanks for discussion, it is 2pm in HK and i will have a lunch break
[Sat Nov 15 2008] [22:22:21] [hswong3i] hope to give you some good news after that ;)
[Sat Nov 15 2008] [22:22:53] [hswong3i] brb~~
Comment #5
hswong3i commentedThe idea is totally correct, and I have completely research for this almost 8 months ago. You can check this pdo_oci driver and common shared file which implement for D6, and search for string
ORACLE_EMPTY_STRING_PLACEHOLDER. With this implementation:ORACLE_EMPTY_STRING_PLACEHOLDERwill replace as\x01which is a non-printable character.\x01insidedb_escape_string(). So we need everything define as'%s'.\x01will backward replace as '' (empty string) insidedb_fetch_*().For sure that we can redo this logic with PDO by implement Iterator (PHP) and DatabaseStatementInterface (Drupal CVS HEAD), but this won't be a easy-go because:
\x01, data will not able to fetch out from Oracle directly (as empty string is now encoded). It is also difficult for such data interact with other else system (they will not understand the meaning of\x01).If the main concern of this issue is all about
= ''andIS NULL, it is now solved with following independent issues. They keep overall programming logic as valid with=and<>, don't need to revamp withIS NULL, and also benefit for this issue. It is now integrated with patch attached:I totally agree with this logic. But once investigate the possibility of this issue with implementation, I found that we have no way to utilize such different for a web application. E.g. once people submit a new form, the server can only understand submitted values as 1. valid , or 2. empty. There is no way for us to classify the different between "empty" and "undefined".
From Moodle:
Comment #6
chx commentedDo not reopen until you address the following points:
Comment #7
hswong3i commentedchx: I will try to answer your question one by one:
This is my first step for this patch. Once revamp all char/varchar/text/blob as nullable, there is no extra error for all simpletest. This is something already proved for MySQL, and so we don't need to ask Oracle handle this internally. This change is cross database compatible.
As proved with patch attached (remove the "force NULL" input, but keep fetching with PDO::ATTR_ORACLE_NULLS), this point is not such important as we expected. IF we only apply PDO::ATTR_ORACLE_NULLS, SQL with empty string will always function with MySQL/PostgreSQL but only buggy with Oracle. We can left the revamp of 'default' for Oracle users: if they do need the support, please contribute! (Yes, now I also understand why PDO only provide PDO::ATTR_ORACLE_NULLS for fetching, because input is not such important).
This can't solve the problem, because Oracle will still working with 2 status only (valid and NULL) and so won't able to emulate 3 status (valid, empty and NULL) as other RDBMS (please refer to http://edin.no-ip.com/node/226 for more information). We need the additional 'default' status, so all database engine can work with 3 status (valid, 'default' and NULL) and single code base. On the other hand, around 95% of field remap don't require IS NULL checking, where the rest 5% can handle with 'default' + programming logic update. The str_replace is not a must for this case.
Most likely we don't need to use empty string together with NULL (i.e. both 3 status). As mentioned above, around 95% of Drupal core programming logic can keep untouch, which also means we only use at most 2 status (value and "nothing") for 95%. The rest (use both 3 status: valid, empty and NULL) can be revamp with the use of 'default'. We can't control contribute developers, but they don't always need it :S
We will offer Oracle driver for Drupal, so user may use it with project required :D
The truth picture of this issues is: we just take away something not really meaningful for developers, and request a small revamp of coding style, so code will also functioning with Oracle nicely. We can assist developers with documentation, as like as what we request for
{}, db_query_range() and SchemaAPI. We are not introducing trouble to contribute developers :DComment #8
chx commentedAnd what is going to happen if you happen to have
foo = '%s' AND foo is NOT NULLand %s is empty? You will need to have a foo_is_null column, I believe.Comment #9
chx commentedBack to won't fix according to our IRC reviews. The Oracle driver must be confined to includes/database directory.
Comment #10
hswong3i commented