aurovrata / cf7-grid-layout

A WordPress plugin extension for Contact Form 7 to design responsive grid-layout forms.
https://wordpress.org/plugins/cf7-grid-layout/
GNU General Public License v2.0
6 stars 7 forks source link

Fix bugs on attachments #5

Closed Birmania closed 5 years ago

Birmania commented 5 years ago

-Double in attachments (as the first FILE is already processed by the original plugin) -"+" operator replaced by array_unique(array_merge...)) as "+" is an union on keys and not values. -"dup_tag" replaced by "sg_field_tag" -$components['attachments'] is an array, use the addition operator -"apply_filters" instead of "apply_fitlers"

Birmania commented 5 years ago

Note that I discovered a new bug which is not solved in this PR : In the "filter_wpcf7_validate" method, when there is the "@since 2.1.5 fix validation of non-toggled checkox/radio" code. In fact it only verify the first element of a tag and not the tabbed ones. That imply some strange behaviors like :

If there is a radio which is neither selected in the first and second tab, the validation only occurs on the first tab. If there is a input file which is only set in the second tab, it will be lost as the "validate" did not occurs.

Birmania commented 5 years ago

Hi @aurovrata

Note that I discovered a new bug which is not solved in this PR :

O.K, I think that I have a fix ! I I clean it a bit and send you the PR, let's count 24 hours.

Birmania commented 5 years ago

Hi @aurovrata

Sorry for the time it took but I tried three different design and was only satisfied by the simplicity of this last one. This is a major change on consolidation phase as it will no more try to guess the fields on server-side. It will use a list built thanks to JS.

This will solve problem with Checkbox/Radio that are not sent when empty. It may be my point of view but I feel server-side methods more simple this way.

Birmania commented 5 years ago

O.K, I think I didn't test everything. It was your backend consolidation that set the values for posted_data and so mail sent values. I will modify my PR tomorrow to keep the consolidation (based on cf7sg_fields) and validate using the posted data hierarchy (as you did before).

Do not merge now please. :)

Birmania commented 5 years ago

Hi back @aurovrata,

Sorry for the time it take, I just finished the PR and validate it with :

Everything seems O.K ! Yours faithfully, Birmania

aurovrata commented 5 years ago

Hey Birmania,
Sorry for the late reply, I was travelling for work and could not look at your messages. thanks for the PR. Let me go through it and get back to you.

Birmania commented 5 years ago

Hi aurovrata,

No problem, hope your travel was pleasant even for working.

This PR rework a bit the form post consolidation philosophy but it should cover/fix many special cases.

aurovrata commented 5 years ago

-"+" operator replaced by array_unique(array_merge...)) as "+" is an union on keys and not values.

yes, you are right except that you have not taken into account the way preg_grep works. It,

Returns an array indexed using the keys from the input array.

and this is the reason why it works, since I am taking the keys to build an array of values, preg_grep extracts these as say,

( [1]=> \<matched-key>, [3] => \<matched-key>,)

in the next preg_grep results, the values are:

( [0]=> \<matched-key>, [5] => \<matched-key>,)

and therefore adding the above 2 arrays using a '+' operator results in,

( [1]=> \<matched-key>, [3] => \<matched-key>, [0]=> \<matched-key>, [5] => \<matched-key>,)

I guess the code is confusign to read, and would likely be clearer using your expanded code, but given that I have used this in multiple places in the plugin code as well as making it more concise, I prefer to stick to this code structure.

Birmania commented 5 years ago

Hi Aurovrata,

Thanks for this info, I clearly missed it ! You are right, keep your writing style with '+', it is a good visual marker.

aurovrata commented 5 years ago

Dear Birmania,

so after quite a protracted period of coding/testing I finally came to the same conclusion as yours, that a tracking of tab/table/toggled section fields was required in order to stop guessing how many such fields were submitted in order to properly solve the problem of file and checkbox fields validation.

I was already partially tracking tab and table fields, and now I added a hidden field for each table/tab sections to track the number of rows/panels added. For toggle fields

For toggled sections, the plugin was already tracking their usage, but this was not integrated yet with the validation. This is now done.

Could you get some time to test to see if it works with your installation/forms as per your expectations?

Moving forward.

I appreciate your enthusiasm for helping out with improving/fixing this plugin, however, I realised that due to the lack of proper code documentation, it is difficult to make meaningful contributions to what can be complex data manipulations.

Hence, if you are keen to continue to collaborate, I propose to start documentation various classes/php files so as to make it clearer for you to understand.

I would also propose that before you embark on further code modifications, that we have have an intial discussion by raising an issue in order for me to give you specific guidance and therefore ensuring that your efforts are not in vain.

What do you think?

Birmania commented 5 years ago

Hi @aurovrata,

Happy new year to you and your family. Sincerely sorry for the time I take to answer... I am totally busy with my "traditional" work...

I agree with all you said ! I need to create issues first to talk about the subjects.

As for the new versions, the behavior seems perfect, validation is going well. Good job. ;)

aurovrata commented 5 years ago

HI @Birmania great to read you, wishing you a very good 2019 :)

I have a few ideas for improving the plugin and I am going to draw up a wiki page with some of these so we can maybe collaborate on some of the functional development.

aurovrata commented 5 years ago

Here is a roadmap requirements page.

Take a look and let me know if there is anything that might of interest to you.