Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Scott switched to using the batch api as it works around some memory issues caused by the PHPExcel libraries. This needs to be pulled into the main project
Comment | File | Size | Author |
---|---|---|---|
#21 | commerce_xls_import-backport-2609430-21.patch | 911 bytes | kfitz |
#14 | commerce_xls_import-backport_batchapi-2609430-14.patch | 22.46 KB | smccabe |
Comments
Comment #2
TimRutherford CreditAttribution: TimRutherford commentedBackported Scott's changes from Background process to Batch API from the Vault project. I tried to keep the code structure from the dev branch's version as much as possible.
An unresolved issue arouse from using this Batch API though. When it creates more than 1 batch it seems to reset the CommerceXlsImportVariables.inc static class. I couldn't find a solution to this issue but I added a simple work-around and it seems to work fine. I removed the check to see if either CommerceXlsImportVariables's validate or import are true from the commerce_xls_import_phpexcel_export() function (either validate or import are set to true normally so it would always return true). This issue might have some side-effects else where in the code, but the main functionality still works.
Comment #3
TimRutherford CreditAttribution: TimRutherford commentedRerolled old patch and updated it to remove background_process dependency.
Comment #4
kfitz CreditAttribution: kfitz at Acro Commerce commentedComment #5
kfitz CreditAttribution: kfitz at Acro Commerce commentedI tested import functionality with 200 orders without applying the patch and everything seemed to have worked fine. Also, I'm getting the following error when attempting to apply the patch:
patching file classes/CommerceXlsImportReadFilter.inc
patching file commerce_xls_import.info
Hunk #2 FAILED at 10.
1 out of 2 hunks FAILED -- saving rejects to file commerce_xls_import.info.rej
patching file commerce_xls_import.module
patching file includes/commerce_xls_import.admin.inc
Comment #6
kfitz CreditAttribution: kfitz at Acro Commerce commentedComment #7
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commentedPatch applies fine for me.. not sure why your getting that error @kfitz. Just need to do some review and get a test import going.
Comment #8
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commentedPatch doesn't throw any errors when applying, but it creates some issues in the commerce_xls_import.module where lines don't get placed correctly. Manually fixed them following the patch review. Going to run a test import now.
Comment #9
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commentedGetting an ajax error during batch import..beginning to debug.
Comment #10
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commentedError came from class not found CommerceXlsImportReadFilter, when applying the patch it created and placed classes/CommerceXlsImportReadFilter in the root directory of my drupal build. after placing it in its rightful spot the batch import works. doing a Code review now.
Comment #11
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commentedCode looks good. Leaving as "needs work" until I know why the patch didn't apply clean.
Comment #12
smccabe CreditAttribution: smccabe at Acro Commerce commentedpatch just needed a reroll as it was done against the code from 6 months ago.
https://www.drupal.org/patch/reroll
Comment #13
smccabe CreditAttribution: smccabe at Acro Commerce commentedA few things needed updating after the reroll, lets see if this patch passes properly.
Comment #14
smccabe CreditAttribution: smccabe at Acro Commerce commentedRemoved some testing that doesn't work with d.o since we can't download the libraries
Comment #16
smccabe CreditAttribution: smccabe at Acro Commerce commentedComment #20
smccabe CreditAttribution: smccabe at Acro Commerce commentedLol, committing this patch caused the tests to finally pass fully, which then caused all the old pending tests to finally run and then of course fail to apply since their code was already committed.
Comment #21
kfitz CreditAttribution: kfitz at Acro Commerce commentedAdds patch to include the 'CommerceXlsImportReadFilter' class. I don't think the class was added to the patch that was committed into the dev branch.
Comment #22
kfitz CreditAttribution: kfitz at Acro Commerce commentedComment #24
smccabe CreditAttribution: smccabe at Acro Commerce commentedCommited, can you confirm everything is working and if so, set this to fixed.
Comment #25
smccabe CreditAttribution: smccabe at Acro Commerce commentedWorks fine for me in testing, closing.