Issues

ZF-3791: Add file upload testing

Description

The testing system should allow for testing of file uploads, a cURL style syntax might be useful ('@filename.jpg')

Comments

Actually, the Zend_File_Transfer component use a pair of *_uploaded_file() functions that are not mockable. My proposal is to add a unitTesting public property to Zend_File_Transfer like the one in Zend_Session. When property is set, the component can use rename() instead of move_uploaded_file() for example.

Patch proposal. Verified under zf 1.7.5 and it does not break tests of Zend/File, Zend/Test and Zend/Validate folder. After adding this patch, an user can mock $_FILES array in controller testcases.

Great idea, Giorgi. Thanks for sharing the patch!

This patch is problematic. It breaks actual coding standard by adding methods/constants only for testing. In the last 2 years I was said several times that a class should not have methods/constants which are for testing purposes only.

It also allows to break behaviour and function of the base component which means that all further additions will not work as they should.

Also to note: Testing the upload itself would mean to test if the webserver works but this in not possible and also not necessary. Testing a upload works in 2 steps... first look if the form you receive contains the correct values. And second if all parts after the upload itself work correct. Both can be done without changing behaviour. The process itself, which you patched, is part of the server upload and can not be tested.

Take a look at the actual testbed within trunk to see how this can be archived.

You should also keep in mind that this component is not about using HTTP upload, it is about using file transfer mechanism, this includes not only also download but additionally also other protocols. And here this patch would disallow functionallity.

As developer of this component I have to warn about integration of this patch into the core.

Hey hey - not saying it has to be included. It's here for developers like me and Till that need a quick way to falsificate upload for testing. If in Zend_Test there would be a $this->request->setPost()->addFile() I will be happy to use it, but I have subclassing of Zend_Form_Element_File where the code path from preleving from $_FILES to automatically create a entity in the database linked to the physical file has to be covered by testing in some way.

Regarding coding standards, well: class Zend_Session extends Zend_Session_Abstract { /** * Whether or not Zend_Session is being used with unit tests * * @internal * @var bool / public static $unitTestEnabled = false; ..it's the same hack used to bypass session() functions, the equivalent of *_uploaded_file().

A possible solution is providing a way to inject transfer adapter in form elements via factory in registry/service locator/dependency injection container/whatever, like it's done with the request object (that permits me to set a header like X-Requested-With to simluate an ajax call, without having to modify $_SERVER superglobal array, equivalent of $_FILES).

Just because other classes do not conform the coding practice does not mean that it will become coding standard for other components ;-)

What I said is still true: There is another way of testing, so such a change is not necessary for the core itself beside of the problem it would integrate.

Btw: You can change the attached transfer adapter within the file form element since it has been released. So it's also possible to create a own mock adapter. That's what I meant as I said "look at the existing testbed of the core".

Thomas -- I'm not sure who's perpetuating that the coding standards do not allow adding methods/constants only for use with testing, but it's surely not me, and it flies in the face of some of the very practices we try to evangalize. ZF applications should be easy to test, and currently there are a few areas that are actually quite difficult -- this is one.

The patch looks good and solves a problem related to testing ZF applications. If you do not apply it, I will.

Matthew: Do what you think you must do. You know that I am not fetching issues which are attached to another person when I am not the main lead of this component. And I am not related to Zend_Test nor have I done anything in past with it.

Related to coding practice (not standard): I was said several times in past from the dev-team that I am not allowed to add methods and constants in public visibility which are only for testing purposes. This was before your time mainly by gavin and others in 2006 and 2007.

Also to note: The patch itself is errorous as it breaks behaviour in case of testing. It does not work correct when the file is optional. I have to warn that the component will then behave different when being tested (and I am not speaking of the file being moved or not). Applying a correct solution would be better in my eyes. Also for the upload progress we added protected solutions and not public ones.

I don't know details of Zend_Test, only of Zend_File_Transfer, but in my eyes a new option would be better, as it's not visible to IDEs due to being part of the adapters options array. Also the validator should and can be solved differently. There is no need to add a new dependency to Zend_File.

After checkout of 1.7 branch and applying, I see that the patch does not broke current behaviour (Zend/File/AllTests.php). However, I have found only an AbstractTest.php that tests the abstract adapter and not the Http one. I can add a pair of test cases and produce a new patch with correct behaviour on optional files, but where I should insert them?

I have done a checkout of the trunk and found the HttpTest there, now I'll try.

The design is broken as you depend on a static dependency and prohibit to override the behaviour. A good solution would allow developers to inject the dependency themselves in a testing environment or at least monkey-patch it somehow but in this case and in case of PHP in general the Runkit extension is the only solution.

If we extracted {{Zend_File_Transfer_Adapter_AbstractTest_MockAdapter}} from {{tests/Zend/File/Transfer/Adapter/AbstractTest.php}} into a separate class, {{Zend_File_Transfer_Adapter_MockAdapter}} or something similar, would this solve the problem at hand? I think the problem outlined here is very similar (if not the same as) ZF-11197