Skip to content
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

Contact form loses data after postback with error #17743

Merged
merged 3 commits into from
Jan 8, 2018
Merged

Contact form loses data after postback with error #17743

merged 3 commits into from
Jan 8, 2018

Conversation

sandewt
Copy link
Contributor

@sandewt sandewt commented Aug 28, 2017

Pull Request for issue #17126.

Summary of Changes

Some code.

Testing Instructions

Instructions, see issue #17126

  • Go to Single Contact.
  • Fill in the form.
  • Do not fill in captcha.
  • Send email.

Expected result

Warning + textboxes are NOT empty.

Actual result

All textboxes are empty, so the user has to fill in AGAIN all textboxes.

Documentation Changes Required

No.

Thanks to

@pascal-910

Pull Request for issue #17126.

Testing Instructions. See issue #17126
* Go to Single Contact. 
* Fill in the form. 
* Do not fill in captcha.
* Send email.

Expected result:  
Warning + textboxes are NOT empty. 
So the user should not entered all text again.

Actual result:
All textboxes are empty, so the user has to enter again all textboxes.

Thanks to: 
@ pascal-910.
@sandewt
Copy link
Contributor Author

sandewt commented Aug 28, 2017

I'm new on Github.
If I have downloaded the code and look to the characters.
So I see in my editer, that after the "{" and the "}" braces (accolades) is a tab. In line 76 and 78. (NOK.)
How can I avoid this in github by making a PR?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@sandewt
Copy link
Contributor Author

sandewt commented Aug 28, 2017

Ask for input.

Is the following code nessecary?

empty($item->catid)

In the DB table xxx_content is defined:

`catid` int(10) UNSIGNED NOT NULL DEFAULT '0'

So the catid may not be empty !?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@n9iels
Copy link
Contributor

n9iels commented Aug 28, 2017

The patch works fine for me.

Is the following code nessecary?

There is never a problem with extra test :-). However in this situation only !empty($item->catid) is enough. !isset($item->catid) will only check if the $item object has a 'catid' property. !empty($item->catid) will check if the $item object has a 'catid' property, and this property is not empty. See this post for more information:

https://stackoverflow.com/questions/7191626/isset-and-empty-what-to-use#answer-7191642

If you change this I can conform the test via the J!Issue tracker.

@sandewt
Copy link
Contributor Author

sandewt commented Aug 28, 2017

Thanks, interesting post.

Do you mean!empty($item->catid) or empty($item->catid) is enough? Because catid is not empty.
The function $app->setUserState(..) may not be executed in this situation!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

Delete some tabs.
@sandewt
Copy link
Contributor Author

sandewt commented Sep 1, 2017

Do not fiil in captcha. After send all data form loses after postback !!!

contactform

@sandewt sandewt closed this Sep 1, 2017
@sandewt
Copy link
Contributor Author

sandewt commented Sep 1, 2017

Sorry, a mistake for closing!!

@sandewt sandewt reopened this Sep 1, 2017
@sandewt
Copy link
Contributor Author

sandewt commented Sep 23, 2017

Please test this PR #17126 or give comment.

@conconnl
Copy link
Member

conconnl commented Oct 7, 2017

I have tested this item ✅ successfully on 8004b7a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

1 similar comment
@slibbe
Copy link

slibbe commented Oct 7, 2017

I have tested this item ✅ successfully on 8004b7a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@ghost
Copy link

ghost commented Oct 8, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 8, 2017
@wojsmol
Copy link
Contributor

wojsmol commented Oct 8, 2017

@franz-wohlkoenig Please see @n9iels #17743 (comment)
@sandewt Can you address this #17743 (comment)?

@ghost
Copy link

ghost commented Oct 8, 2017

@wojsmol means this Comment not RTC?

@wojsmol
Copy link
Contributor

wojsmol commented Oct 8, 2017

@franz-wohlkoenig Let's leve desicion to one of maainterners. Code my to be a little simpler, but both versions works.

if (!isset($item->catid) || empty($item->catid)) changes to if (empty($item->catid))
@sandewt
Copy link
Contributor Author

sandewt commented Oct 9, 2017

On closer inspection and the comments, the following:

In the Joomla! core rarely used the combination of 'isset and empty', and not for no reason.
So I choise for the proposal of @niels that one test is enough.
I expect a string for $item->catid (gettype) which is p.e. '16'. And not '' or '0'.
So I prefer 'empty' in stead of '!isset' if the user state ($app->setUserState) is not yet set.
See also: http://php.net/manual/en/types.comparisons.php

So I changed the code, PLEASE TEST AGAIN.

@sandewt sandewt closed this Oct 9, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 9, 2017
@sandewt
Copy link
Contributor Author

sandewt commented Oct 9, 2017

Sorry for closing, issue should be open @franz-wohlkoenig.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@brianteeman
Copy link
Contributor

re-opened

@brianteeman brianteeman reopened this Oct 9, 2017
@sandewt
Copy link
Contributor Author

sandewt commented Oct 9, 2017

Thanks Brian, and @niels should be @n9iels.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@Quy
Copy link
Contributor

Quy commented Dec 12, 2017

I have tested this item ✅ successfully on 9377adb


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@ghost
Copy link

ghost commented Dec 16, 2017

@conconnl @slibbe can one of you please test?

@Sieger66
Copy link
Contributor

Sieger66 commented Jan 7, 2018

I have tested this item ✅ successfully on 9377adb


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17743.

@Quy
Copy link
Contributor

Quy commented Jan 7, 2018

RTC

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 7, 2018
@mbabker mbabker added this to the Joomla 3.8.4 milestone Jan 8, 2018
@mbabker mbabker merged commit 67e95cb into joomla:staging Jan 8, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 8, 2018
@ReLater
Copy link
Contributor

ReLater commented Apr 18, 2018

Looks like this pr has opened another issue. See #20195 please.

@infograf768
Copy link
Member

@sandewt
Please look at #20195 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.