I made these fix to be able to use the multiple attach file.

[lucasfevi]

Using the full filename in that fourth condition didn't work if I had to send a file with a different extension.

[krolow]

hey @lucasfevi may you explain a bit better your scenario that you are facing, what is exactly the issue that happend

[lucasfevi]

Here is my $actsAs:

public $actsAs = array(
        'Attach.Upload' => array(
            'Attach.type' => 'Imagick',
            'product' => array(
                'dir' => 'webroot{DS}media{DS}uploads{DS}images',
            ),
            'product_photo' => array(
                'dir' => 'webroot{DS}media{DS}uploads{DS}products',
                'thumbs' => array(
                    'thumb' => array(
                        'w' => 66,
                        'h' => 90,
                    ),
                ),
                'multiple' => true
            ),
        )
    );

Returned data from the find() line 533:
1º product_photo - find returns empty
2º product_photo - find returns the previously one
3º product_photo - find returns the previously one
...

That happens because the foreign_key, model and the type are the same for them, so it updated the record instead of creating a new one. It needed the fourth condition to differentiate them. It may also works with only model and filename in the conditions array, since the filename has the type and the foreign_key.

And the like in the fourth condition is to make sure the update works when the file has a different extension.

I don't know if I'm using it correctly, but thats how i made it works. Please correct me if I'm wrong. Also, you may have a workaround.

And the other commit, is ok?

[krolow]

@lucasfevi i think i got the scenario, the multiple part of Attach was never tested and as far i know used too much, the lack of unit tests of project is one of the issues that we never know if the things are working as expected or not...

anyway i think commit e13413e looks good and yes, it is one bug, so i'd like to ask you if you can create another PR just with this specific commit so i can merge this directly.

The second commit 74bde4c

is the one that i'm not totally sure, to be honest i don't like too much sql LIKE %% because it does not scale well, its not possible to make use of indexes of MySQL when we have wildcards for both sides... and I think the solution would impact as well in the normal workflow, if we upload a new file with different name/extension it will not be able to find, so it will create instead of updated (thinking in single file instead of multiple)

Having this in mind, for multiple file i think does not make sense to have a update feature, we would add a new one or remove one existent, instead of update... maybe if you have such a need @lucasfevi you can explain why you need that...

So thinking about one solution for that we might be able to detect if is a multiple file, we can just ignore the find of line Line 533

So when it is multiple file we will be always dealing with creation of file instead of update, and we do not need to add the additional condition to check the file...

what do you think @lucasfevi ?

[lucasfevi]

Thinking that way, this may be a solution only for my case.

Both single and multiple upload are working, but it's because i'm not using the multiple upload the "right" way. I have 3 input file and the name of them are product_photo[], so when I update one of them, I know which of the records needs to be updated (0, 1, or 2) and so I can compare them by the filename generated.

I could have created 3 different types for them (product_photo_0, product_photo_1, producto_photo_2) and use the single file attach. But I tried the multiple feature and that solution came up.

So I think the solution here is the one you said, just create and remove. But the $index maybe needs to be fixed, otherwise it will have duplicated filenames, since the index generating the filename use the key of foreach to be defined. So if I send 3 files in 3 differents requests, all the files will use 0 as index. But it also can be a separated case, what do you think?

[krolow]

okay, i think i got how are you using, what do you think if maybe we do like we have said:

1. check if is a multiple and ignore the update
2. Change the calls for saveAttachment to be a model call Line 450
3. Reduces the logic of saveAttachment to allow implementation at models Line 530-564

So the second idea it would be in case where the user expect a different behavior for save the attachment we would allow to override such a thing at model level.

So having in mind your situation, you would be able to add the additional condition in one method at your Product model where it would handle the saveAttachment differently, in the mean time the reduction of logic in the saveAttachment would be good to enable reuse code at model level...

As models in cakephp, have the __call method to check if some behavior implements the method we can take advantage of that...

Let me know what do you think and if it might help you... and so i can maybe propose one PR and we can discuss into more details about the solution!

In the mean time lets keep this PR open as a reminder for such a thing!