If I understand #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x correctly, the schema for the text format column should be unsigned and default to NULL. Patch forthcoming.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee’s picture

Status: Active » Needs review
FileSize
829 bytes
TwoD’s picture

Yikes, lots of info in that issue. Only have time to skim through it now and can't find where they talk about that schema change, but I believe you.
Patch looks good, but do we need an upgrade path for that? We haven't had an official D7 release yet, but if someone just replaces an existing -dev version they won't get this schema change, right?

ksenzee’s picture

FileSize
813 bytes

No, if somebody replaces an existing dev version they won't get this schema change. Core isn't supporting an upgrade path - I guess it's your choice whether you want to support it or not. If I were you I probably wouldn't - anyone using a D7 dev version is likely savvy enough to go in and update their database manually - but up to you.

Oh, and here's an updated patch. (I left in the default => 0 bit, which is dumb, since the whole point here is to default to NULL and never use 0.)

sun’s picture

Status: Needs review » Needs work

Thank you! This patch is basically ready to fly for HEAD, but we additionally have to do the same upgrade path like Drupal core modules do (i.e., replace 0 and any unknown format with the last known default text format) -- Filter and User modules implement special module update examples for this; we merely have to copy that code and adjust the table name.

...mmm, with one exception.... Wysiwyg profiles can only exist once, so before changing any possibly existing data, we need to check whether a target row already exists.

sun’s picture

Status: Needs work » Postponed

Postponing on the final decision on #934050: Change format into string

TwoD’s picture

Category: task » bug
Priority: Normal » Critical
Status: Postponed » Needs review
FileSize
6.64 KB

Reopening as #934050: Change format into string is now fixed.
Escalating to critical bug as Wysiwyg no longer functions at all in D7 Beta 2.

#950702: Failure to install with Drupal 7 beta 2 was recently posted and I marked a duplicate of this issue.
It indicates Wysiwyg is no longer able to save the editor profiles, as predicted here.
The first error in that issue seems unrelated, but I have confirmed the second one:

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'filtered_html' for column 'format' at row 1: INSERT INTO {wysiwyg} (format, editor) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => filtered_html [:db_insert_placeholder_1] => ckeditor ) in wysiwyg_profile_overview_submit() (line 502 of /home/henrik/Projects/Drupal/wysiwyg/HEAD/wysiwyg.admin.inc).

The attached patch includes schema changes I made to get Wysiwyg working properly again.
I also removed all 'format' prefixes on format ids as they are no longer needed and it just looks weird having them there.

Only thing missing is an hook_update_N() implementation if this one is approved.

TwoD’s picture

Title: update text format schema » Update text format schema (PDOException in D7 Beta 2)

Better title.

sun’s picture

Status: Needs review » Needs work

Thanks!

+++ wysiwyg.install	23 Oct 2010 19:52:40 -0000
@@ -8,7 +8,7 @@ function wysiwyg_schema() {
-      'format' => array('type' => 'int', 'not null' => TRUE, 'default' => 0),
+      'format' => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE),

1) 'not null' should be FALSE.

2) We need to add a module update that converts the 'format' column accordingly for existing installations.

+++ wysiwyg.js	23 Oct 2010 19:52:40 -0000
@@ -56,7 +56,7 @@ Drupal.behaviors.attachWysiwyg = {
-      var format = 'format' + this.value;
+      var format = this.value;

That, we need to test. Format IDs were integers previously. If the first format key in Drupal.settings was an integer/numeric, then http://api.drupal.org/api/function/drupal_json_encode/7 converted the settings into an indexed array, losing the actual format IDs.

It may be possible that PHP's json_encode() is smart enough to differ between integer keys and numeric strings, but as of now, I doubt that, though I didn't test.

Background: Existing format IDs are just simply converted into strings. Only newly created formats get a "proper" machine name. So we still have to take the possibility of format machine names in the form of "1" into account.

Powered by Dreditor.

aspilicious’s picture

Thnx for fixing this nice message in tha future :)

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'filtered_html' for column 'format' at row 1: INSERT INTO {wysiwyg} (format, editor) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => filtered_html [:db_insert_placeholder_1] => ckeditor ) in wysiwyg_profile_overview_submit() (line 502 of C:\xampp\htdocs\vocal4s\sites\all\modules\wysiwyg\wysiwyg.admin.inc).

