Fix for race condition while uploading multiple files with uploadimage plugin #1216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comandeer
requested changes
Nov 23, 2017
| @@ -68,6 +68,8 @@ | |||
|
|
|||
| this.responseData = {}; | |||
| }; | |||
|
|
|||
| // sinon.spy( CKEDITOR.fileTools.fileLoader.prototype, 'upload' ); | |||
There was a problem hiding this comment.
This line could probably be deleted safely.
| wait(); | ||
| }, | ||
|
|
||
| 'test uploads generate unique names (#1213)': function() { |
There was a problem hiding this comment.
Test is always failing:
TypeError: Attempted to wrap create which is already wrapped
There was a problem hiding this comment.
Ooops, TBH don't know how I missed that.
| // Returns an unique image file name. | ||
| function getUniqueImageFileName( type ) { | ||
| var date = new Date(), | ||
| dateParts = [ date.getFullYear(), date.getMonth() + 1, date.getDate(), date.getHours(), date.getMinutes(), date.getSeconds() ]; |
There was a problem hiding this comment.
Couldn't we use simply timestamp here, returned from Date.now()/Date.prototype.getTime()?
There was a problem hiding this comment.
We want to give it some human readability, at the same time not making the name too long. So we'd like to stick to YYYYmmddhhmmss schema.
Comandeer
approved these changes
Nov 23, 2017
…files having the same name.
…imes. Also removed commented code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
Bug fix
This PR contains
What changes did you make?
This issue is more of a workaround, as the problem is actually on a CKFinder side, but we can't quickly fix it there, so we'll patch client side first.
Now, the problem was that CKFinder had a problem handling multiple files with the same name loaded at once, so we'll generate unique file name for each one. It's a bit dirty, and optimistic, but should do the trick for now.
Manaul Tests
To be honest I had no good idea how to make a good manual test here. To make it reliable I have set a CKFinder 3 instance on my dev workstation, and added configuration to
config.js:By adding:
And adding
'uploadimage'to the plugin list.Closes #1213.