? ? ? Pending

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
30 Apr 2018

Summary of Changes

Finally com_mailto allow the usage of a captach by using JForm

As discussed with the JSST this is now a public tracker item: cc @SniperSister @mbabker

Testing Instructions

  • apply this patch
  • check that the mail to friend feature still works
  • configure the captcha
  • check that com_mailto now requires the captcha

Expected result

com_mailto supports captcha's
com_mailto uses JForm

Actual result

com_mailto don't supports captcha's
com_mailto don't uses JForm

Documentation Changes Required

none

avatar zero-24 zero-24 - open - 30 Apr 2018
avatar zero-24 zero-24 - change - 30 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Apr 2018
Category Front End com_mailto Language & Strings
avatar zero-24 zero-24 - change - 30 Apr 2018
Labels Added: ? ?
avatar brianteeman brianteeman - change - 30 Apr 2018
Title
Finally com_mailto allow the usage of a captach by using JForm
Finally com_mailto allow the usage of a captcha by using JForm
avatar brianteeman brianteeman - edited - 30 Apr 2018
avatar brianteeman
brianteeman - comment - 30 Apr 2018

milestone is set to 3.8.8. but surely this is a new feature according to semver

avatar zero-24
zero-24 - comment - 30 Apr 2018

milestone is set to 3.8.8. but surely this is a new feature according to semver

new feature? Where do you see a new feature? This is a fix for a broken and exploited attack against sites as noted here in public and in private many times. ;)

avatar mbabker
mbabker - comment - 30 Apr 2018

You technically get a new feature introducing captcha support to the form. Otherwise it's all just internal refactoring/restructuring.

avatar zero-24
zero-24 - comment - 30 Apr 2018

You technically get a new feature introducing captcha support to the form.

Correct but this is just a side effect of the refactoring to JForm ;)

avatar brianteeman
brianteeman - comment - 30 Apr 2018

Adding captcha is a new feature. Semantic versioning is all about technicalities

avatar zero-24
zero-24 - comment - 30 Apr 2018

@mbabker do you agree with @brianteeman or can we move forward with that protection against spam in 3.8.8 by allowing that feature?

Or do we need to go the hard way in patching the xml lines for the captcha out and explain in a doc page where to add the support for that.?

avatar ot2sen
ot2sen - comment - 30 Apr 2018

Must be considered a bug at this stage. Captcha has been a core feature by ages so it must be a bug that it didn´t arrive in all contact or form interactions when first added :)

avatar mbabker
mbabker - comment - 30 Apr 2018

To be completely honest it's one of those edge cases where you have to outweigh the user benefit with strict adherence to the standard. At this point we're dealing with a weakness in core that could've legitimately just been committed to 3.8.8 as a security fix without this public item but we collectively decided this doesn't need to be committed with that type of "secrecy" (for a lack of a better term). So for me, even though a standard says this strictly must trigger a minor version release, I don't think this absolutely must go into 3.9 and not be addressed in a 3.8 release.

avatar brianteeman
brianteeman - comment - 30 Apr 2018

@mbabker even if it was a security issue it would still require a version bump
@ot2sen that would be perhaps true if it was indeed added to all forms but it still hasnt been

there are a lot more urgent issues which are actually going to b useful that are being held up for 3.9

either we use semver or we dont - its not something that can be done half heartedly

avatar mbabker
mbabker - comment - 30 Apr 2018

It's not being done half-heartedly. The standard is the guiding line and we have to justify deviating from it (which should be an exceptional basis). It's not "follow the standard when it's convenient". If we followed the standard that strictly then we would have to version bump when we:

  • Run composer update and it includes minor version increments (the changes may not all be part of our API but as part of our distributed package it does introduce new features)
  • Update any JavaScript library which is a minor version release (CodeMirror being the common one)
  • Merge any patch that is not strictly fixing a bug

So we already do it "half heartedly" in some ways. Considering I can't get good answers on how to handle releases beyond 3.8.x right now the best thing I can do while I'm running releases is use my best judgment on things. And considering the amount of marketing hype that the project likes to put onto .0 releases (and the amount of annoyance end users have if .0 releases don't have something new and shiny, see 3.4 and 3.5 reactions) I doubt anyone would want us doing "full blown" feature releases every 2-3 months as that's about the cycle we're on for something being merged that is not 100% in line with the SemVer definition of a patch release.

avatar brianteeman
brianteeman - comment - 30 Apr 2018

guess i am just frustrated that several pr of mine got delayed because of semver and the privacy plugin will be useless by the time it is released

avatar Quy
Quy - comment - 30 Apr 2018

The form no longer fits within the popup window. You have to scroll down to see the buttons.
The email address is not populated.

Before:
20265-before

After:
20265-after

avatar joomla-cms-bot joomla-cms-bot - change - 30 Apr 2018
Category Front End com_mailto Language & Strings Front End com_content com_mailto Language & Strings
avatar zero-24
zero-24 - comment - 30 Apr 2018

should be fixed now thanks @Quy

avatar brianteeman
brianteeman - comment - 30 Apr 2018

the last change isnt a good solution. You have hard coded the height of the popup to the height required by the core captcha irrespective of it being enabled or not. In addition the user could be using a different captcha plugin that requires more space. The modal height should be dynamic based on the content and not fixed or at least in a file that the user can override.

avatar zero-24
zero-24 - comment - 30 Apr 2018

Still not great but better than nothing.

avatar Quy
Quy - comment - 1 May 2018

Without captcha, it should be 500.

avatar brianteeman
brianteeman - comment - 1 May 2018

did the JSST review the changes in this pr - there are issues (not for public explanation)

avatar brianteeman
brianteeman - comment - 1 May 2018

Note the style changes because the markup is different which effects the height

Before PR

chrome_2018-05-01_08-50-38

After PR

chrome_2018-05-01_08-52-22

avatar zero-24
zero-24 - comment - 1 May 2018

Fixed thanks. @brianteeman

avatar zero-24
zero-24 - comment - 1 May 2018

Without captcha, it should be 500.

well with captcha we than have just 50 more than we can just do it 550 for all?

avatar zero-24
zero-24 - comment - 1 May 2018

did the JSST review the changes in this pr - there are issues (not for public explanation)

Just to don't let this claim unanswered. We are in 2018 such a check should not be required any more (It seams to be something added in 2006). But I have just added the check back. And for the record I got an review on the code first for sure. Thanks.

avatar brianteeman
brianteeman - comment - 1 May 2018

This PR doesnt "allow" the use of captcha it forces it on unless you turn it off - is that the intended behaviour?

Maybe I am thick but I dont see where the mailto xml options can be accessed

avatar zero-24
zero-24 - comment - 1 May 2018

This PR doesnt "allow" the use of captcha it forces it on unless you turn it off - is that the intended behaviour?

Well when you have setup a captcha joomla should use it by default like we did for the registration right?

avatar brianteeman
brianteeman - comment - 1 May 2018

We are in 2018 such a check should not be required any more (It seams to be something added in 2006)

the issue addressed back then is still a valid issue today so it is still required

avatar zero-24
zero-24 - comment - 1 May 2018

the issue addressed back then is still a valid issue today so it is still required

As i said this check was added back. ;) And even improved :P

