Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
2.88 KB

Here is a locally untested patch that addresses the above.

Status: Needs review » Needs work

The last submitted patch, 1813184-1-tracker-index-nid.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
2.57 KB

Looks like misspelling tracker_update_8002() does not help... Here is a revised patch.

Status: Needs review » Needs work

The last submitted patch, 1813184-3-tracker-index-nid.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Missed the use in TrackerTest.php... Included in this patch.

Berdir’s picture

Status: Needs review » Needs work

Update function needs to be updated to use the new helper, but looks good otherwise.

brandenlhamilton’s picture

Status: Needs work » Needs review
FileSize
3.36 KB

#6. The patch included in #5 is actually converting the variable to the State API, and uses a different upgrade path from the Config API. However, I did notice that the variable_del() function was still being used in an hook_uninstall implementation. Attached patch removes the hook as well as the function call.

Berdir’s picture

Status: Needs review » Needs work

Yes, and there is a helper function to convert state variables now too, look for update_variables_to_state() :) That document needs to be updated to reflect that...

We should also add an upgrade test, see StateSystemUpgradePathTest, you just need to add it to drupal-7.state.system.database.php and that test class.

brandenlhamilton’s picture

Status: Needs work » Needs review
FileSize
4.56 KB

@Berdir thanks for giving me the name of the new helper function. For reference, the new function can be found at api.drupal.org. Nevertheless, for completeness, I also updated the original documentation page that I referenced in #7.

I took your suggestion, and added the tracker.index_nid to the upgrade test in StateSystemUpgradePathTest.php and drupal-7.state.system.database.php. Please let me know if I was mistaken in that interpretation.

The attached patch includes the updated state system test, as well as the changes implemented in #7.

Status: Needs review » Needs work

The last submitted patch, drupal-tracker_index_nid-1813184-9.patch, failed testing.

brandenlhamilton’s picture

Status: Needs work » Needs review
FileSize
4.56 KB

Seems my branch was a bit stale. Fetch/Merge latest and generate new patch.

Status: Needs review » Needs work

The last submitted patch, drupal-tracker_index_nid-1813184-10.patch, failed testing.

brandenlhamilton’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

Ack! Trying again.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/tracker/tracker.installundefined
@@ -6,19 +6,12 @@
 /**
- * Implements hook_uninstall().
- */
-function tracker_uninstall() {
-  variable_del('tracker_index_nid');

Shouldn't this delete the value from state now?

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.7 KB

Yes, it should. Thanks :)

Berdir’s picture

#16: tracker-index-nid-1813184-16.patch queued for re-testing.

longwave’s picture

The most nitpicky thing I think I've ever posted:

+  ->key( array('name' => 'tracker_index_nid'))

There is an unnecessary space after key(

aspilicious’s picture

Status: Needs review » Needs work

Needs a reroll based on #18

ACF’s picture

Status: Needs work » Needs review
FileSize
4.7 KB

Just a reroll of berdir's patch with tiny fix.

Status: Needs review » Needs work

The last submitted patch, 1813184-tracker_index_to_state-drupal8-20.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1813184-tracker_index_to_state-drupal8-20.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
Lars Toomre’s picture

Assigned: Lars Toomre » Unassigned
longwave’s picture

Status: Needs review » Needs work

The function comment for tracker_cron() needs updating, as it still mentions the 'tracker_index_nid' variable.

ACF’s picture

Status: Needs work » Needs review
FileSize
5.17 KB

updated comment.

Status: Needs review » Needs work

The last submitted patch, 1813184-tracker_index_to_state-drupal8-27.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looking good, will retest to verify

aspilicious’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.