Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Copying this in from the main upgrade issue:

Talked to @Amitaibu over IRC.

[07:42am] GaborHojtsy: amitaibu: hi
[07:42am] amitaibu: GaborHojtsy: Hi man, what's up?
[07:47am] GaborHojtsy: amitaibu: hey, I was wondering if you know of any upgrade path for og_user_roles to og 7 and/or messaging/notifications to your message module for the l.d.o D7 update?
[07:48am] amitaibu: GaborHojtsy: ogur to OG should be easier to write (no upgrde path that I know of). messaging => message would require more effort, as Message is a simple API, and the subscription logic isn't there
[07:49am] amitaibu: GaborHojtsy: about ogur -- are the roles the same in each group, or different?
[07:51am] GaborHojtsy: amitaibu: they are same, I don't think ogur allows us to have different roles for different groups
[07:53am] amitaibu: GaborHojtsy: ok, that's good, it will be easier to map the roles
[07:54am] GaborHojtsy: amitaibu: :)
[07:54am] amitaibu: GaborHojtsy: is there already a dev site with OG7 running there?
[07:55am] GaborHojtsy: amitaibu: no, not yet
[07:55am] GaborHojtsy: amitaibu: I'm trying to help connect the dot for the guys there is a d.o test site to use for this but that is still running a copy of the D6 site
[07:56am] GaborHojtsy: amitaibu: basically the two biggies we expect in the update are the ogur and messaging upgrade/migration
[07:56am] GaborHojtsy: amitaibu: the site is going to be http://d7upgrade-localize.redesign.devdrupal.org/
[07:57am] amitaibu: GaborHojtsy: i think/ hope ogur shouldn't be a big issue. Messaging is imo the bigger problem
[07:59am] GaborHojtsy: amitaibu: hm, ok
[08:01am] GaborHojtsy: amitaibu: thanks

amitaibu’s picture

Why not port directly to 2.x?

SebCorbin’s picture

@Amitaibu an upgrade path exists from 6.x-2.x to 7.x-2.x?

What are the main differences between 7.x-1.x and 7.x-2.x?

amitaibu’s picture

No upgrade path exists.

2.x is a rewrite of 1.x based on the new improvements in D7, mostly notable:

  • Deprecated the Group entity.
  • Use Entity reference module, and allow creating multiple group-audience fields.
  • You can set different permissions/ roles for different content types (i.e. not a single global roles/ permissions).

Although 2.x is only in it's alpha stage, I think you should atleast have a look in it, as 1.x is mostly a bug fixes release now.

SebCorbin’s picture

Ok so I still need to do an upgrade path from D6.
Once this is done I'll try an update to the 7.x-2.x to see if it's relevant for l.d.o

SebCorbin’s picture

Added an og_migrate plugin, patched attached.

SebCorbin’s picture

Status: Active » Needs review
amitaibu’s picture

Status: Needs review » Needs work

