New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't add new files after saving initial upload(s) #18

Closed
johannahoerrmann opened this Issue Oct 17, 2014 · 18 comments

Comments

Projects
None yet
4 participants
@johannahoerrmann

johannahoerrmann commented Oct 17, 2014

This is a quite annoying but nonetheless very interesting bug I encountered when trying to add more files to an already populated field (running 2.3.3RC3). This bug seems to already have been reported by @nathanhornby in #9, but in my case, it is not linked to wether a file was uploaded before or not as it happens in both cases.

Here's my step by step and what happens:

  • add 1 or more files, save
  • add another file, save
  • error: „Some errors were encountered while attempting to save.“
  • field doesn’t show any entries anymore and shows no error

a:

  • save again
  • no errors
  • field is empty (files are not deleted from my media folder though)

b:

  • abort, go to overview, re-open entry
  • the initally uploaded files are shown in field, first file throws an error: „The file, …, is no longer available. …“ Confirm: Yes, it indeed got deleted from my media folder.
  • remove file, save
  • the next file now throws the error: „The file, …, is no longer available. …“ Again, it got deleted from my folder.
  • this goes on until all initially uploaded files got deleted.

Is anyone able to help? Thanks a lot!

@brendo

This comment has been minimized.

Show comment
Hide comment
@brendo

brendo Oct 27, 2014

Member

Just to assist debugging, does the probably still exist in the latest version of Symphony? Or just in 2.3.3?

Member

brendo commented Oct 27, 2014

Just to assist debugging, does the probably still exist in the latest version of Symphony? Or just in 2.3.3?

@nathanhornby

This comment has been minimized.

Show comment
Hide comment
@nathanhornby

nathanhornby Oct 27, 2014

@brendo 2.5 here (couple of sites).

nathanhornby commented Oct 27, 2014

@brendo 2.5 here (couple of sites).

@nilshoerrmann

This comment has been minimized.

Show comment
Hide comment
@nilshoerrmann

nilshoerrmann Oct 28, 2014

Member

The question is: Does the extension send the wrong data when the user hits save (that would be a scripting issue) or does the extension handle that data incorrectly after sending it to the server (that would be a PHP issue).

Member

nilshoerrmann commented Oct 28, 2014

The question is: Does the extension send the wrong data when the user hits save (that would be a scripting issue) or does the extension handle that data incorrectly after sending it to the server (that would be a PHP issue).

@nilshoerrmann

This comment has been minimized.

Show comment
Hide comment
@nilshoerrmann

nilshoerrmann Oct 28, 2014

Member

Looking at most of the open issues (#9, #12) saving in general seems to be buggy.

Member

nilshoerrmann commented Oct 28, 2014

Looking at most of the open issues (#9, #12) saving in general seems to be buggy.

@brendo

This comment has been minimized.

Show comment
Hide comment
@brendo

brendo Oct 30, 2014

Member

Saving and removing files is indeed buggy, especially if you don't do everything 'all at once'. Doing partial actions, whether that be save or delete is messy at the moment and removes/keeps the wrong files. This is going to be a challenge as the duplicator index order does not reflect the order of the files in the backend. At this stage, I don't know how to resolve this.

Member

brendo commented Oct 30, 2014

Saving and removing files is indeed buggy, especially if you don't do everything 'all at once'. Doing partial actions, whether that be save or delete is messy at the moment and removes/keeps the wrong files. This is going to be a challenge as the duplicator index order does not reflect the order of the files in the backend. At this stage, I don't know how to resolve this.

@nilshoerrmann

This comment has been minimized.

Show comment
Hide comment
@nilshoerrmann

nilshoerrmann Oct 30, 2014

Member

Why is the index needed?

Member

nilshoerrmann commented Oct 30, 2014

Why is the index needed?

@nilshoerrmann

This comment has been minimized.

Show comment
Hide comment
@nilshoerrmann

nilshoerrmann Oct 30, 2014

Member

I've been taking a deeper look at the script and we are storing the filename in the entries markup. So if we save the entry, we'll send a list of all attached files by name. If we compare that list of files with the old one, shouldn't we get a distinct list of the files to be saved and the files to be removed?

Member

nilshoerrmann commented Oct 30, 2014

I've been taking a deeper look at the script and we are storing the filename in the entries markup. So if we save the entry, we'll send a list of all attached files by name. If we compare that list of files with the old one, shouldn't we get a distinct list of the files to be saved and the files to be removed?

@brendo

This comment has been minimized.

Show comment
Hide comment
@brendo

brendo Oct 30, 2014

Member

Yes, we can and we have to do something more than what exists currently as it's simply not up to task.

  • If you have 3 images, and you remove the 2nd, currently the extension drops the 3rd.
  • If you add an image after the original entry creation, the upload.php script will move the file into place, but it won't add any database entries, so the script thinks the file shouldn't exist and freaks out.

All the processing needs to be reviewed to handle multiple uploads, partial uploads (remembering old), partial uploads and removals, removals, replacement.

The workaround for all of the issues at the moment is to to everything at once. It's a bad workaround though.

Member

brendo commented Oct 30, 2014

Yes, we can and we have to do something more than what exists currently as it's simply not up to task.

  • If you have 3 images, and you remove the 2nd, currently the extension drops the 3rd.
  • If you add an image after the original entry creation, the upload.php script will move the file into place, but it won't add any database entries, so the script thinks the file shouldn't exist and freaks out.

All the processing needs to be reviewed to handle multiple uploads, partial uploads (remembering old), partial uploads and removals, removals, replacement.

The workaround for all of the issues at the moment is to to everything at once. It's a bad workaround though.

@brendo

This comment has been minimized.

Show comment
Hide comment
@brendo

brendo Oct 30, 2014

Member

If you can give the integration branch a little test, that'd be magic.

Member

brendo commented Oct 30, 2014

If you can give the integration branch a little test, that'd be magic.

@nathanhornby

This comment has been minimized.

Show comment
Hide comment
@nathanhornby

nathanhornby Oct 30, 2014

Amazing, thanks @brendo I'll give it a spin when get the chance!

nathanhornby commented Oct 30, 2014

Amazing, thanks @brendo I'll give it a spin when get the chance!

@johannahoerrmann

This comment has been minimized.

Show comment
Hide comment
@johannahoerrmann

johannahoerrmann Nov 4, 2014

Okay, so I've given the integration branch a test. Much better, thanks! I've come across the following problem though. When deleting and adding files in my field, the changes are only saved, if the total count of files is different from what it was before:

  • delete 1 old, add 1 new, save = changes are lost, field shows the entries I originally started with
  • delete 1 old, add 2 new, save = changes are saved
  • delete 2 old, add 1 new, save = changes are saved

I guess this is fairly easy to solve though :)

johannahoerrmann commented Nov 4, 2014

Okay, so I've given the integration branch a test. Much better, thanks! I've come across the following problem though. When deleting and adding files in my field, the changes are only saved, if the total count of files is different from what it was before:

  • delete 1 old, add 1 new, save = changes are lost, field shows the entries I originally started with
  • delete 1 old, add 2 new, save = changes are saved
  • delete 2 old, add 1 new, save = changes are saved

I guess this is fairly easy to solve though :)

