Skip to:
Content

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#2517 closed defect (bug) (fixed)

[patch] Confirm Password to change Password or Email (dup check) on General Settings

Reported by: nuprn1 Owned by:
Milestone: 1.5 Priority: normal
Severity: Version:
Component: Core Keywords: has-patch, needs-testing
Cc: hashmore@…

Description

Attaching a patch file that modifies

bp-core-settings.php

Just a simple edit to check for the correct current password before changing the actual password and/or email address.

If email address is changed - a few WordPress checks are made

sanitize_email
limited_email_domains
email_exists

Patched generated against 1.2.5.2

Attachments (6)

bp-core-settings-confirm-pw-email-check.patch (19.0 KB) - added by nuprn1 4 years ago.
2517.001.patch (4.8 KB) - added by r-a-y 4 years ago.
Patch based on nuprn1's botched patch ;)
2517.002.patch (4.8 KB) - added by r-a-y 4 years ago.
Updated fix based off of nuprn1's suggestion
2517.003.patch (5.1 KB) - added by r-a-y 4 years ago.
Added missing global variables
2517.004.patch (6.0 KB) - added by r-a-y 4 years ago.
Cleaned up form and misc. settings page titles
2517.005.patch (6.0 KB) - added by r-a-y 4 years ago.
Removed h3 from gettext, added h3 to plugins.php template file

Download all attachments as: .zip

Change History (28)

comment:1 nuprn14 years ago

sorry, i can't get an updated winmerge to generate a proper diff patch - so this is a whole file replacement.

comment:2 jeffsayre4 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from 1.3 to 1.2.6

comment:4 pisanojm4 years ago

I got the following error:
Parse error: syntax error, unexpected T_IF in /home/musicpln/public_html/wp-content/plugins/buddypress/bp-core/bp-core-settings.php on line 6

using bp 1.2.5.2 wp3.0

r-a-y4 years ago

Patch based on nuprn1's botched patch ;)

r-a-y4 years ago

Updated fix based off of nuprn1's suggestion

r-a-y4 years ago

Added missing global variables

r-a-y4 years ago

Cleaned up form and misc. settings page titles

comment:5 pisanojm4 years ago

Wow... that was an extensive patch ray... Ok, works for me! :)

comment:6 johnjamesjacoby4 years ago

Possible to repatch this without the HTML in the gettext?

r-a-y4 years ago

Removed h3 from gettext, added h3 to plugins.php template file

comment:7 r-a-y4 years ago

Repatched to remove <h3> from gettext, but the headings are missing in the setting templates (go to any settings or plugin page and you'll see what I mean).

Also in patch is a fix to the /bp-themes/bp-default/members/single/plugins.php template to wrap the <h3> around the "bp_template_title" do_action.

comment:8 hnla4 years ago

  • Cc hashmore@… added

Patch tested on WP 3.0 MS BP 1.2.6 Works as described no problems noted.

One thing 'Lost your password' so how have I just managed to log into my account to read that??? :P

comment:9 r-a-y4 years ago

You're right, hnla about the "Lost Your Password" link!

@JJJ, do you need a re-patch based on this?

comment:10 nuprn14 years ago

i borrowed the 'lost password' link from twitter's settings page ;)

what if you select "Remember Me" and have been logged in for a bit then forgot what your password was.

comment:11 johnleblanc4 years ago

Shouldn't email change require email verification?

comment:12 johnjamesjacoby4 years ago

How upset would anyone be if I bumped this to 1.3? Since WP core doesn't do this, I'm prone to thinking this is plugin territory?

Thoughts?

comment:13 r-a-y4 years ago

I wouldn't be upset as it's primarily Rich's work.

It could be a plugin, but would require removing and duplicating a bunch of BP settings functions.

I would at least like to see the <h3> patch make it (view the last part of the patch), since that makes sense.

Screenie: http://img717.imageshack.us/img717/6586/20100820224115.jpg

comment:14 hnla4 years ago

I would back Ray on this as bare naked character data is a crime! however my personal solution was '' :) pages with lots of words on, some big some small.

comment:15 johnjamesjacoby4 years ago

  • Milestone changed from 1.2.6 to 1.3

Would rather not force the h3 in case bp_template_title isn't used. Then we end up with an empty tag.

Better solution would be to put the h3 in the functions that use it imo. Only a few functions to patch.

Punting to 1.3 for further discussion and review. Boy am I going to hate myself for all the things that got punted real soon. :)

comment:16 r-a-y4 years ago

Couldn't you do a check on "bp_template_title" though?

Good to punt it anyway. More time to think about it ;)

comment:17 boonebgorges3 years ago

The perfect is the enemy of the good, and this is a very good patch. Letting it fester because of an <h3> seems downright silly.

If anyone wants to propose changes to bp_template_title() and how it's used, please open an enhancement ticket.

comment:18 boonebgorges3 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [3524]) Adds password verification before an email address or password change on the General Settings screen. Fixes #2517. Props nuprn1 and r-a-y

comment:19 hnla3 years ago

What is the issue perceived or otherwise with fixing template_title()?

As Boone will testify due to the lack of correct and proper semantic formatting for that tag in the template files he didn't use that function in a plugin instead opting to add a heading tag within his template_content function, result for all the right reasons is wrong sadly as we have merged data layer with presentational layer the frontend coder has no means of control of a markup tag that should rightly only exist in a template/view file (I can see the argument that says the plugin function for content being a view but nonetheless it's in a file that shouldn't be modified.

The template tag does need to wrapped in a semantic element as it's worse if it is used and bare naked cdata is output, as Ray mentioned is it not simple enough to wrap element and template function in a:
if(template_title())

I'll test that and submit a patch if necessary.

comment:20 boonebgorges3 years ago

I have no issue with fixing bp_template_title() in order to ensure greater separation between presentation and logic. I just didn't want it to hold up this ticket anymore, as it's not really relevant to the original bug report.

comment:21 r-a-y3 years ago

Boone, in changeset [3524], there's a double semi-colon at the end of line 197 in bp-core-settings.php

comment:22 boonebgorges3 years ago

r-a-y - Good eye. Also, yikes, it's been there since [3352]. Fixed in [3526]

Note: See TracTickets for help on using tickets.