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

Allow onValidateContact plugin to show error message #37540

Merged
merged 13 commits into from
Aug 30, 2023

Conversation

beefcakefu
Copy link
Contributor

@beefcakefu beefcakefu commented Apr 12, 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.

If a plugin listening to the `onValidateContact` event wants to stop contact form submission, it is supposed to return an exception. Currently, after submission is halted, user is sent back to a blank contact form page, stripped of all form elements with no error message indicating why the submission was unsuccessful.

PR resolves this by copying the actions for submissions failing native form validation, which is to redirect to contact form pre-filled with submitted values and displaying exception message, hopefully a descriptive explanation of why submission failed.

I've written an email validation plugin that returns exceptions with descriptive messages that can be used to test out the PR. Download here: https://www.dropbox.com/s/s1m9j8uqq0689p9/plg_contact_cvalidation.zip?dl=0
@beefcakefu beefcakefu changed the title Allow onValidateContact listener to give feedback Allow onValidateContact plugin to show error message Apr 13, 2022
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67
Copy link
Member

@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.

@beefcakefu
Copy link
Contributor Author

@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.

Updating reference to $stub with $id at @PhilETaylor's suggestion to prevent XSS.
@beefcakefu
Copy link
Contributor Author

@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.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

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.

@beefcakefu
Copy link
Contributor Author

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.

@richard67
Copy link
Member

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.

@bembelimen bembelimen changed the base branch from 4.1-dev to 4.2-dev May 4, 2022 11:43
@bembelimen
Copy link
Contributor

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.

@zero-24 zero-24 removed the PR-4.1-dev label May 4, 2022
@richard67 richard67 added the Maintainers Checked Used if the PR is conceptional useful label May 4, 2022
@brianteeman
Copy link
Contributor

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

beefcakefu added a commit to beefcakefu/joomla-cms that referenced this pull request May 5, 2022
If a plugin listening to the `onValidateContact` event wants to stop contact form submission, it is supposed to return an exception. Currently, after submission is halted, user is sent back to a blank contact form page, stripped of all form elements with no error message indicating why the submission was unsuccessful.

This change resolves the problem by copying the actions for submissions failing native form validation, which is to redirect to contact form pre-filled with submitted values and displaying exception message, hopefully a descriptive explanation of why submission failed.

Relates to Joomla! 4 pull request: joomla#37540 This is for anyone looking for a manual fix for Joomla! 3, as this is not a bug fix and will not be merged into Joomla! 3.
@toivo
Copy link
Contributor

toivo commented May 6, 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.

@beefcakefu
Copy link
Contributor Author

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!

@joomla-bot
Copy link
Contributor

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

@HLeithner HLeithner removed the psr12 label Oct 23, 2022
@Hackwar Hackwar added the Small A PR which only has a small change label Feb 26, 2023
@Hackwar Hackwar added the bug label Apr 6, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:30
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.3-dev.

@TLWebdesign
Copy link

Tested successfully on ‎4.3.5-dev


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

1 similar comment
@RickR2H
Copy link
Member

RickR2H commented Aug 26, 2023

Tested successfully on ‎4.3.5-dev


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

@joomla-cms-bot joomla-cms-bot added PR-4.2-dev and removed bug PR-4.3-dev Small A PR which only has a small change labels Aug 26, 2023
@RickR2H
Copy link
Member

RickR2H commented Aug 26, 2023

RTC (Tests where added as comments as issue tracker had issues)


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 26, 2023
@Quy Quy added PR-4.3-dev and removed PR-4.2-dev labels Aug 30, 2023
@obuisard obuisard added this to the Joomla! 4.3.5 milestone Aug 30, 2023
@obuisard obuisard added the bug label Aug 30, 2023
@obuisard obuisard merged commit 17585ed into joomla:4.3-dev Aug 30, 2023
@obuisard
Copy link
Contributor

