CakePHP-Bootstrap / cakephp3-bootstrap-helpers

CakePHP 3.x Helpers for Bootstrap 3 and 4.
https://holt59.github.io/cakephp3-bootstrap-helpers/
MIT License
130 stars 79 forks source link

Fix FancyFileWidget #127

Closed ypnos-web closed 7 years ago

ypnos-web commented 7 years ago

Various fixes for the fancy file upload widget.

To fix #123 and some more problems I found.

ypnos-web commented 7 years ago

A test is failing, I have no idea what is going on there, maybe you have an idea?

Holt59 commented 7 years ago

I can't look into this right now but quickly looking at the error from travis:

1) Bootstrap\Test\TestCase\View\Helper\FormHelperTest::testFormSecuredFileControl
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'Contact.picture-text'
+    0 => 'Contact.picture]-text'
     1 => 'Contact.picture.name'
     2 => 'Contact.picture.type'
     3 => 'Contact.picture.tmp_name'
     4 => 'Contact.picture.error'
     5 => 'Contact.picture.size'
 )

Maybe you added a typo somewhere?

ypnos-web commented 7 years ago

Yes I got this failure also with phpunit, I don't know where the extra ']' is coming from. I am not as experienced with Cake's templating so I thought I'd ask you first.

I will try to debug the test. In my own use case everything works as expected. That's at least something ;).

Holt59 commented 7 years ago

I think I got something similar and the beginning, and it's why I switch to the [text] field, so the issue might be that you reverted this change... I cannot investigate more right now unfortunately.

Holt59 commented 7 years ago

@ypnos-web Could you provide me a working example (e.g. from your own use case)? I will see if I can do something about that false phpunit warning.

ypnos-web commented 7 years ago

This would be a working example:

    public function testFormSecuredFileControl() {
        $this->form->setConfig('useCustomFileInput', true);
        // Test with filename, see issue #56
        $this->assertEquals([], $this->form->fields);
        $this->form->file('picture');
        $expected = [
            'picture-text',
            'picture.name', 'picture.type',
            'picture.tmp_name', 'picture.error',
            'picture.size'
        ];
        $this->assertEquals($expected, $this->form->fields);
    }
Holt59 commented 7 years ago

@ypnos-web I was talking about a working application (because I did not manage to create one that reproduces everything... ), but if this test fixes the issue, feel free to update the TestCase with it.

ypnos-web commented 7 years ago

I rewrote the code to work in both cases. I feared it would bite back at us as soon as someboday actually has the file as a member of another entity.

codecov-io commented 7 years ago

Codecov Report

Merging #127 into master will increase coverage by 0.39%. The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #127      +/-   ##
============================================
+ Coverage     82.55%   82.94%   +0.39%     
- Complexity      331      334       +3     
============================================
  Files            17       17              
  Lines          1043     1061      +18     
============================================
+ Hits            861      880      +19     
+ Misses          182      181       -1
Impacted Files Coverage Δ Complexity Δ
src/View/Widget/FancyFileWidget.php 98% <100%> (+2.65%) 12 <3> (ø) :arrow_down:
src/View/Helper/EasyIconTrait.php 100% <0%> (ø) 7% <0%> (+2%) :arrow_up:
src/View/Helper/HtmlHelper.php 84.35% <0%> (+0.21%) 39% <0%> (+1%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ca0ebf8...3f48549. Read the comment docs.

Holt59 commented 7 years ago

@ypnos-web Thanks for the update.

Could you merge testFormSecuredFileControl and testFormSecuredFileControl2 (place the 3 asserts in a single testFormSecuredFileControl)? If you don't have time, I'll merge and update it myself this week-end.

ypnos-web commented 7 years ago

Sorry I didn't spend more time on how I could merge them the first time.

Holt59 commented 7 years ago

I was actually thinking about simply putting the two assertEquals in the same method, but this work, so I'm merging, thanks ;)

ypnos-web commented 7 years ago

Well in that case I reckon the form would've needed to be resetted in between, or the second assert include the fields from the first.