don't alter form action unless required [#1402608]

If a form is delivered to a user then she must already be on HTTPS if she has one of the HTTPS secured roles (forced redirect in hook_init()). So it does not make sense to check that again in hook_form_alter(). Patch attached.

This also solves an incompatibility with the overlay module for me, as the overlay javascript considers a form action beginning with "https" to be an external URL. So it changes the form target to "_new" which opens a new window on form submission, which is really confusing.

Comment	File	Size	Author
#9	securepages-form-role-1402608-9.patch	882 bytes	klausi
#8	securepages-form-role-1402608-8.patch	1.05 KB	klausi
#5	1402608-securepages-form-role.patch	812 bytes	klausi
	securepages-remove-role-check.patch	776 bytes	klausi

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

grendzy CreditAttribution: grendzy commented 3 February 2012 at 02:32

Title:	Remove the role check on forms	» don't alter form action unless required
Version:	7.x-1.x-dev	» 6.x-2.x-dev
Status:	Needs review	» Patch (to be ported)

Instead, I added a check for $is_https so the action isn't changed unless needed.

Commit fcfbf84 on master

Comment #2

grendzy CreditAttribution: grendzy commented 3 February 2012 at 02:48

Status:

Patch (to be ported)

» Fixed

Thanks. The overlay thing was driving me nuts too.

Commit 5098b1f on 6.x-2.x

Comment #3

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 3 February 2012 at 12:15

Status:

Fixed

» Active

But why don't you just remove the role check all together as I suggested? This is useless code in hook_form_alter(), no?

Comment #4

grendzy CreditAttribution: grendzy commented 3 February 2012 at 17:34

Version:	6.x-2.x-dev	» 7.x-1.x-dev
Status:	Active	» Needs work

Yes, now I understand what you mean. This is indeed dead code.

Comment #5

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 8 February 2012 at 09:03

Status:

Needs work

» Needs review

File	Size
1402608-securepages-form-role.patch	812 bytes

Rerolled.

Comment #6

David4514 CreditAttribution: David4514 commented 21 April 2012 at 14:50

I too was having issues with overlays. I had included 'admin/people' as one of the pages to be secured.

With the 7.x-1.x branch of securepages (as of 04/20/12), the overlay was hanging when I clicked on the People tab. Until I browsed to a url without an overlay, it remains in a hung state.

I switched to the "master" branch of securepages and applied the patch in #5 (the patch does not apply to the 7.1-1.x branch ???).

Now, when I click on the "People" tab, it opens up, but with the overlay disabled. Is that the intended result? It is better than a hang, but not what I was expecting. I was expecting the People tab to act like the other administrative tabs, but forcing https.

Comment #7

David4514 CreditAttribution: David4514 commented 21 April 2012 at 15:00

I may have answered my own question. As described in the #952820: Drupal 7 port, overlays are incompatible with mixed http/https mode (i.e. you cannot overlay https on top of http).

Ok... now that I understand, I can live with that. I was only having the issue in the administrative area anyway.