Thank you @beefcakefu for this PR.
I consider it a long lasting bug fix and took the responsibility to merge it.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 30, 2023
HLeithner pushed a commit that referenced this pull request Sep 4, 2023
* fix calendar positioning (#41477)

* Update joomla-field-media.w-c.es6.js (#41361)

* Fixing md5 Deprecation in document.php (#39880)

* modal field check for danger alert (#39173)

* fix (#41138)

* [4.x] Cassiopeia Install SQL (#37389)

* Fix/showon (#37451)

* Bugfix for Issue #38599 - Smart Search indexing macros from articles (#38998)

* [4,3] joomla alert icons (#39312)

* Update StyleModel.php (#40665)

* Update Query.php (#40687)

* remove hitcount variable from uri after voting (#41474)

* Fix deprecated message in categories when extension is empty (#40604)

* Re-add minus to allowed filenames in com_templates (#41369)

* Invalid delete of all content articles from smart search index (#41288)

* [4][cli] schema update check (#40468)

* Tags: Don't put NULL into trim() (#41509)

Co-authored-by: Quy <quy@nomonkeybiz.com>

* [4] Add multilanguage filter taxonomy rows in com_finder advanced filters (#41470)

* Fix taxonomy filter options

* refactor rest of query to prepared statements

---------

Co-authored-by: Quy <quy@nomonkeybiz.com>

* Check if the pre update check box actually exists (#41467)

* Allow onValidateContact plugin to show error message (#37540)

* [4.4] Backward compatibility handling for plugins that setting the result directly (#41525)

* Backward compatibility handling for plugins that setting the result directly.

* Update libraries/src/Event/AbstractImmutableEvent.php

Co-authored-by: Brian Teeman <brian@teeman.net>

---------

* Tests for com_privacy consent on user site (#41529)

* Tests for com_privacy consent on user site

* Allow ability to enable extension in tests to prevent errors

* remove paramater not needed

* Added fixes to stop  errors in drone build

* Added fixes to stop  errors in drone build

* Added fixes to stop  errors in drone build

* [4.4] system test for com_installer (#41483)

* system test for com_installer

* cs

* Missing System Tests in #41362 (#41479)

* Update Categories.cy.js

* Update Categories.cy.js

* Update Categories.cy.js

* Update Categories.cy.js

* Update Categories.cy.js

* Update Categories.cy.js

added spaces and semicolons

* Update Categories.cy.js

Missing spaces and semicolons

* Update Categories.cy.js

Missing spaces and semicolons

* Update Categories.cy.js

Missing spaces and semicolons

* Update Categories.cy.js

removed tabs

* Update Categories.cy.js

Removed tabs

* Update Categories.cy.js

Removed tabs

* Update Categories.cy.js

Removed tabs

* Update Language.php (#41353)

* Filter for special characters that fail sending test mail (#41469)

* Backporting event changes (#41484)

Co-authored-by: Allon Moritz <allon.moritz@digital-peak.com>

* api tests for endpoints for com_privacy consent (#41544)

* Show the plugin name in the update screen (#41537)

* Show the plugin name in the update screen

* cs

---------

* Replace the special characters in site name for mail templates (#41582)

* Updates the dependencies (#41538)

* Updating dependencies

* Major update cypress

* Thanks for the reminder @brianteeman

* Composer

* weekend updates

* Update dependencies

* Update incompatible HTML file

---------

Co-authored-by: Magnus Singer <magnussinger@icloud.com>
Co-authored-by: Dimitris Grammatikogiannis <d.grammatiko@gmail.com>
Co-authored-by: Crystal Dionysopoulos <9141288+crystalenka@users.noreply.github.com>
Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>
Co-authored-by: Denitz <197527+Denitz@users.noreply.github.com>
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: toroworx <toroworx@users.noreply.github.com>
Co-authored-by: Martin Carl Kopp <6154099+MacJoom@users.noreply.github.com>
Co-authored-by: Jonathan Brain <3941269+BrainforgeUK@users.noreply.github.com>
Co-authored-by: Olivier Buisard <olivier.buisard@simplifyyourweb.com>
Co-authored-by: Christiane Maier-Stadtherr <dev@chmst.de>
Co-authored-by: Manuel Huber <ManuelHu@users.noreply.github.com>
Co-authored-by: Nicola Galgano <optimus4joomla@gmail.com>
Co-authored-by: Hannes Papenberg <info@joomlager.de>
Co-authored-by: Quy <quy@nomonkeybiz.com>
Co-authored-by: David Jardin <d.jardin@djumla.de>
Co-authored-by: beefcakefu <103491197+beefcakefu@users.noreply.github.com>
Co-authored-by: Fedir Zinchuk <getthesite@gmail.com>
Co-authored-by: andyforrest <46865032+andyforrest@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Maintainers Checked Used if the PR is conceptional useful
Projects
None yet
Development

Successfully merging this pull request may close these issues.