avatar brianteeman
brianteeman - comment - 1 May 2018

Maybe I am thick but I dont see where the mailto xml options can be accessed

avatar zero-24
zero-24 - comment - 1 May 2018

Maybe I am thick but I dont see where the mailto xml options can be accessed

As there are no options for com_mailto itself no where :P You can enable and disable the mailto link in com_content / global / menu.

avatar mbabker
mbabker - comment - 1 May 2018

To render a JForm instance on the frontend it still uses those XML definitions.

avatar mbabker
mbabker - comment - 1 May 2018

Plus it defines some rules that are used in form validation. It's not strictly for backend configuration.

avatar brianteeman
brianteeman - comment - 1 May 2018

see told you I was thick. I guess it was an error then that we didnt use an xml before

avatar mbabker
mbabker - comment - 1 May 2018

Well when you have a half-million line code base and few people looking over and maintaining all aspects of it... 😆

avatar zero-24
zero-24 - comment - 1 May 2018

see told you I was thick. I guess it was an error then that we didnt use an xml before

Yes. We didn't needed that xml before as we just did plain html without using jform now with jform the xml is used for the things Michael posted configuration & validation settings. ;)

avatar Quy
Quy - comment - 1 May 2018

In Chrome without captcha, there is a vertical scrollbar.
20265-chrome

avatar zero-24
zero-24 - comment - 1 May 2018

In Chrome without captcha, there is a vertical scrollbar.

I have made the frame even a bit more bigger.

avatar zero-24
zero-24 - comment - 1 May 2018

@brianteeman can you please approve the language changes (adding the string for Captcha) as the requested changes had been implemented. Thanks.

avatar Quy
Quy - comment - 2 May 2018

Probably beyond the scope of this PR, can we limit the length of subject and sender to curtail spam? Currently, you can submit a paragraph of text/links in the sender field that will be inserted into the body of the email.

avatar zero-24
zero-24 - comment - 2 May 2018

Probably beyond the scope of this PR, can we limit the length of subject and sender to curtail spam? Currently, you can submit a paragraph of text/links in the sender field that will be inserted into the body of the email.

Yes i would call that out of scope but i think we can do that in a following PR when this gets merged. Can you open a issue so we can keep track on that?

avatar zero-24
zero-24 - comment - 3 May 2018

Fixed thanks @Quy

avatar Quy
Quy - comment - 3 May 2018

I have tested this item successfully on 65b0c3b


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

avatar Quy Quy - test_item - 3 May 2018 - Tested successfully
avatar Quy Quy - test_item - 3 May 2018 - Tested successfully
avatar infograf768
infograf768 - comment - 4 May 2018

Tested fine here. Will mark test OK after the change suggested by @laoneo

avatar Quy
Quy - comment - 5 May 2018

I have tested this item successfully on 7e70b61


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

avatar Quy Quy - test_item - 5 May 2018 - Tested successfully
avatar Quy
Quy - comment - 10 May 2018

Ping @infograf768 Thanks.

avatar zero-24
zero-24 - comment - 16 May 2018

@infograf768 as the requested change have been implemented can you please do your test so we can move that into 3.8.8? Thanks!

avatar zero-24
zero-24 - comment - 20 May 2018

Well this is going to be delayed to 3.8.9. :( @infograf768 can you please mark your test now so we can finally release it in 3.8.9?

avatar infograf768
infograf768 - comment - 21 May 2018

I have tested this item successfully on 7e70b61


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

avatar infograf768 infograf768 - test_item - 21 May 2018 - Tested successfully
avatar infograf768 infograf768 - edited - 21 May 2018
avatar infograf768 infograf768 - change - 21 May 2018
Status Pending Ready to Commit
Labels
avatar infograf768
infograf768 - comment - 21 May 2018

RTC. Thanks.


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

avatar infograf768
infograf768 - comment - 21 May 2018

RTC. Thanks.


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

avatar zero-24
zero-24 - comment - 21 May 2018
avatar mbabker mbabker - change - 23 May 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-05-23 23:53:11
Closed_By mbabker
Labels Added: ?
avatar mbabker mbabker - close - 23 May 2018
avatar mbabker mbabker - merge - 23 May 2018

Add a Comment

Login with GitHub to post a comment