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

Workaround to the get the customer order view working with email addresses that include a "+" character #1391

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pbek
Copy link
Contributor

@pbek pbek commented Sep 20, 2018

This is a continuation of the closed pull request #1199 with suggested changes from @andrerom.

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

Travis fails with Fatal error: A database transaction in eZ Publish failed.. Since this problem already occured 3 days ago, I guess my pull request has nothing to do with it.

@andrerom
Copy link
Contributor

I guess my pull request has nothing to do with it.

yeah that is not relevant here.

@pkamps
Copy link
Contributor

pkamps commented Sep 20, 2018

It would be good to have steps to reproduce that issue.
I want to test if that's related to:
#1360

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

@pkamps, please see: https://jira.ez.no/browse/EZP-24798

@pkamps
Copy link
Contributor

pkamps commented Sep 20, 2018

OK, I will try if I can reproduce the issue. I honestly don't have a webshop site currently.
Would you mind to test the patch in #1360 and see if it fixes the issue for you?

@pbek
Copy link
Contributor Author

pbek commented Sep 21, 2018

@pkamps, I tested your pull request and it didn't solve the issue of showing customer information with urls like https://your-server/admin/shop/customerorderview/10/test%[email protected]

@pkamps
Copy link
Contributor

pkamps commented Sep 21, 2018

@pbek You're right, the patch from #1360 is not enough to fix the issue. I see that you fixed the missing url encoding in the template - that was missing and is not a workaround. I don't think you need to work with a query parameter ?email=. It's probably cleaner to stick with the the ezp ordered parameter.

The additional URL decoding $Email = urldecode( $Email ); is the problem. It's already decoding the query string in: https://github.com/ezsystems/ezpublish-legacy/blob/master/lib/ezutils/classes/ezsys.php#L1184

So it's safe to just remove that line from the code base.
Verdict: I suggest to add the url encoding in the template and remove the line $Email = urldecode( $Email ); from the module/view.

@pkamps
Copy link
Contributor

pkamps commented Sep 21, 2018

Oh, and maybe you still need the patch from #1360 - but I'm not sure, there is a chance it works without that patch.

@mateuszbieniek
Copy link
Contributor

mateuszbieniek commented Sep 21, 2018

@pkamps @pbek
After some checking I think I found the root of the problem:
We get $Param['Email'] from URI (eZURI), which is initated here:
https://github.com/ezsystems/ezpublish-legacy/blob/master/kernel/private/classes/ezpkernelweb.php#L1083
using eZSys::requestURI().

eZSys::requestURI() is initaited using urlencode (https://github.com/ezsystems/ezpublish-legacy/blob/master/lib/ezutils/classes/ezsys.php#L1184), then passed to eZURI::instance( eZSys::requestURI()) which again uses urlencode (https://github.com/ezsystems/ezpublish-legacy/blob/master/lib/ezutils/classes/ezuri.php#L89), so we end up with email encoded twice.

So: test%2Btest%40example.com is becoming [email protected], and then test example.com.

But as I see this is exactly what #1360 should be fixing.

@@ -15,7 +15,17 @@

$tpl = eZTemplate::factory();

$Email = urldecode( $Email );
// allow usage of get parameter as well which is safer for some email formats
if ( $Email )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always be true, unless $Email would be set null or false, which won't happen because of the line 10.
I sugest:

$Email = urldecode( $Email );
if ( $http->hasGetVariable( "email" ) )
{
    $Email = $http->getVariable( "email" );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mateuszbieniek, that is what I had before I followed the suggestion of @andrerom in #1199 (comment) 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now changed it back to your suggestion.

@pkamps
Copy link
Contributor

pkamps commented Sep 21, 2018

That's how we'll fix the issue in our fork:
mugoweb#119

@pbek
Copy link
Contributor Author

pbek commented Sep 26, 2018

@pkamps

That's how we'll fix the issue in our fork:
mugoweb#119

that didn't seem to work for me, the urlencoded + didn't get across $Params

@mateuszbieniek
Copy link
Contributor

I have to test it, but it seems like mugoweb#119 alongside with #1360 seems like a good approach and should work.

@pbek
Copy link
Contributor Author

pbek commented Sep 26, 2018

I would also like if my workaround with the extra get-parameter isn't needed any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants