To reproduce

drush sqlq "INSERT INTO wysiwyg (format, editor, settings) VALUES ('6', '', '{{hello}} {this} is a test');"

Expected result

The database contains the value '{{hello}} {this} is a test' as a string in the column 'settings'

Actual result

String is 'hello this is a test' with the curly braces (brackets?) stripped out.

Details

I don't think it's possible to support table prefixing unless sqlq also supports parameter substitution. Since that may be a bit out of scope, I think that the table prefixing should be removed. Curly braces are needed when inserting serialized values (as I was trying to do above for wysiwyg editor).

Patch attached that removes this behavior.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mark Theunissen’s picture

Patch attached.

moshe weitzman’s picture

Maybe a --no-prefix option which skips braces replacement? Inserting braces is a bit edge case (as are DB prefixes).

Mark Theunissen’s picture

Would you consider an off-by-default approach? So that --prefix option enables it. I know this may break people's scripts.

Otherwise we could put a warning in the help text explaining that all curly braces are removed in the default behavior. With serialization happening a lot in Drupal, it's becoming quite common to insert them in strings.

greg.1.anderson’s picture

Status: Needs review » Needs work

Changing behavior that breaks compatibility can only happen in master (currently 5.x-dev), but not in 4.x. However, I think that turning off brace replacement by default is a dubious proposition. I think off-when specified is better. You could just set $options['no-prefix'] = TRUE; in your drushrc.php file, and then it would be off-by-default for you.

moshe weitzman’s picture

Category: bug » feature
Status: Needs work » Patch (to be ported)

To be clear, Drupal only do brace replacement when a site actually uses table db prefixing. Most don't and thats why most folks are inserting whatever they please. let me know if you are seeing something different.

I just committed to master a new --db-prefix option which you can set to 0 to disable DB prefix processing.

greg.1.anderson’s picture

Yes, I know usually the braces are just stripped, and I know there are lots of places where drush does not support db prefixing. However, this is one place where it does; it is nice, if you have a prefix on some sites, to have scripts that work regardless of the target. I only have one site (inherited) that uses prefixing, but I try to remember the braces when I can anyway.

I think that the solution in #5 is fine. Thanks.

Mark Theunissen’s picture

Title: Unable to run a query that contains braces as a string value. » sqlq removes all curly braces from sql, including those that are in the text values.

Moshe: My site doesn't use DB prefixing, and I am seeing that the ->prefixTables() function will strip out all curly braces from the query.

Sorry I didn't get back to you both earlier. Let me explain why I still disagree:

When a developer executes a query that contains curly braces in the value, those braces are silently stripped out, and drush returns no error or warning about this. The data in the database is corrupted and the developer will probably not realise that this has happened until much later, and then probably spend a large amount of time debugging this issue. Here are some places where curly braces can legitimately appear in the string value:

1. Tables that contain serialized data, e.g. all the cache_* tables, settings tables like wysiwyg, variable, users, and much more probably.
2. Field values, e.g. a node that contains a snippet of php code.

So all Drupal databases contain curly braces, as all databases contain serialized data. Thus the potential is there for any developer to try INSERT or UPDATE this using Drush and have it silently corrupt the data.

I would guess that very few sites use table prefixing, and those that do are on shared hosts and are unlikely to be using drush. Maybe I'm not understanding the use case completely, but that's my intuition.

If someone executed a query expecting that drush would by default perform the prefixing, and it doesn't, it will fail loudly with a SQL syntax error:

[www]$ drush sqlq "SELECT * FROM {cache_field}"
ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '}' at line 1

At that point, the developer would type drush help sqlq and read about the option to switch on prefixing. They would also read a big warning explaining that if they switch on prefixing, they cannot use sqlq to perform a query with curly braces in the string value. The developer can then check that their data does not contain any curlies.

So out of the two options, the one will silently corrupt data, with no hint or clue given, and the other will fail loudly allowing the developer to investigate. I much prefer the loud fail, and I also think that there is more of a use case for working with serialized data.

Mark Theunissen’s picture

The functions that do table prefixing in core Drupal are meant to run on queries before the value substitution happens. I.e you can run db_prefix_tables() on a query in this form without issues:

"UPDATE {users} SET data = %s"

It can happily remove all {} without affecting anything.

When you run it on a query where the substitution has already happened, you're no longer using it in the way it was intended:

"UPDATE {users} SET data = 'a:3:{....{}....}'"

