sdboyer committed one patch some little time ago.

I just notice some fixes to do, so I create this, and also because that commit do not refer to an issue.

What is pending:

- There is a new vc_op_id field on the {versioncontrol_item_revisions} table(coming from the original {versioncontrol_operation_items} table), but the VersioncontrolOperation class do not have a vc_op_id data member.
- The operation class is not handling correctly the pass of the recently-created vc_op_id at VersioncontrolOperation::insert().
- The commit mentioned also add logic to versioncontrol_update_6303(), but it only wants to add a new hook_update_N() function, so we need to remove that code(IIRC I talked to Sam about this item but neither him nor me committed a fix for it :-p).

I realized this trying to fix #965930: Get the log parser finished. Really.

Preparing the patch

CommentFileSizeAuthor
#7 0001-bug-972058-follow-up-Complete-removing-of-versioncon.patch14.88 KBmarvil07
#1 0001-bug-972058-Fixed-Complete-removing-of-versioncontrol.patch2.19 KBmarvil07

Comments

marvil07’s picture

marvil07 commented
Component: Commit Log » API module
Status: Active » Needs review
StatusFileSize
new 0001-bug-972058-Fixed-Complete-removing-of-versioncontrol.patch2.19 KB

The patch :-)

To test this it try to import a repo with git backend, then see that vc_op_id is always 0 at {versioncontrol_item_revisions}, and after the patch it have the right value.

@sdboyer: if you can review the patch it would be great.

marvil07’s picture

marvil07 commented
Status: Needs review » Needs work

Mmm, we need a bigger patch.

$ git  grep -n versioncontrol_operation_items
includes/VersioncontrolOperation.php:173:      FROM {versioncontrol_operation_items} opitem
includes/VersioncontrolOperation.php:399:    db_query('DELETE FROM {versioncontrol_operation_items}
includes/VersioncontrolOperation.php:484:   * Insert an operation item entry into the {versioncontrol_operation_items} table.
includes/VersioncontrolOperation.php:489:    db_query("DELETE FROM {versioncontrol_operation_items}
includes/VersioncontrolOperation.php:493:    db_query("INSERT INTO {versioncontrol_operation_items}
includes/views/handlers/versioncontrol_handler_field_operation_items.inc:44:                          FROM {versioncontrol_operation_items} voi
includes/views/versioncontrol.views.inc:283:    'versioncontrol_operation_items' => array(
includes/views/versioncontrol.views.inc:288:      'left_table' => 'versioncontrol_operation_items',
includes/views/versioncontrol.views.inc:485:  $data['versioncontrol_operation_items']['table']['group'] = t("VersionControl Item Revisions");
includes/views/versioncontrol.views.inc:486:  $data['versioncontrol_operation_items']['table']['join'] = array(
includes/views/versioncontrol.views.inc:496:  $data['versioncontrol_operation_items']['item_revision_id'] = array(
includes/views/versioncontrol.views.inc:504:  $data['versioncontrol_operation_items']['type'] = array(
versioncontrol.install:715: * Remove the versioncontrol_operation_items table, as it was an unnecessary
versioncontrol.install:728:  $result = db_query("SELECT vc_op_id, item_revision_id FROM {versioncontrol_operation_items}");
versioncontrol.install:732:    db_query('UPDATE {versioncontrol_operation_items} SET vc_op_id = %d WHERE item_revision_id = %d', $row->vc_op_id, $row->item_revision_id);
versioncontrol.install:735:  db_drop_table($ret, 'versioncontrol_operation_items');
versioncontrol.module:45: * items get directly included in the {versioncontrol_operation_items} table.)
versioncontrol.module:876:  // in the {versioncontrol_operation_items} table.
versioncontrol.module:915:      FROM {versioncontrol_operation_items}
sdboyer’s picture

sdboyer commented
Status: Needs work » Fixed

God I hate trying to keep everything up to date with a patch-based workflow. Thanks for catching this, clearly I neglected to commit the second half of what I have locally.

I have to nitpick on your coding standards, though:

   /**
+   * Operation identifier.
+   *
+   * @var    int
+   */
+  public $vc_op_id;

should be

   /**
+   * Operation identifier.
+   *
+   * @var int
+   */
+  public $vc_op_id;

One space. I've noticed this on most of the stuff you add, so I dunno if it's what your IDE does, or what, but...

In any case, yep, committed.

marvil07’s picture

marvil07 commented
Status: Fixed » Needs work

Thanks for committing the change with the formatting issues fixed(it was an ancient inherit from dia2code :-p, my vim does not do anything special), but this still needs work as the grep output mentioned in #2.

sdboyer’s picture

sdboyer commented
Status: Needs work » Fixed

Tackling the VersioncontrolOperation stuff from the grep output in #2 via this issue is not the right approach - it's legacy-ish code, which is being refactored as part of other issues, and we'd end up keeping everything open if we were waiting for all of that. The views stuff I fixed in a separate commit already, and I've been intentionally leaving those broken functions in versioncontrol.module more or less as a reminder that we need to address them. They're going to need to be addressed as part of some further refactoring of how items work.

marvil07’s picture

marvil07 commented
Status: Fixed » Needs work

changing the status as I will be providing a patch for this excluding views as it seems to be fixed somewhere

marvil07’s picture

marvil07 commented
Status: Needs work » Needs review
StatusFileSize
new 0001-bug-972058-follow-up-Complete-removing-of-versioncon.patch14.88 KB

Here the patch, it would be great to receive a review here.

    bug #972058 follow-up: Complete removing of {versioncontrol_operation_items()} table.
    
    - Remove unused versioncontrol_fetch_item_commit_operations()
    - Fix VersioncontrolOperation::getItems()
    - Remove VERSIONCONTROL_OPERATION_{MEMBER_ITEM,CACHED_AFFECTED_ITEM} idea.
      That is not using controllers, so remove it and let controllers deal with
      optimization.
    - item_revisions instead of operation_items on VersioncontrolOperation
      class.
sdboyer’s picture

sdboyer commented
Status: Needs review » Reviewed & tested by the community

Good to go. But if versioncontrol_fetch_item_commit_operations() gets removed, then we need another issue to be opened for recreating it elsewhere. We might decide we don't want and close it later, and that'd be fine, but if the function is gone from the code then we need an issue as a reminder.

sdboyer’s picture

sdboyer commented

Committed this, so that I could incorporate it into the changes in #879858: Unify entity C(R)UD . Follow-up posted at #973922: Reintroduce versioncontrol_fetch_item_commit_operations() behavior.

marvil07’s picture

marvil07 commented
Status: Reviewed & tested by the community » Fixed

Ok

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 4

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

Issue tags: +git phase 2, +git sprint 4

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

  • Commit 0a91643 on repository-families, drush-vc-sync-unlock by sdboyer: 
    Issue #972058 by marvil07: Finish removal of {...

  • Commit 0a91643 on repository-families by sdboyer: 
    Issue #972058 by marvil07: Finish removal of {...