Thanks, I would much rather see this plugin for 2.x (which shouldn't be too hard), as 1.x is now mainly a bug fix release.

+++ b/og.installundefined
@@ -671,19 +671,24 @@ function og_update_7000(&$sandbox) {
+    if (db_table_exists('og_users_roles')) {

Let's add a comment about this table, that it's OGUR.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+  'name' => t('OG User roles'),

Lets add a description as-well to the plugin.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+  'dependencies' => array(),

You can remove this line.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+    // Create OG roles and map to drupal roles

Missing dot.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+      ->execute()->fetchCol();

Move fetchCol() one line down + Shouldn't it be fetchAll()?

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+      $og_role = og_create_global_role($role->name);

This function no longer exists in 2.x. An OG role is now always assigned to the group it belongs to.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+    // Calculate max items.
+    $query = db_select('d6_og_users_roles', 'ogur');

Shouldn't this be above "Create OG roles..."?

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+    if (!$context['sandbox']['max']) {
+      // No data to process, so return.
+      $context['finished'] = 1;
+      return;

This seems to be duplicated and not needed, as we have a few lines down again.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+  foreach ($records as $record) {

In other places we use foreach ($result as $row) { as variable names.

+++ b/og_migrate/plugins/og_migrate/upgrade_ogur.incundefined
@@ -0,0 +1,72 @@
+    $group = og_get_group('node', $record->gid);

In 2.x the node is the group, so we don't need this.

pgillis’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
FileSize
4.19 KB

I believe I have gotten most of the migration working. Not sure if the permissions need to be migrated here or not. Uploading patch to get feedback early to make sure my approach is sound so far.

pgillis’s picture

Status: Needs work » Needs review
amitaibu’s picture

thanks, but lots of tabs, and whitespaces. Can you fix/ trim them and re-roll

pgillis’s picture

Sorry about that. I think this is more inline with the style guidelines.

Status: Needs review » Needs work

The last submitted patch, og-og_users_roles_update-1565546-12.patch, failed testing.

pgillis’s picture

Status: Needs work » Needs review
FileSize
4.17 KB

Just one of those days:(

amitaibu’s picture

Status: Needs review » Needs work
+++ b/og.installundefined
@@ -792,18 +792,25 @@ function og_update_7000(&$sandbox) {
+	 // Drop the current primary key, as we are adding a serial column.
+	 db_drop_primary_key($table);
+	 db_add_field($table, 'upgrade_id', array(

Sorry, still tabs...

pgillis’s picture

Ok, must be something happening when I'm creating the patch with git because I have updated eclipse to use spaces and they appear to be spaces in my file before the patch.

pgillis’s picture

Status: Needs work » Needs review
FileSize
4.25 KB

Hmm...I've attached another version of the patch. The whitespace you mentioned in #15 above is comprised of spaces. Am I still doing something wrong there? It is preserving the format of the original file. It is adding 2 additional spaces because of the required conditional.

amitaibu’s picture

Status: Needs review » Needs work
+++ b/og.installundefined
@@ -792,18 +792,25 @@ function og_update_7000(&$sandbox) {
+    }
+    ¶
     // Add serial ID to d6_og* tables so we can keep track of records that were
     // processed.

White space (I use Dreditor to see them)

+++ b/plugins/og_migrate/7200/og_7200_ogur.incundefined
@@ -0,0 +1,74 @@
+    // Create OG roles and map to drupal roles

Missing dot in end of line.

+++ b/plugins/og_migrate/7200/og_7200_ogur.incundefined
@@ -0,0 +1,74 @@
+      $og_role = og_role_create($role->name, 'node', 0, 'group');

This looks wrong - "group" isn't a hardcoded bundle.

pgillis’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

Sorry about all the whitespace!

I've updated the code per your requests. I am assuming that the group type shouldn't be hardcoded either so I am now looping over all types and their bundles.

Status: Needs review » Needs work

The last submitted patch, og-og_users_roles_update-1565546-19.patch, failed testing.

pgillis’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

Sigh...I will now open all my patches in vi before upload to make sure there are no ^M hidden in there!

amitaibu’s picture

Status: Needs review » Fixed

I've cleaned up a bit and committed, thank you guys!

Status: Fixed » Closed (fixed)

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

amitaibu’s picture

Assigned: SebCorbin » Unassigned

@pgillis,
Due/ Thanks to #1686394: Replace custom og-migrate with a proper integration with "Migrate" module we'll need to revisit the upgrade path. Any chance you can have a look there?

pgillis’s picture

Assigned: Unassigned » pgillis
Status: Closed (fixed) » Active

I'll take a look.

pgillis’s picture

Status: Active » Needs review
FileSize
9.22 KB

I have attached a patch to implement this functionality utilizing the migration module.

pgillis’s picture

Status: Needs review » Active

sorry ignore that last patch I have to fix my git repo.

pgillis’s picture

Status: Active » Needs review
FileSize
5.78 KB

The attached patch should work properly against the dev line.

amitaibu’s picture

Thanks.

@SebCorbin, any chance you can confirm this patch?

SebCorbin’s picture

What if I want to use global roles instead of duplicating role for each group?

This is what happens on the l.d.o database:
Screen Shot 2013-05-02 at 16.29.13.png

Just removing the dependency over OgMigrateOgurRoles in OgMigrateOgur should do that?

amitaibu’s picture

> What if I want to use global roles instead of duplicating role for each group?

IMO, That's actually what should happen - AFAIK OGUR didn't export roles per group, but "global" roles - no?

pgillis’s picture

I believe the patch is doing what you ask. You will notice it is passing 0 in for the gid. Having per group roles is not handled in this migration. The reason it has to copy them over to og_role is because I believe in drupal 7 you can no longer reference global roles from within og. So you need to move these global roles into the og_role with 0 as gid to make it "global" in og. Does that make sense?

SebCorbin’s picture

Status: Needs review » Reviewed & tested by the community

I opened another issue for this matter #1985800: Set default value of og_roles_permissions field during migration

As for this patch it works good for me.

amitaibu’s picture

Status: Reviewed & tested by the community » Needs work

We should probably conditionally add the Migrate handlers in og_migrate_api() if they are really needed.

SebCorbin’s picture

Issue tags: +Localize D7 port

Adding tag

SebCorbin’s picture

Assigned: pgillis » SebCorbin
Status: Needs work » Needs review
FileSize
5.84 KB

Re-rolled as per #34

amitaibu’s picture

Thanks, minor issues.

+++ b/includes/migrate/OGUR_6000-7200/og_ogur_roles.migrate.incundefined
@@ -0,0 +1,65 @@
+        ->fields('ogur', array('rid'))
+        ->groupBy('rid')
+        ->orderBy('rid', 'ASC')
+        ->execute()->fetchCol();

Wrong indentation, also fetchCol() should be in own line.

+++ b/og.installundefined
@@ -1020,6 +1020,9 @@ function og_update_7200() {
+  // Temporary variable to indicate we need to migreate OGUR data.

migreate => migrate

+++ b/og.moduleundefined
@@ -3542,6 +3542,12 @@ function og_migrate_api() {
+      // OGUR related migrations.

OGUR => OG user roles (OGUR)

SebCorbin’s picture

Re-rolled as per #37

amitaibu’s picture

Status: Needs review » Fixed

Committed, thanks.

amitaibu’s picture

I missed it before committing. Can we move the files under the 7000 directory, and rename them to follow the other files?

SebCorbin’s picture

Status: Fixed » Needs review
FileSize
9.01 KB
amitaibu’s picture

index 0000000..c81e78f
--- /dev/null

--- /dev/null
+++ b/includes/migrate/7200/og_ogur.migrate.incundefined

Ok, last nitpick :) file should be called og_ogur.inc

SebCorbin’s picture

Why is that? Other files in this directory are named og_*.migrate.inc

amitaibu’s picture

Status: Needs review » Fixed

Patch seems wrong? Anyway, I've moved it manually, thanks.

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