Big patch for translation, localization, messages, redirects, remove register override [#1202070]

I had the need for this module but noticed a number of issues posted with available patches that really need to be implemented for a stable module. Working of the 6.x-1.x_dev version I have applied a number of, IMO critical, patches that fix operation:

- All but last of @lotyrin's patches from #339120: Redirect issues
- @jimkeller's patch for custom redirect location from #1114824: Custom 403 redirect for anonymous users as well [patch included]
- Made a proper fix for @Thomas_Zahreddin isuue with translation from #1138562: Integrate with i18n_variable

I have also:

- Added a new configurable message for authenticated users that still have access denied
- Removed authenticated user handling when trying to register when already logged in

Here is the logic for changes:

- Drupal goto should be used without 'exit' calls so that other modules can coexist with this one
- Message text is saved in default language without translation but all displays of messages are wrapped in t() to allow translation. Form input stored values should never be wrapped in t().
- Register redirect removed because it really does not help the user. There is another module, Already In, that handles this a lot better by simply redirecting an authenticated/logged-in user to their profile page if they try to login again, request password, or create an account. I have tested this patched module with Already In and they work harmoniously.

I will attach a patch in the next post.

Comment	File	Size	Author
#4	r4032login_many_fixes-1202070-3.patch	7.94 KB	tinker
#4
#1	r4032login_many_fixes-1202070-1.patch	7.99 KB	tinker
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tinker CreditAttribution: tinker commented 28 June 2011 at 01:54

File	Size
r4032login_many_fixes-1202070-1.patch	7.99 KB

Please provide feedback or issues. Let me know if there are any problems in getting this patch applied to dev. Thanks.

Comment #2

Heorhi Lazarevich CreditAttribution: Heorhi Lazarevich commented 16 August 2011 at 01:11

Thank you, tinker. Your patch works for me.

Comment #3

brunorios1 CreditAttribution: brunorios1 commented 25 August 2011 at 14:28

works for me too.
but it displays some errors in the terminal:

git apply -v r4032login_many_fixes-1202070-1.patch
r4032login_many_fixes-1202070-1.patch:87: trailing whitespace.
  
r4032login_many_fixes-1202070-1.patch:112: trailing whitespace.
                                      'Access denied. You must login to view this page.')), 'error');    
Checking patch r4032login.install...
warning: r4032login.install has type 100755, expected 100644
Checking patch r4032login.module...
warning: r4032login.module has type 100755, expected 100644
Applied patch r4032login.install cleanly.
Applied patch r4032login.module cleanly.
warning: 2 lines add whitespace errors.

Comment #4

tinker CreditAttribution: tinker commented 8 October 2011 at 09:19

Status:

Needs review

» Reviewed & tested by the community

File	Size
r4032login_many_fixes-1202070-3.patch	7.94 KB

Thanks @brunorios1 for pointing out the problems with the patch in #1. Figured out it was bad Samba share settings that changed the file permissions when I edited the files in windoze rather than my usual Linux. Cleaned the trailing white spaces too.

I have attached the cleaned patch which only removes the permission and whitespace changes.

Maintainers any chance this could get committed? I know two reviews is not much but I have this running perfectly on two sites for months.

Comment #5

lotyrin CreditAttribution: lotyrin commented 24 February 2012 at 09:12

tinker, I landed my patches, could you make the remaining two issues separate patches and put them in their respective issues?

I might go ahead and do so if you don't beat me to it.

Comment #6

lotyrin CreditAttribution: lotyrin commented 24 February 2012 at 09:15

Category:	bug	» task
Status:	Reviewed & tested by the community	» Needs work

This is essentially a meta-issue of #1114824: Custom 403 redirect for anonymous users as well [patch included] #1138562: Integrate with i18n_variable, changing to task

Comment #7

tinker CreditAttribution: tinker commented 24 February 2012 at 23:19

@lotyrin, Well this has become a PITA. I just checked the D6 branch in GIT and I still see header location redirects with exits rather than drupal_goto.

I thought your patches fixed that?
What patches are you referring to? Can you give issue and post numbers?
Are you committing patches?

This patch was not simply a meta-issue. There are at least 6 issues fixed. I would hate to have to split this out into many separate issues because a number of the fixes completely rework redirection. This means I would have to do additional work to create incremental patches that function in still buggy states .Then provide other patches that remove the buggy states (with the incremental patches) and properly fix everything.

Issues fixed in this patch include:

Fix translation issues properly
Fix defined messages not being used in errors
Remove authenticated user redirect -> use Already in Module
Add another access denied error message and handling for when user is already logged in but still does not have permission
Remove header redirection and exit statements use drupal_goto instead so module will coexist with others
Make sure destination is passed correctly when needed
Make sure destination uses url alias if available
Fix redirect if site is in sub directory
Fix typos and poor descriptions

I'd be happy to re-roll this patch as a whole for newer dev version.

Comment #8

tinker CreditAttribution: tinker commented 26 February 2012 at 01:10

Status:

Needs work

» Postponed (maintainer needs more info)

Comment #9

lotyrin CreditAttribution: lotyrin commented 26 February 2012 at 15:09

They should be drupal_exit() now (as they were in d7 branch, wanted to keep them the same).

http://drupalcode.org/project/r4032login.git/commitdiff/153231f

http://drupalcode.org/project/r4032login.git/blob/refs/heads/6.x-1.x:/r4...

Comment #10

tinker CreditAttribution: tinker commented 28 February 2012 at 02:25

@lotyrin So how do we to proceed? As you can see there are many issues. I don't think that your patches correctly fix redirection. Redirection should always use drupal_goto() and not header redirects. That's why the function exists. Using drupal_exit() will cause problems.

Comment #11

lotyrin CreditAttribution: lotyrin commented 28 February 2012 at 18:05

I agree. The thing with that approach (and I did take that approach before.) Is that preventing recursive redirect requires internal knowledge of drupal_goto(). (Which superglobal it pulls 'destination' out of.) Perhaps it's not too late to change D8's drupal_goto to be a little smarter to where that's not the case.

But instead of making that call, I decided to merge D7's use of drupal_exit().

If you open an issue for switching to drupal_goto(), especially one that illustrates issues with the current solution, I still think that is probably a good idea. Also, if there's anything else from this big patch that should be an issue, please open them.