Now, when it merrily strips out all the {}, it's corrupting the data.

Mark Theunissen’s picture

Status: Patch (to be ported) » Needs work

I'm putting this back to "needs work" because I'm not sure if it will fall off your radar otherwise. Apologies if this is not the case.

moshe weitzman’s picture

Status: Needs work » Fixed

Mark has me convinced. Now off by default in master. Not eligible for backport.

Feel free to reopen this is it needs more discussion.

greg.1.anderson’s picture

Status: Fixed » Needs review
FileSize
3.33 KB

I am also convinced. I committed a change to the sql-query help to match the changes above; see 7533d27.

While this change is an improvement, I don't think we're quite done yet, as it is not very convenient to use sqlq if you have both curly braces in your data and table prefixes. In bash, you can do this:

bash -c 'echo $2 $1 $0' a b c

That will, of course, produce "c b a", as the '$n' parameters are replaced with the values of the arguments that follow. I propose a similar syntax for drush sql-query:

drush sql-query "UPDATE {users} SET data = '%1'" "a:3:{....{}....}"

This special "replacement mode" is only activated if there are arguments that appear after the query; if there are no extra arguments, then any %1 - %n that appear in the query will be left unchanged. Also, if replacements are used, --db-prefix is automatically set.

Here is an example:

drush @site -s sql-query "UPDATE {users} SET mail = '%1' where uid = '%2'" user@domain 1
sql-query: UPDATE users SET mail = 'user@domain' where uid = '1'
Calling system(mysql --database=sitedb --host=localhost --user=www-data --password=password  < /tmp/drush_mrbTvO);

Note that this feature kills the following existing sql-query syntax:

drush sql-query "This parameter is always IGNORED" fileContainingMyQuery.sql

This form is a little odd anyway; the help for sql-query recommends `drush sql-connect` < example.sql instead, so I think it is okay to obsolete the above in drush-5, but it does make this feature ineligible for backporting.

Finally, although I have taken no action on this point, I wonder if the --db-prefix option should be renamed in sql-query, as it could potentially conflict with the --db-prefix used in drush_sql_read_db_spec. The later is only relvant if --db-url is also supplied.

greg.1.anderson’s picture

FileSize
3.44 KB

I should have committed #880154: sql-query could take a --file option. a while time ago; it adds a --file option to sqlq, and pre-dates the second-arg-as-filename form for the same command. This patch re-introduces --file along with the features added in #11.

moshe weitzman’s picture

I'm OK with the direction of #12. We could consider --input-file to closer match and distingush from --result-file.

Note that archive-restore (i think) will have to change as a result of this.

I think it is OK to use --db-prefix here for consistency since it ought to be set to the same value when using drush_sql_read_db_spec (now merged with _drush_sql_get_db_spec())

moshe weitzman’s picture

Priority: Normal » Critical
msonnabaum’s picture

So for 4.5 (since we want to get that out rather soon), I'm good with removing a filename as a second arg since I think that's a bit awkward anyhow, and I'm also good with adding --file (although I really think we should support STDIN here…).

I'm not so sure about the multiple arg approach in #11 though; It needs more discussion. I think it's a reasonable approach, but it's also a really odd use case. How often do we ever input serialized php on the cli? The two most common use cases I can think of would be variables and cache, which we already offer abstractions for both. Seems like a feature that would very rarely get used.

greg.1.anderson’s picture

I agree with #15 completely in the context of drush-4. Let's leave it to --input-file for that branch.

I don't think we need to worry about STDIN, because `drush sql-connect` < file.sql is available.

For drush-5, though, I think we support the replacements. It's not much code, and could be necessary if prefix replacement is ever done; in short, it should go in because it is correct, and prefix replacement can't be guarenteed to work without it. The use case is rare, but remote sqlq is one example I think this might be helpful for.

New patch as soon as I have time...

moshe weitzman’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

I changed both master and 4.x to honor --input-file and not a second argument.

i agree that proper support is welcome in 5.x as Greg describes in #16. i think that needs a small reroll now.

greg.1.anderson’s picture

Assigned: Unassigned » greg.1.anderson
kotnik’s picture

Subscribing.

EugenMayer’s picture

/me notes, that the current -dev version does have a wron example / arg description of --input-file ( its implemented as --file, documented as --input-file)

divbox’s picture

hi there.

