PR-4.2-dev Maintainers Checked
avatar beefcakefu
beefcakefu
12 Apr 2022

Pull Request for Issue #37584 .

Summary of Changes

This pull request copies actions performed on contact form submissions that fail native form validation, and applies it to submissions that fail onValidateContact plugins.

Testing Instructions

This dummy plugin always fail contact form submissions and should demonstrate the issue:

<?php
defined('_JEXEC') or die;

class plgContactDummyValidation extends JPlugin {

	protected $autoloadLanguage = true;

	function onValidateContact($contact, $data) {
		return new InvalidArgumentException("Exception message is not shown.");
	}
}
  1. Install dummy plugin above.
    (System > Manage > Extensions > Install Extensions > Upload Package File)
  2. Enable dummy plugin.
    (System > Manage > Plugins > Contact - Dummy Validation > Enable plugin)
  3. Create a contact form in admin panel.
    (Menus > Main Menu > Add Site Menu Item > Menu Item Type > Contacts > Single Contact > Select Contact [create if there isn't already one], and give it a title)
  4. Set up valid mail credentials if not already present.
    (System > Setup > Global Configuration > Server > Mail)
  5. Fill and submit contact form on the public site. Observe how user is redirected to a blank page.
  6. Apply patch.
  7. Fill and submit contact form again. Observe how user is redirected to contact form pre-filled with previously entered values, with exception message in an alert banner which plugins can use to explain why the submission was unsuccessful.

Actual result BEFORE applying this Pull Request

If user submits a contact form with parameters failing an onValidateContact triggered plugin, plugin returns an exception, and user is redirected to a blank contact form page, stripped of all form elements with no error message indicating why the submission was unsuccessful.
Screenshot from 2022-04-12 23-41-00

Expected result AFTER applying this Pull Request

User is redirected to contact form page, pre-filled with submitted values and displaying exception message explanation of why submission failed. Same as if submission had failed native form validation.
Screenshot from 2022-04-21 11-42-00

Documentation Changes Required

None.

avatar beefcakefu beefcakefu - open - 12 Apr 2022
avatar joomla-cms-bot joomla-cms-bot - change - 20 Apr 2022
Category Front End com_contact
avatar beefcakefu beefcakefu - change - 20 Apr 2022
Labels Added: PR-4.1-dev
avatar richard67
richard67 - comment - 20 Apr 2022

@beefcakefu I've noticed you have just used the "Update branch" button because the branch was shown as not up to date. That's not a problem. But it is also not necessary as long as there are not shown conflicting files in addition. So I just wanted to let you know it's not necessary to always do that.

Regarding your review request: Code style looks ok, but for a deeper review I don't have the time now.

avatar beefcakefu
beefcakefu - comment - 21 Apr 2022

@beefcakefu I've noticed you have just used the "Update branch" button because the branch was shown as not up to date. That's not a problem. But it is also not necessary as long as there are not shown conflicting files in addition. So I just wanted to let you know it's not necessary to always do that.

Regarding your review request: Code style looks ok, but for a deeper review I don't have the time now.

Thanks @richard67! As you know, I'm very new to this, and your guidance is much appreciated.

avatar beefcakefu beefcakefu - change - 21 Apr 2022
The description was changed
avatar beefcakefu beefcakefu - edited - 21 Apr 2022
avatar beefcakefu beefcakefu - change - 21 Apr 2022
The description was changed
avatar beefcakefu beefcakefu - edited - 21 Apr 2022
avatar beefcakefu beefcakefu - change - 21 Apr 2022
The description was changed
avatar beefcakefu beefcakefu - edited - 21 Apr 2022
avatar beefcakefu beefcakefu - change - 23 Apr 2022
The description was changed
avatar beefcakefu beefcakefu - edited - 23 Apr 2022
avatar beefcakefu
beefcakefu - comment - 4 May 2022

@richard67 Sorry to bug you again, but it looks like this PR has stalled. I hope I don't come off sounding demanding, because that's not my intention, and I totally understand everyone here is volunteering their free time for the Joomla! project.

I was hoping you could enlightening me about the typical lifecycle of a PR. At this point, I've addressed 2 reviews by yourself and @PhilETaylor. Does something need to happen from you guys, like to acknowledge that your feedbacks were satisfactorily resolved? Or do we just wait for 2 human testers to confirm the PR works and we're ready to commit?

I've actually authored a plugin to validate that enquirers' email addresses are working before allowing a contact form submission. It opens a connection to the enquirer's SMTP server and checks that the mailbox exists and isn't full and everything. I think it'll be a great plugin to combat prolific spammers like eric.jones.z.mail@gmail.com. I hope to release the plugin as open source, and the only thing stopping me from putting it on the Joomla! Extensions Directory is that exception messages currently cannot be shown to an enquirer without this PR.

This PR and my plugin also work in Joomla! 3.x. What do I have to do so this PR is also committed to the 3.x branch?

Thank you for your attention, and your patience when I opened the unnecessary Issue, and teaching me it's not necessary to always "update branch" everytime it shows not up to date.

avatar PhilETaylor
PhilETaylor - comment - 4 May 2022

The Joomla project is well known for leaving PR contributions open as long as 5+ years… so 22 days open for this PR is nothing. If a PR is not tested and merged within 3 days then it normally stalls for months or even years. This is not opinion, this is fact. You can do no more except wait for someone to test your PR and for a maintainer to accept or reject it.

avatar richard67
richard67 - comment - 4 May 2022

Or do we just wait for 2 human testers to confirm the PR works and we're ready to commit?

@beefcakefu That's the case.

What do I have to do so this PR is also committed to the 3.x branch?

You would have to make a PR for the 3.10-dev branch and mention this PR here in the description of the other PR so it's clear they do the same.

But let's ask the 3.10 release lead @zero-24 first if this is a bug fix which he would like to have in 3.10.

avatar beefcakefu
beefcakefu - comment - 4 May 2022

Thank you both for your input! I'll tidy up the code and share my plugin on the Joomla! Forum and see if anyone adventurous wants to take it for a test drive together with this PR (and pray they also have a GitHub account to mark it as working on the issue tracker).

If a PR is not tested and merged within 3 days then it normally stalls for months or even years.

I've actually suspected that might be the case after skimming through the issue tracker. Since making this PR, I've also set up a test environment and shared my first test result. I intend to start contributing as a patch tester and hopefully make a difference in this unclosed PR contribution situation.

avatar richard67
richard67 - comment - 4 May 2022

I intend to start contributing as a patch tester and hopefully make a difference in this unclosed PR contribution situation.

@beefcakefu That would indeed be a good help because we are so few people doing that. Thanks in advance.

avatar bembelimen
bembelimen - comment - 4 May 2022

Hey @beefcakefu very cool addition 👍 Thank you for your contribution.
I moved it to the 4.2 branch, as it's not really a bug fix but an enhancement.
So it just needs now two valid tests and we're good to go.

avatar brianteeman
brianteeman - comment - 4 May 2022

I moved it to the 4.2 branch, as it's not really a bug fix but an enhancement.

Which also means that it wont be accepted for j3

avatar toivo
toivo - comment - 6 May 2022

Unable to report failed test result from Joomla 4.2.0-alpha3-dev of 6 May. Is that caused by the out-of-date message "This branch is out-of-date with the base branch"?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37540.
avatar beefcakefu beefcakefu - change - 6 May 2022
Labels Added: PR-4.2-dev Maintainers Checked
Removed: PR-4.1-dev
avatar beefcakefu
beefcakefu - comment - 6 May 2022

Unable to report failed test result from Joomla 4.2.0-alpha3-dev of 6 May. Is that caused by the out-of-date message "This branch is out-of-date with the base branch"?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37540.

I think that was it! Have updated branch and it's now passing. Thanks toivo!

avatar richard67
richard67 - comment - 6 May 2022

Unable to report failed test result from Joomla 4.2.0-alpha3-dev of 6 May. Is that caused by the out-of-date message "This branch is out-of-date with the base branch"?

@beefcakefu What @toivo means is that he can't use the "Test this" button in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/37540 to mark his test result.

@toivo That's a but which the issue tracker sometimes has. For some reason it did not see that this PR is a PR and not an issue, and so the status is "New" and not "Pending" in the issue tracker, and the button is missing. What you can do is you can report back your test result in a normal comment and provide some information about what has failed so @beefcakefu can fix his PR if necessary.

avatar beefcakefu
beefcakefu - comment - 6 May 2022

@beefcakefu What @toivo means is that he can't use the "Test this" button in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/37540 to mark his test result.

I see.

Thank you for taking the time to test this PR, @toivo. Where are you running into problems?

avatar Kubik-Rubik
Kubik-Rubik - comment - 6 May 2022

@bembelimen I don't see the "Test" button in the Joomla! Issue Tracker?

grafik

IMO this can be considered a bugfix (entered data is kept on Exception that a 3rd party extension could trigger) and should also be added to Joomla! 3!

avatar richard67
richard67 - comment - 6 May 2022

@Kubik-Rubik If you would scroll a bit up you would find my comment regarding the issue tracker problem.

avatar beefcakefu beefcakefu - change - 6 May 2022
The description was changed
avatar beefcakefu beefcakefu - edited - 6 May 2022
avatar beefcakefu
beefcakefu - comment - 6 May 2022

@toivo I have updated the testing instructions above to be a bit more detailed. Hopefully that helps.

avatar joomla-bot
joomla-bot - comment - 27 Jun 2022

This pull requests has been automatically converted to the PSR-12 coding standard.

Add a Comment

Login with GitHub to post a comment