Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#12 | wysiwyg-931374-12-format-schema.patch | 2.05 KB | TwoD |
#10 | wysiwyg-931374-10-format-schema.patch | 1.74 KB | TwoD |
#6 | wysiwyg-931374-6-format-schema.patch | 6.64 KB | TwoD |
#3 | 931374-3-format-schema.patch | 813 bytes | ksenzee |
#1 | 931374-1-format-schema.patch | 829 bytes | ksenzee |
Comments
Comment #1
ksenzeeComment #2
TwoDYikes, 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?
Comment #3
ksenzeeNo, 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.)
Comment #4
sunThank 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.
Comment #5
sunPostponing on the final decision on #934050: Change format into string
Comment #6
TwoDReopening 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:
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.
Comment #7
TwoDBetter title.
Comment #8
sunThanks!
1) 'not null' should be FALSE.
2) We need to add a module update that converts the 'format' column accordingly for existing installations.
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.
Comment #9
aspilicious CreditAttribution: aspilicious commentedThnx 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
Comment #10
TwoDSun 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.
Comment #11
sun1) 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
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.
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.
Comment #12
TwoDRight, here we go again. Thanks for reviewing this so quickly!
Comment #13
sunNote: It's "Implements hook_FOO()." now, regardless of Drupal core version. :)
Powered by Dreditor.
Comment #14
TwoDDoh! Oh well, I'll fix that before comitting when I get back home.
EDIT: Or maybe you did already?
Comment #15
aspilicious CreditAttribution: aspilicious commentedI 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)
Comment #16
TwoDIf you updated an existing installation of Wysiwyg, did you run update.php?
Comment #17
sun@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?
Comment #18
aspilicious CreditAttribution: aspilicious commentedI 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.
Comment #19
BrittaL CreditAttribution: BrittaL commentedPlease 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.
Comment #20
aspilicious CreditAttribution: aspilicious commentedBritta 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)
Comment #21
BrittaL CreditAttribution: BrittaL commentedYou're right aspilicious.
And thanks for the link. Sorry, I didn't know about the other issue.
thanks for understanding.
Comment #22
susheel_c CreditAttribution: susheel_c commentedThanks. The patch fixes things with my install of WYSIWYG and drupal 7 b2.