i have updated to the 5.0-rc2 version of drush to prevent the { } from being removed in a serialized array I am trying to pass into a sqlq update statement on wysiwyg table. this version works and does not remove the { }, but it is removing the " " around the values. is this something I'm doing wrong?

here's what i pass

~divbox/drush/drush sqlq "UPDATE wysiwyg set settings='a:20:{s:7:"default";i:1;s:11:"user_choose";i:0;s:11:"show_toggle";i:1;s:5:"theme";s:8:"advanced";s:8:"language";s:2:"en";s:7:"buttons";a:2:{s:7:"default";a:33:{s:4:"Bold";i:1;s:6:"Italic";i:1;s:9:"Underline";i:1;s:6:"Strike";i:1;s:11:"JustifyLeft";i:1;s:13:"JustifyCenter";i:1;s:12:"JustifyRight";i:1;s:12:"JustifyBlock";i:1;s:12:"BulletedList";i:1;s:12:"NumberedList";i:1;s:7:"Outdent";i:1;s:6:"Indent";i:1;s:4:"Link";i:1;s:6:"Unlink";i:1;s:6:"Anchor";i:1;s:9:"TextColor";i:1;s:7:"BGColor";i:1;s:11:"Superscript";i:1;s:9:"Subscript";i:1;s:10:"Blockquote";i:1;s:6:"Source";i:1;s:14:"HorizontalRule";i:1;s:3:"Cut";i:1;s:4:"Copy";i:1;s:5:"Paste";i:1;s:9:"PasteText";i:1;s:13:"PasteFromWord";i:1;s:4:"Font";i:1;s:8:"FontSize";i:1;s:6:"Styles";i:1;s:5:"Table";i:1;s:5:"Flash";i:1;s:6:"Smiley";i:1;}s:11:"drupal_path";a:1:{s:4:"Link";i:1;}}s:11:"toolbar_loc";s:3:"top";s:13:"toolbar_align";s:4:"left";s:8:"path_loc";s:4:"none";s:8:"resizing";i:1;s:11:"verify_html";i:1;s:12:"preformatted";i:0;s:22:"convert_fonts_to_spans";i:1;s:17:"remove_linebreaks";i:1;s:23:"apply_source_formatting";i:0;s:27:"paste_auto_cleanup_on_paste";i:1;s:13:"block_formats";s:32:"p,address,pre,h2,h3,h4,h5,h6,div";s:11:"css_setting";s:5:"theme";s:8:"css_path";s:0:"";s:11:"css_classes";s:0:"";}' where editor='ckeditor'"

Here's what's in the DB


mysql> select settings from wysiwyg where editor="ckeditor";

| a:20:{s:7:default;i:1;s:11:user_choose;i:0;s:11:show_toggle;i:1;s:5:theme;s:8:advanced;s:8:language;s:2:en;s:7:buttons;a:2:s:7:default;a:33:s:4:Bold;i:1;s:6:Italic;i:1;s:9:Underline;i:1;s:6:Strike;i:1;s:11:JustifyLeft;i:1;s:13:JustifyCenter;i:1;s:12:JustifyRight;i:1;s:12:JustifyBlock;i:1;s:12:BulletedList;i:1;s:12:NumberedList;i:1;s:7:Outdent;i:1;s:6:Indent;i:1;s:4:Link;i:1;s:6:Unlink;i:1;s:6:Anchor;i:1;s:9:TextColor;i:1;s:7:BGColor;i:1;s:11:Superscript;i:1;s:9:Subscript;i:1;s:10:Blockquote;i:1;s:6:Source;i:1;s:14:HorizontalRule;i:1;s:3:Cut;i:1;s:4:Copy;i:1;s:5:Paste;i:1;s:9:PasteText;i:1;s:13:PasteFromWord;i:1;s:4:Font;i:1;s:8:FontSize;i:1;s:6:Styles;i:1;s:5:Table;i:1;s:5:Flash;i:1;s:6:Smiley;i:1;}s:11:drupal_path;a:1:s:4:Link;i:1;}}s:11:toolbar_loc;s:3:top;s:13:toolbar_align;s:4:left;s:8:path_loc;s:4:none;s:8:resizing;i:1;s:11:verify_html;i:1;s:12:preformatted;i:0;s:22:convert_fonts_to_spans;i:1;s:17:remove_linebreaks;i:1;s:23:apply_source_formatting;i:0;s:27:paste_auto_cleanup_on_paste;i:1;s:13:block_formats;s:32:p,address,pre,h2,h3,h4,h5,h6,div;s:11:css_setting;s:5:theme;s:8:css_path;s:0:;s:11:css_classes;s:0:;} | 

