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.
This issue is for sharing any thoughts on schema changes we might possibly like to do.
Comment | File | Size | Author |
---|---|---|---|
#6 | flag_options_schema5.patch | 12.68 KB | quicksketch |
#6 | flag_options_schema6.patch | 12.35 KB | quicksketch |
Comments
Comment #1
mooffie CreditAttribution: mooffie commented(Our module cannot be installed together with the "Flag content" module, because they both use a "flag_content" table. However, since now we now prevent bad things from happening by refusing to install our module if the other one is present, this issue is closed.)
Comment #2
mooffie CreditAttribution: mooffie commentedSome fields --flag_short, flag_long, flag_message, unflag_short, ...-- can be folded into the 'options' column.
Comment #3
mooffie CreditAttribution: mooffie commentedWhat do we need the {flag_types} table for?
(Some may say "user flags could use it too, to store roles", and in some other time I'll respond to this.)
Comment #4
mooffie CreditAttribution: mooffie commentedWe need to document the schema (using
'description' => t('blah blah blah')
).People reading our code will be led to believe that a flag may be used to flag different types of content because there's a 'content_type' column everywhere.
Comment #5
quicksketchThanks for bringing all this up mooffie. After some serious internal-debate with myself, I agree with your statement in #2, where all our messages can be moved into the options column, while obvious columns (fid, content_type, name, title, roles, and global) would stay as separate columns.
Comment #6
quicksketchThis issue is blocking #160519: Allow admin to select Ajax or non-ajax behaviour per Bookmark type, as we need an additional option for "Confirmation form message" when a user is being asked to flag or unflag something via a popup or confirmation page. Since I can't bear to put some options in the "options" column and others having their own column, this patch consolidates our (un)flag _short, _long, and _message columns all into the options column. It also fixes some bugs with the Drupal 5 Views Bookmark -> Drupal 5 Flag conversion. I retested all our upgrade paths and things seem to be working well with these changes.
Comment #7
mooffie CreditAttribution: mooffie commentedVery nice! That was quite ugly what we had there.
I haven't tested the patch, but I read it carefully, so I'm marking it RTBC.
Some notes (*but you can ignore them*):
In our hook_install() we're doing direct SQL to create the 'bookmarks' flag. This isn't needed anymore because we have a CRUD API. The SQL could be replaced with:
==========
(That's for the D5 branch.)
hook_install(), in the D5 branch, already loads 'flag.inc' (search for the string "Future code in this hook may.,."). But it does this late, so you may move that loading code to the start of hook_install (and remove the loading code you just added). (If you change my dirname(__FILE__) to drupal_get_path(), make sure to do that in flag_init() as well.)
Comment #8
mooffie CreditAttribution: mooffie commented(I suspect in the future we'll move 'roles' too into 'options' because, to implement #285237: Ability to disallow a flag/unflag operation, we'll probably introduce a couple more similar fields and there won't be a reason to keep only one of them outside 'options'. And... why shouldn't 'global' and 'title' too be moved into 'options'? Of course, these musings shouldn't postpone committing the patch. Go ahead.)
Comment #9
mooffie CreditAttribution: mooffie commentedBTW, you could commit this patch, and similars, without waiting for my "approval". I'd hate to think that I caused you to stop your work by just not being here for a couple of hours/days.
Comment #10
mooffie CreditAttribution: mooffie commentedBy now I have tested the patch on D5. All works fine. (I haven't tested the views_bookmarks thingies, though.)
Comment #11
quicksketchThanks for the review mooffie. This is something big enough that I wanted a second set of eyes before putting in. We can further consolidate columns in #285237: Ability to disallow a flag/unflag operation, where we'll probably need to rename "roles" to "mark_roles". This will probably have wider implications throughout our api, so keeping it separate makes sense.
Committed both versions. w00t!
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.