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.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | 734762_tidy_filter_upgrade_10.patch | 3.51 KB | scor |
#8 | drupal.filter-updates.8.patch | 3.69 KB | sun |
#1 | 734762-tidy-filter-upgrade-path.patch | 3.77 KB | Damien Tournoud |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #3
sunHm. 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!
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commented@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 :)
Comment #5
sunok. 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 ;)
When is filter_formats renamed into filter_format (singular) ?
Can fix the casing and shorten the second part to "and add {filter_format}.weight." ?
Can we move the second comment + first code line up, so that the first comment + following code maps together? :)
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.
Comment #6
ctmattice1 CreditAttribution: ctmattice1 commentedone too many 'weight',
Also ran into a problem not directly related to indexes issue here but before creating a new patch need some input.
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
It is never returned and the update.php silently dies at 7005 and proceeds to run through to completion
any Ideas?
Comment #7
ctmattice1 CreditAttribution: ctmattice1 commentedOk 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
Comment #8
sunDid 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.
Comment #9
Dries CreditAttribution: Dries commentedI think it would be nice if we could run an actual upgrade, not?
Comment #10
scor CreditAttribution: scor commentedin filter_update_7000(), the filters table is named filters (plural), it's only after filter_update_7002() that it becomes filter (singular).
Comment #11
Dries CreditAttribution: Dries commentedGreat. It sounds like scor tested the upgrade, and that he fixed a problem. Committed to CVS HEAD.