I have also tried using ' ' around values in the serialized string, but they are also not working.

Any ideas? Perhaps I am doing something wrong here?

thanks for your help! drush rules!

div

kotnik’s picture

Divbox, basically this is what you are doing (spot the error):

drush sqlq "UPDATE wysiwyg set settings="something""
greg.1.anderson’s picture

Status: Needs work » Fixed

To be perhaps a little clearer than #22, note that the ' quotes do not protect the embedded " quotes in Bash, because it is just an ordinary character embedded inside of a "-quoted string. You'll need to add backslashes in front of each embedded ".

divbox’s picture

Status: Fixed » Needs work

thanks guys!

i thought i had tried the /" on this once, but I must have missed some so thought it wasn't working. i am now getting this to work so thanks again for the help!

div

scottrigby’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

magnusk’s picture

Version: All-versions-4.x-dev »
Category: feature » bug
Status: Closed (fixed) » Needs work

This change does not seem to work in D7 with Drush 5. I have scripts that rely on the curly-braces naming of tables, and it's been working just fine in D6 and D7, with Drush 4.

In trying to make it work again I found that the sql-query command needs to bootstrap to DRUSH_BOOTSTRAP_DRUPAL_DATABASE instead of DRUSH_BOOTSTRAP_DRUSH.

Also, I think the default should be to handle the curly braces around table names as Drupal does, not the reverse. With D6's db_prefix_tables it was known that the curly braces would be replaced throughout the query, and hence one should use placeholders. Having looked at D7's DatabaseConnection::prefixTables I think the same curly-brace-in-data issue potentially exists, but the new database API is mostly using placeholders so it should be a fairly rare problem.

scottrigby’s picture

Status: Needs work » Needs review
FileSize
965 bytes

@magnusk that does the trick with --db-prefix.

Not sure if we should do this here, but I rolled a patch with that small change against master.

moshe weitzman’s picture

That change removes our ability to use drush to query a DB without even a Drupal site setup. There are use cases for that.

greg.1.anderson’s picture

Status: Needs review » Needs work

We shouldn't bootstrap to DRUSH_BOOTSTRAP_DRUPAL_DATABASE unless curly-brace table replacements are in use. Leave 'bootstrap' => DRUSH_BOOTSTRAP_DRUSH unchanged, and instead call drush_bootstrap_max(DRUSH_BOOTSTRAP_DRUPAL_DATABASE) at the appropriate time.

Any change to the default for curley-brace expansion would have to be a Drush-6 change. I don't care too much which way the default goes, but since different people have different opinions, my inclination would be to keep the defaults the same unless there is sufficiently strong reasons to reverse the past decision. If you don't like the normal Drush default, you can always set $command_specific['sql-query'] = array('db-prefix' => TRUE);.

scottrigby’s picture

Ah… resetting to needs work

greg.1.anderson’s picture

Putting in a no-content comment so that @scottrigby gets an 'updated' marker for #30.

scottrigby’s picture

Status: Needs work » Needs review
FileSize
970 bytes

@moshe weitzman yep - i hadn't thought through that, just made a blind patch from #27 after minimal testing :p good point!

@greg.1.anderson ok, how about in drush_sql_query()?

This patch bootstraps to DRUSH_BOOTSTRAP_DRUPAL_DATABASE only if --db-prefix is used (am I interpreting #13 and #30 correctly?). Didn't seem necessary to pass an additional $db_prefix (or $substituteBraces) param to _drush_sql_query() since it already checks drush_has_boostrapped(DRUSH_BOOTSTRAP_DRUPAL_DATABASE) before enabling prefix processing… if we can just set it in the same place that drush_sql_bootstrap_further() gets called. Does this makes sense?

greg.1.anderson’s picture

#33 looks right, but I haven't had a chance to test it yet.

magnusk’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #33 solves the DRUSH_BOOTSTRAP_DRUPAL_DATABASE problem for sql-query and using curly braces.

Greg, thanks for the tip on setting a default option for the command!

moshe weitzman’s picture

Status: Reviewed & tested by the community » Fixed

Committed #33 to master. Does not apply cleanly to 4.x. If someone wants this there, please reroll and reopen this issue.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

EugenMayer’s picture

@Moshe i think that is pretty interesting for the aegir people for the 1.x branch, since it is still 4.x