Problem/Motivation

Currently this module purges all outdated webform submissions at once. As it does this via the webform_submission_delete() API method while also using node_load() and webform_menu_submission_load() during the process, this will not work on sites that have thousands or millions of submissions to be purged.

Proposed resolution

  • Add another option in the settings form to limit the number of purged submissions to 10, 100, 1000, 10000, 100000
  • Rewrite the webform_purge_cron() implementation to respect this limit when purging webform submissions

Remaining tasks

  • Provide a patch

User interface changes

  • Settings form will be extended with a new option

API changes

n/a

Data model changes

  • New variable for maximum number of purged webform submissions will be introduced
CommentFileSizeAuthor
#8 webform-purge-cron-2959844-8.patch2.94 KBstborchert
#6 weborm_purge_cron-2959844-6.patch2.78 KByogeshmpawar
#2 weborm-purge-cron-2959844-2.patch1.83 KBhctom

Comments

hctom created an issue. See original summary.

hctom’s picture

hctom
Primary language German
commented
Assigned: hctom » Unassigned
Status: Active » Needs review
StatusFileSize
new weborm-purge-cron-2959844-2.patch1.83 KB

Here is a patch that enables the cron limit functionality.

CAUTION: This patch only applies when patch from #2937269-6: Cannot load admin form because of incorrect query is applied first!

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

How does this work in combination with the "webform_purge_run_once" setting?

hctom’s picture

hctom
Primary language German
commented

It should work as normal, because the "run once" setting only limits the number of runs per day, but not the number of records to check/delete.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
StatusFileSize
new weborm_purge_cron-2959844-6.patch2.78 KB

@hctom, currently you patch failed to apply on 7.x-1.x branch & while setting the "Maximum number of purged submissions" with your patch "webform_purge_cron_limit" value not set after form submit. so i have added form_submit in my patch to overcome this problem. Also i have reduced the "limit the number of purged submissions" from "100000" to "50000" because when we run for a "100000" cron fails because of memory allocations issue. so i reduced this size for maximum number of purged submissions.
Please feel free to review my patch.

hctom’s picture

hctom
Primary language German
commented
Status: Needs review » Needs work

@yogeshmpawar sorry for the late reply, but here is my feedback:

  • I'm confused why my patch does not apply to the 7.x-1.x branch, because, that is the branch I created it from. Did you read my comment, that you have to first apply the patch from #2937269-6: Cannot load admin form because of incorrect query?
  • Saving of variable values is handled via Drupal core's system_settings_form() wrapper function, so there shouldn't be a custom form submit handler to be necessary - did you try the module with my patch on a clean Drupal install? I guess it will work there and in there is something else interfering in your project. Also, in our project's the value saving is working perfectly without the need of another submit handler
  • Please leave the 100000 limit value as is, because it may work on other person's projects - and it is only an option, where you can decide on your own, whether to use it or not. Perhaps the 50000 should be added but not replace the 100000

... and last but not least the most important thing: Please always provide an interdiff (see https://www.drupal.org/documentation/git/interdiff) for a new patch outlining the changes between the old and the new patch. Otherwise it is almost impossible to see what changed in your patch.

Therefore I will hide your patch file for now, but feel free to provide a new/improved one with all my feedback applied.

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new webform-purge-cron-2959844-8.patch2.94 KB

Re-roll of patch in comment #2 against latest dev.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs review » Needs work

I would suggest adding a “no limit” option and an update hook that sets the default value to that “no limit” option so the module keeps working as before for users that don’t need the limit option.

  • JeroenT committed 1ec24ff on 7.x-1.x authored by stBorchert 
    Issue #2959844 by hctom, yogeshmpawar, stBorchert, JeroenT: Allow...
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Fixed

Committed and pushed to 7.x-1.x. Thanks!

Status: Fixed » Closed (fixed)

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