Go ksenzee and sun

TwoD’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Sun and I had a chat and decided 'not null' should be TRUE and to deal with the 'format' prefix in another issue.

Updated patch with my first attempt at an hook_update_N() implementation. ;)
http://api.drupal.org/api/function/db_change_field/7 recommended to remove and recreate any affected primary keys so I did that too.

sun’s picture

+++ wysiwyg.install	24 Oct 2010 20:47:28 -0000
@@ -163,3 +163,17 @@ function wysiwyg_update_6200() {
+ *  Convert text format ids to strings.

1) Duplicate leading space.

2) Should be "Change {wysiwyg}.format into a string."

3) We should additionally implement hook_update_dependencies(), in order to ensure that this update runs after Filter module's update. Use http://api.drupal.org/api/function/user_update_dependencies/7 as template

+++ wysiwyg.install	24 Oct 2010 20:47:28 -0000
@@ -163,3 +163,17 @@ function wysiwyg_update_6200() {
+function wysiwyg_update_7201() {

This update is not specific to 7.x-2.x, but is rather required for any 7.x branch of Wysiwyg. Therefore, the update number should be 7000.

+++ wysiwyg.install	24 Oct 2010 20:47:28 -0000
@@ -163,3 +163,17 @@ function wysiwyg_update_6200() {
+  db_drop_primary_key('wysiwyg');
...
+  array(
+    'primary key' => array('format'),
+  ));

I never understood the actual behavior of that additional db_change_field() argument. So I'd prefer to just do a db_add_primary_key() at the end.

Powered by Dreditor.

TwoD’s picture

Right, here we go again. Thanks for reviewing this so quickly!

sun’s picture

Status: Needs review » Fixed
+++ wysiwyg.install	24 Oct 2010 21:22:31 -0000
@@ -163,3 +163,28 @@ function wysiwyg_update_6200() {
+ * Implementation of hook_update_dependencies().

Note: It's "Implements hook_FOO()." now, regardless of Drupal core version. :)

Powered by Dreditor.

TwoD’s picture

Doh! Oh well, I'll fix that before comitting when I get back home.
EDIT: Or maybe you did already?

aspilicious’s picture

I updated (just through the interface). Still got this:

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'filtered_html' for column 'format' at row 1: INSERT INTO {wysiwyg} (format, editor) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => filtered_html [:db_insert_placeholder_1] => ckeditor ) in wysiwyg_profile_overview_submit() (line 502 of C:\xampp\htdocs\vocal4s\sites\all\modules\wysiwyg\wysiwyg.admin.inc)

TwoD’s picture

If you updated an existing installation of Wysiwyg, did you run update.php?

sun’s picture

@TwoD: Yup, I already did + also re-located hook_update_dependencies() in the file.

@aspilicious: Did you either run update.php or completely re-install the module?

aspilicious’s picture

I used the update function from the UI.
Copy pasting the module in the correct folder did tha trick.

Second time in a day that the install/update UI broke with beta 2.
Totally useless at this moment.

BrittaL’s picture

Please don't say "totally useless", because as how you formulate it, it turns to a general statement which is individually simply not true. If there is a repeating problem with it on your side, I think it is better to open a new issue and ask for testing by others in the community. Don't get me wrong. YOU are a contributer and I am quiet new hear, but: I can report, that it mostly already works fine and I would love to see the install/upate UI finished and with more options (like in the plugin manager) by default, because it is wide spread standart in other CMS and Blog MS and one of the reasons for me to decide for drupal 7 next. No need to explain the difficulties with ftp clients, which mostly don't support upload with unzip function, what makes starting a new installation of drupal with all the frequently needed 20 modules by default a two-days task. This is absolutely a no-go for heavy drupal usage in the queue with many clients.

aspilicious’s picture

Britta I alrdy opened an issue WITH a video :).

Its not totally useless but its kinda bad if 50% of the operations I did failed in some way...

#952368: UI installer doesn't work (on fresh install)

BrittaL’s picture

You're right aspilicious.
And thanks for the link. Sorry, I didn't know about the other issue.

thanks for understanding.

susheel_c’s picture

Thanks. The patch fixes things with my install of WYSIWYG and drupal 7 b2.

Status: Fixed » Closed (fixed)

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