Posted by Damien Tournoud on March 6, 2010 at 11:37pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | filter.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D7 upgrade path |
Issue Summary
We are changing (hear: dropping and recreating) the 'list' index several times, and the unique key on (filter, module, name) is completely unnecessary, because (filter, name) is already a primary key.
Comments
#1
#2
#3
+++ modules/filter/filter.install@@ -54,9 +54,6 @@ function filter_schema() {
- 'unique keys' => array(
- 'fmn' => array('format', 'module', 'name'),
- ),
Hm. Why are we removing the unique key? This exposed and saved us from introducing major bugs at least once.
145 critical left. Go review some!
#4
@sun: we add a primary key on (filter, name), which render the unique key on (filter, module, name) unnecessary. In other words: the primary key is the new unique key :)
#5
ok. It took some time to find the original issue #546350: Remove hardcoded numeric deltas from hook_filter_info(), because the docs for http://api.drupal.org/api/function/hook_filter_info/7 don't mention at all that filter names have to be namespaced/prefixed with the name of the exposing module - which effectively approves this change. I'll file a separate issue about this entire pile of crap of http://api.drupal.org/api/function/hook_filter_info/7 documentation, btw ;)
+++ modules/filter/filter.install@@ -161,11 +158,30 @@ function filter_update_dependencies() {
function filter_update_7000() {
- db_add_field('filter_formats', 'weight', array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'size' => 'tiny'));
- db_add_index('filter_formats', 'weight', array('weight'));
...
+ db_add_field('filter_format', 'weight', 'weight', array(
When is filter_formats renamed into filter_format (singular) ?
+++ modules/filter/filter.install@@ -161,11 +158,30 @@ function filter_update_dependencies() {
+ * Increase the size of {filter}.weight and Add a weight column to the {filter_format} table.
Can fix the casing and shorten the second part to "and add {filter_format}.weight." ?
+++ modules/filter/filter.install@@ -161,11 +158,30 @@ function filter_update_dependencies() {
+ // Change the weight column of the filter table to normal (ie. non tiny) int.
+ // The list index will be recreated by filter_update_7004().
+ db_drop_index('filter', 'list');
+ db_change_field('filter', 'weight', 'weight', array(
Can we move the second comment + first code line up, so that the first comment + following code maps together? :)
+++ modules/filter/filter.install
@@ -217,9 +233,11 @@ function filter_update_7003() {
+ // as filter_update_7004() will add a primary key on (filter, name).
@@ -229,11 +247,8 @@ function filter_update_7003() {
+ 'list' => array('weight', 'module', 'name'),
The comments (also in update 7000) refer to 7004, but this patch actually says 7003 :)
Aside from those minor issues, this patch looks good - although I did not apply it to see the resulting flow, but I trust that you did that right.
#6
+++ modules/filter/filter.install@@ -161,11 +158,30 @@ function filter_update_dependencies() {
+ // Change the weight column of the filter table to normal (ie. non tiny) int.
+ // The list index will be recreated by filter_update_7004().
+ db_drop_index('filter', 'list');
+ db_change_field('filter', 'weight', 'weight', array(
one too many 'weight',
Also ran into a problem not directly related to indexes issue here but before creating a new patch need some input.
function filter_update_7005() {
// Move role data from the filter system to the user permission system.
$all_roles = array_keys(user_roles());
$default_format = variable_get('filter_default_format', 1);
$result = db_select('filter_format', 'ff')->fields('ff')->execute();
foreach ($result as $format) {
// We need to assign the default format to all roles (regardless of what
// was stored in the database) to preserve the behavior of the site at the
// moment of the upgrade.
$format_roles = ($format->format == $default_format ? $all_roles : explode(',', $format->roles));
foreach ($format_roles as $format_role) {
if (in_array($format_role, $all_roles)) {
print "pass checkpoint "; die();
user_role_grant_permissions($format_role, array($fname));
print "pass checkpoint "; die();
}
}
}
print "pass checkpoint "; die();
I'm using
print "pass checkpoint "; die();to debugon the first pass with update.php I get the the break point and it displays "pass checkpoint" and dies. when I comment out the first checkpoint and it proceeds to
user_role_grant_permissions($format_role, array($fname));It is never returned and the update.php silently dies at 7005 and proceeds to run through to completion
any Ideas?
#7
Ok in playing around with filter update I think I can get 7005 working.
BUT it requires a change to the user.module. Before I go any further with this I would like to get some opinions.Damien?, sun?, catch? Temporarily marking this as critical because of node access violations I seen croppping up today on the forum. Will mark back down to normal if I'm totally off the wall or will post these results to "user.module" if correct
In the function
user_role_grant_permissions($format_role, array($fname));I believe there is a foreach() missing. The variable $modules is an array but the origional code does not account for it. Here is the diff of the changes for comments.@@ -2678,13 +2678,15 @@ function user_role_grant_permissions($ri
$modules = user_permission_get_modules();
// Grant new permissions for the role.
foreach ($permissions as $name) {
- db_merge('role_permission')
- ->key(array(
- 'rid' => $rid,
- 'permission' => $name,
- 'module' => $modules[$name],
- ))
- ->execute();
+ foreach ($modules as $value) {
+ db_merge('role_permission')
+ ->key(array(
+ 'rid' => $rid,
+ 'permission' => $name,
+ 'module' => $value,
+ ))
+ ->execute();
+ }
}
// Clear the user access cache.
and here is the actual code from todays 7.x-dev
function user_role_grant_permissions($rid, array $permissions = array()) {
$modules = user_permission_get_modules();
// Grant new permissions for the role.
foreach ($permissions as $name) {
db_merge('role_permission')
->key(array(
'rid' => $rid,
'permission' => $name,
'module' => $modules[$name],
))
->execute();
}
}
am I missing something? Isn't the a name space collision as well
#8
Did the minor comment tweaks myself, fixed the "filter_format[s]" table name mismatch, fixed the duplicate 'weight', and double-checked that anything removed or moved in this patch is actually covered.
Didn't run an actual upgrade though.
Either way, this looks ready to fly for me.
@ctmattice1: That user_role_grant_permissions() issue may (but most likely not) be a different issue.
#9
I think it would be nice if we could run an actual upgrade, not?
#10
in filter_update_7000(), the filters table is named filters (plural), it's only after filter_update_7002() that it becomes filter (singular).
#11
Great. It sounds like scor tested the upgrade, and that he fixed a problem. Committed to CVS HEAD.
#12
Automatically closed -- issue fixed for 2 weeks with no activity.