This issue is for sharing any thoughts on schema changes we might possibly like to do.

Comments

(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.)

Some fields --flag_short, flag_long, flag_message, unflag_short, ...-- can be folded into the 'options' column.

What 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.)

We 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.

Thanks 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.

Status:Postponed» Needs review
StatusFileSize
new12.35 KB
new12.68 KB

This 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.

Status:Needs review» Reviewed & tested by the community

Very 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:

$configuration = array(
  'show_on_page' => 1,
  ..
  ...
  ...
  'types' => array('story', 'forum', 'blog'),
);
$flag = flag_flag::factory_by_content_type('node');
$flag->form_input($configuration);
$flag->save();

==========

(That's for the D5 branch.)

   // Ensure views is available.
   include_once(drupal_get_path('module', 'views') .'/views.module');
+  include_once(drupal_get_path('module', 'flag') .'/flag.inc');

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.)

while obvious columns (fid, content_type, name, title, roles, and global) would stay as separate columns.

(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.)

[...] so I'm marking it RTBC

BTW, 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.

By now I have tested the patch on D5. All works fine. (I haven't tested the views_bookmarks thingies, though.)

Status:Reviewed & tested by the community» Fixed

Thanks 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!

Status:Fixed» Closed (fixed)

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