Flow.js updates

[justinbeaty]

Description (*)

We use flow.js to upload images in the backend. However, the version in this repo is from Feb 01, 2015.

In this PR:

• Updated flow.js to v2.14.1
• Removed fusty-flow.js -- A JavaScript library which extends Flow.js and allows to use not resumable uploads for older browsers, such as IE7, IE8 and IE9.
• Fixed an error where setting post_max_size or upload_max_filesize to 0 causes the admin dashboard to crash when uploading a file. Ref Image uploader broken? #2432
• Fixed a typo "Disalollowed" -> "Disallowed". This was done in the phtml file but I can change that pending talks in Changing phrases/messages in the locale CSV files but not in the PHP files. #2429

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Image uploader broken? #2432

Manual testing scenarios (*)

1. Pull this PR into your source code
2. Make sure to clear cache and hard refresh the edit product page
3. Try to upload some files, hopefully all should work

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[lgtm-com]

This pull request introduces 1 alert when merging 33cb066 into 71998f5 - view on LGTM.com

new alerts:

• 1 for Useless conditional

[ADDISON74]

LGTM but before I approve it I will test it.

Have you looked in the browser console to check if there are any errors? Also, did you make a comparison between the versions so that there are no surprises that the removed version was one customized by the Magento team?

[justinbeaty]

I do not see any browser errors with the new version.

I did a basic check between versions, you can see the commits here. It mostly looks like bugfixes, a few new features, and a lot of documentation.

There's one thing I want to check here, but I think it's still maybe a local server issue.

Edit: the thing I checked is that it seems 75% of the time when trying to upload a > 2MB file I get a "Disallowed file type" error from here:

magento-lts/app/code/core/Mage/Catalog/Helper/Image.php

Lines 634 to 637 in 33cb066

	$imageInfo = getimagesize($filePath);
	if (!$imageInfo) {
	Mage::throwException($this->__('Disallowed file type.'));
	}

But I reverted to the old flow.js and I still get it... It seems like the server isn't done uploading the file before it's checked. This is likely something in my config somewhere...

[justinbeaty]

Actually, I'm really confused why Magento developers are using flow.js. The point of it is that it can upload files in chunks, but they don't use that functionality at all. Instead they set the chunkSize to the max upload filesize, so flow.js is pointless. They probably used it as a quick upload library when Flash was deprecated.

In any case, the error again seems to come from me setting post_max_size and upload_max_filesize to 0, but I will make the fix better in this PR.

[ADDISON74]

That comparison is valid only if the file used by us has the same content. I will still do a check like I did for ZF1-Future, when I came to the conclusion that even in /lib/Zend the Magento team got their hands up to the elbows.

[justinbeaty]

That comparison is valid only if the file used by us has the same content. I will still do a check like I did for ZF1-Future, when I came to the conclusion that even in /lib/Zend the Magento team got their hands up to the elbows.

Sorry I misunderstood what you asked. It looks like no modifications were made:

$ sha1sum *
bfcbe640f2c3ec26d11c9445b138292d7dc0898a  git-flow.min.js
bfcbe640f2c3ec26d11c9445b138292d7dc0898a  om-flow.min.js

Here is sha1sum of the file in our repo, and the 2.9.0 release from here: https://github.com/flowjs/flow.js/releases/tag/v2.9.0

[justinbeaty]

Fixed the error caused by my config, changes here: https://github.com/OpenMage/magento-lts/compare/33cb0666acfbc2c990951395c82e1a4779e3a37d..9cc750f2a41f54c1f8fafe841723801b33308cb1

Again I am just left scratching my head wondering why Magento developers used flow.js in the first place...

[fballiano]

Again I am just left scratching my head wondering why Magento developers used flow.js in the first place...

let's drop it altogether :-D
maybe for big files?

[justinbeaty]

Again I am just left scratching my head wondering why Magento developers used flow.js in the first place...

let's drop it altogether :-D maybe for big files?

I would like to drop it too, but that's for another PR because it needs much more changes.