@nilshoerrmann

This comment has been minimized.

Show comment
Hide comment
@nilshoerrmann

nilshoerrmann Nov 4, 2014

Member

Seems like this check is not sufficient (is it needed at all?):

if (count($existing_files) !== count($data))

See: aba3f84#diff-f69e70bc608da37723b4003bc430abb0R144

Member

nilshoerrmann commented Nov 4, 2014

Seems like this check is not sufficient (is it needed at all?):

if (count($existing_files) !== count($data))

See: aba3f84#diff-f69e70bc608da37723b4003bc430abb0R144

@nathanhornby

This comment has been minimized.

Show comment
Hide comment
@nathanhornby

nathanhornby Nov 4, 2014

(previous message deleted) Sorry I see why you stated that now @nilshoerrmann - it does look somewhat redundant, like an initial check to see if it needs to check for changes - but I don't think it's needed, as you say.

nathanhornby commented Nov 4, 2014

(previous message deleted) Sorry I see why you stated that now @nilshoerrmann - it does look somewhat redundant, like an initial check to see if it needs to check for changes - but I don't think it's needed, as you say.

@nathanhornby

This comment has been minimized.

Show comment
Hide comment
@nathanhornby

nathanhornby Dec 3, 2014

It seems we're not far off having a working version of this extension - what more needs to be done on integration?

I'm struggling to find anything that's still maintained that handles more than one file - I consider this pretty crucial functionality (IMO core functionality), I'm sure I can't be the only one…

nathanhornby commented Dec 3, 2014

It seems we're not far off having a working version of this extension - what more needs to be done on integration?

I'm struggling to find anything that's still maintained that handles more than one file - I consider this pretty crucial functionality (IMO core functionality), I'm sure I can't be the only one…

@brendo

This comment has been minimized.

Show comment
Hide comment
@brendo

brendo Dec 5, 2014

Member

Heya all, yeah reading back on this thread it looks like that check which @nilshoerrmann highlighted causes @johannahoerrmann's first use case to fail. Removing it should be fine, as there is a check on Line 147 to actually see if it was a part of the previous Entry.

After this it looks like most functionality is good to go in this extension. I'll give it a check over on the integration branch, but looks like we could have a release :)

Member

brendo commented Dec 5, 2014

Heya all, yeah reading back on this thread it looks like that check which @nilshoerrmann highlighted causes @johannahoerrmann's first use case to fail. Removing it should be fine, as there is a check on Line 147 to actually see if it was a part of the previous Entry.

After this it looks like most functionality is good to go in this extension. I'll give it a check over on the integration branch, but looks like we could have a release :)

@nilshoerrmann

This comment has been minimized.

Show comment
Hide comment
@nilshoerrmann

nilshoerrmann Dec 5, 2014

Member

Without that line everything worked as expected on our site.

Member

nilshoerrmann commented Dec 5, 2014

Without that line everything worked as expected on our site.

@nathanhornby

This comment has been minimized.

Show comment
Hide comment
@nathanhornby

nathanhornby Dec 5, 2014

Sweet!

I haven't run into any issues on the integration branch myself either - but haven't been stress testing it so glad to hear that @nilshoerrmann hasn't had any issues.

nathanhornby commented Dec 5, 2014

Sweet!

I haven't run into any issues on the integration branch myself either - but haven't been stress testing it so glad to hear that @nilshoerrmann hasn't had any issues.

@brendo brendo closed this in b26c7cf Dec 6, 2014

@brendo

This comment has been minimized.

Show comment
Hide comment
@brendo

brendo Dec 6, 2014

Member

Done and released as 1.4. Note that if you are running Symphony 2.6, you'll still require the integration branch of this field due to 7d373b7

Member

brendo commented Dec 6, 2014

Done and released as 1.4. Note that if you are running Symphony 2.6, you'll still require the integration branch of this field due to 7d373b7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment