A few ideas to improve the simplytest import module process (given only limited research of the inner workings of yet):

  1. Add the projects list XML as a Library in the projects makefile so that when building a fresh copy of the simplytest.me platform you will at least have the file ready to go.
  2. Have the file downloaded and imported during the installation of the simplytest profile
  3. Have the file downloaded automatically during the import process

That's basically the gist of it. After writing the suggestions I suspect that the second and the third would be the best options, however the first may help with the second to speed up the install process.

CommentFileSizeAuthor
#10 import_processes-1845474-10.patch10.75 KBdeciphered
#10 interdiff.txt5.94 KBdeciphered
#7 import_processes-1845474-7.patch9.93 KBdeciphered
#7 interdiff.txt6.49 KBdeciphered
#4 import_processes-1845474-4.patch6.6 KBdeciphered

Comments

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented

Given the size of the file it's unlikely the the automated download of 2 and 3 is a reality, ideally update.drupal.org could serve the information in smaller blocks, so that one call could be made to provide all the split up files and then each file could be downloaded inside of a batch so that it wasn't trying to download a 12mb file in one hit.

Perfect world....

In the meantime I think I can make an 'experimental' alternative to download the file with a very hight timeout set, but without the feedback of the file being downloaded it will be a bit of a pain to use.

patrickd’s picture

patrickd commented

Yes, I also don't think that the third one is really implementable, it's just too big.

Point 1 and 2 would be a way to go

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented

Actually, I've made some decent progress, while it can't use drupal_http_request (until D8), it is working well enough, still will mark it as experimental, but I'll have a patch hopefully in the next few hours.

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Status: Active » Needs review
StatusFileSize
new import_processes-1845474-4.patch6.6 KB

Patch #1:

- Added two additional import methods: Upload and Download
- Reworked the batch operation.

patrickd’s picture

patrickd commented
Status: Needs review » Needs work

Looks good!

Importing by file path fails

Notice: Undefined variable: file in simplytest_import_form_submit() (line 116 of /home/patrickd/www/simplytest.me/profiles/simplytest/modules/simplytest_import/simplytest_import.module).
Notice: Undefined variable: file in simplytest_import_form_submit() (line 122 of /home/patrickd/www/simplytest.me/profiles/simplytest/modules/simplytest_import/simplytest_import.module).

As the case for this import method is not handled yet

Importing by upload fails, nothing happens.
file_save_upload() should be moved into form validation.

Importing by download, works! :)

Best practice stuff like
- deleting used variables
- checking for curl as requirement,

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented

Hmm, odd that the Import by upload didn't work, as I tested that one locally, could be a permission thing, and yes it absolutely should be in validate, that did occur to me.

Admittedly I didn't test the path based import, shouldn't make assumptions.

Otherwise, yes, the other changes make sense as well.

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt6.49 KB
new import_processes-1845474-7.patch9.93 KB

Updated patch:

- Added simply uninstall function.
- Added curl dependency check, will only show download method when curl is available.
- Added import batch to the profile install.
- Added projects data as a Library and added .gitignore to ignore it.
- Improved upload method, no longer using file_save_upload as it was writing a record to the database for the file.

I have this locally in a feature branch, happy to push that to D.o, or just merge it in to master when you're happy with the patch.

patrickd’s picture

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

Nice!

Some nitpick:

The installation task in simplytest.install always requires the xml downloaded by the makefile.
If it's not available it will just import nothing? Showing the import form in this case, would probably be better here..
'file' => drupal_get_path('module', 'simplytest_import') . '/simplytest_import.module',
simplytest_projects_xml_insert_project() is in simplytest_projects - Is this already included?

I think disabling the download method and describing why, instead of completely hiding would be better

#44: [...]tic download (expirimental)[...]

The problem that nothing happens on the upload method is indeed a problem of my configuration - the file is simply too large

ultra nitpick:
"temporary://{$_FILES['files']['name']['upload']}", <-- I don't like this stuff
if (!$file = file_unmanaged_copy($_FI[...] <-- I don't like this stuff
t('[..]<br /><em><strong>Note:</strong>[...].</em>'); <-- I don't like this stuff
simplytest_import_batch_operation_process_xml() <-- could use some more comments

But here's nothing you could not fix within 5mins, or anything too important..
This can be merged into 7.x-1.x

Thanks a lot!! :)

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Status: Reviewed & tested by the community » Needs work

Actually, that first point isn't a nitpick, it's a good solid bug, it should at the least check for the presence of the file before adding the install task.

Agreed on the disabling of the download method, that way at least the user gets to know that that additional method would be usable if they had CURL enabled.

As for the ultra nitpick, it's definitely a style thing, as I do like those things :), but I have no issue respecting your styling when possible, it's just that it won't always come naturally as habits are hard to break.

Though regarding the html tags in the translation string, I was against that for a while until I was proven that it's actually in the Drupal coding standards that they are meant to be included in the translation string. Of course if you're just saying you don't want to have the styling tags at all, that's another matter.

I'll have another crack at finishing up this issue when I finish work for the day (in about 4 hours).

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt5.94 KB
new import_processes-1845474-10.patch10.75 KB

Ok, this time it should be good to go.

Changes include:
- Download radio button will be disabled with a brief description if cURL isn't available.
- If projects.xml isn't in the Libraries directory, don't add the import task in the profile (and a todo note for future import improvements in the profile install UI)
- Style changes.

Note:
Something related to my merge of the 7.x-1.x into my feature branch messed with the generation of the interdiff.txt, so please disregard any of the things in there unrelated to the issue, they aren't in the actual patch itself.

patrickd’s picture

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

Oh you can ignore most of my ultra nitpick, as long as the code is understandable and documented you can make use of any yoda-condition you want ;-)

One small issue:

    $context['sandbox']['max'] = count($xml->project);  <------------------ if you want that here
    $context['sandbox']['count'] = $defaults['count'];
    $context['sandbox']['index'] = 0;
  }

  // Load the projects data as a SimpleXML object.
  $xml = simplexml_load_file($file);              <------------------------------- you can't move this here

Just swapping these lines back solves the problem, after this small change it's ready to go for me

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented

Oopps, that was a completely last minute change for sake of code cleanup and documentation, can't believe I missed that...

Will fix before I commit (after I test properly :-) )

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Status: Reviewed & tested by the community » Fixed

Committed.

Follow up task: #1847402: Update README.txt for updated import process.

patrickd’s picture

patrickd commented
Status: Fixed » Closed (fixed)

cleaning up