For large files I think that's the point of flow.js, but I don't see anywhere the multiple chunks are read in Magento... And Magento is simply setting the chunkSize to the max upload file size, so there would be no chunks anyway (probably because the PHP code doesn't read them...)

[lgtm-com]

This pull request introduces 1 alert when merging 9cc750f into 71998f5 - view on LGTM.com

new alerts:

• 1 for Useless conditional

[ADDISON74]

Many things have not received a response from the Magento team. From the moment the attention and care moved to the M2 until the grand finale, solutions taken at the corner of the table were preferred. I can't forget the Stackoverflow discussions when bugs appeared for each version and the community reacted. It was solved on one side and changed in place to another version. Boredom was clearly seen starting with 1.9.4.0. Openmage was the place where many of the issues found a solution because those who maintained the project gathered information from the Forums, listened to the opinions. I was brought here by Sven who advised me not to post in the Magento Bugtracker so that no one would notice me. To conclude, it is obvious that we only now realize that the replacement of the uploader based on Flash Player was not an afterthought, but was a solution to quickly solve a issue at that moment. I consider that the Magento team knew most of the issues and felt relieved of the burden when the attention was moved to the new version which fails to impress in many chapters, more and more people complain about the complexity of the platform and the speed, but that is another discussion. I appreciate all of you who make efforts to make this project better.

[ADDISON74]

I confirm that the file we have used so far in OM has not been modified by the Magento team, being kept as they took it over.

I tested so far two places:

1. Catalog > Manage Product > Edit > Image tab. Upload Files works as expected.

2. CMS > Manage Pages > Edit > Content tab > [Insert Image...] button. Upload Files works as expected.

Do I need to test other places in Backend?

[ADDISON74]

We will have to periodically update this file or link it to Composer. That repository has a large number of reported issues. As if today you were discussing the issue of uploading large files. Here you go flowjs/flow.js#291.

[justinbeaty]

Eventually this library can go away. But I don’t think there’s any bugs in the new version that weren’t in our version of flow.js to begin with.

[fballiano]

should we add something in the README (in the v19-v20 differences section)?

[tobihille]

I was working on flow.js this week, good to see that you came to the same realization that fusty-flow.js is not used anymore.
I did not review the lib itself but your changes are looking good to me.

[justinbeaty]

I reduced the simultaneous uploads to 1 which I think is fine because it would take longer for a store admin to fix the messed up sorting of images than it would to wait a bit longer for uploads. Thanks @tobihille @tmewes

However, I moved the config changes to app/code/core/Mage/Uploader/Model/Config/Uploader.php. Originally I didn't do this because PHP_INT_MAX is less than Number.POSITIVE_INFINITY but on a 32-bit system it's still 2.1 GB and on a 64-bit system it's practically infinite. This would allow someone to overwrite the Model class to change simultaneousUploads back if they wanted to.

[lgtm-com]

This pull request introduces 1 alert when merging 7a5ead1 into 71998f5 - view on LGTM.com

new alerts:

• 1 for Useless conditional

[justinbeaty]

The LGTM Useless conditional is because I've included the unminified flow.js in case anyone is wondering...

[justinbeaty]

@fballiano @Sdfendor @ADDISON74 thoughts on changing this to 1.9.4.x branch? I guess it's okay but it is somewhat breaking.

[ADDISON74]

I am also testing today's new changes and I will let you know an opinion. I hope you didn't limit uploading just one image, but I can still select as many as I want and upload them all at once. In the past I had a discussion on this topic with the MageWorx team who didn't want/didn't know how to modify the image uploader so that I can add multiple images to an option at once, instead we upload them one by one. Using that extension became a pain and gave up on it.

Regarding the change in 19, honestly speaking, I think we consume too much energy analyzing what would happen if we did one or the other. We don't really have feedback from those who still use it and when issues were reported, the references were mostly to version 20. This is also the version I use in production. If the PR will be accepted by majority in version 19 and we break something, I am sure we will find out. Good that we know more about the existence of this PR.

I must appreciate the contribution of the two today who had very good suggestions and which were adopted. OM needs as many devoted people as possible to get involved.

[justinbeaty]

I am also testing today's new changes and I will let you know an opinion. I hope you didn't limit uploading just one image,

It shouldn't be limited to just one upload. Instead it limits the concurrency to one upload at a time. The issue reported was that several images are uploaded at once and whichever ones got there first would determine the image order.

[ADDISON74]

Thank you for the explanation, everything is clear now.

Maybe we should analyze another issue related to the file loader. If we have 5 images with the names 1.jpg, 2.jpg, ..., 5.jpg and we upload them all at once, the images in the list are not positioned according to the name but according to another criterion. In order to solve the positioning issue I had to allocate time to establish a number for each one. The naming of the files should have solved this issue, an established order of the images. When we have a few it is no misfortune, but when there are 20 we lose 5 minutes scrolling the window content looking for each image.

[tobihille]

@ADDISON74:
Your issue with the order of 1.jpg to 5.jpg is already adressed by setting setSimultaneousUploads(1).
In general your OS will sort the images (at least in my testcases this was true) but the order is shuffeled again by uploading the files all at once.
The request that is handled first will add the first image and so on.
By Setting simultaneous uploads to 1 the issue is resolved because it will only update one image at a time so 1.jpg is uploaded, then (when 1.jpg is finished) 2.jpg and so on...

[ADDISON74]

@tobihille - Thank you. I will do a test because the modification you refer to is in this PR. I have the possibility to upload those files both from Windows and from Ubuntu to analyze what happens in both situations and based on the OS.

[fballiano]

Do we have any idea if, without the 2 removed js, it would still work on older IE versions (also if doesn't support resumable downloads etc)?

[justinbeaty]

@fballiano Fusty flow says it is for supporting "IE7, IE8 and IE9". So I presume we'd be removing support for those browsers. I don't have access to any old IE version to test.

[fballiano]

so I guess it has to stay on v20 then

[fballiano]

i can't test it right now cause I'm in the middle of another PR, will do ASAP

[fballiano]

tested

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  7 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 5aece6d. ± Comparison against base commit 71998f5.