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

customer order view workaround for "+" in email addresses #1199

Closed
wants to merge 3 commits into from
Closed

customer order view workaround for "+" in email addresses #1199

wants to merge 3 commits into from

Conversation

pbek
Copy link
Contributor

@pbek pbek commented Sep 2, 2015

I added a workaround to the get the customer order view working with email addresses that include a "+" character.

First I thought only an "urlencode" was missing, but then I realized it wasn't possible to get a "+" across $Params.

@andrerom
Copy link
Contributor

andrerom commented Sep 2, 2015

+0,5 on the principle

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But, if i'm not wrong, here you are remove Email Param from the url, right? i mean, this is not valid as you won't have Email as Param anymore

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 use a normal "get" parameter in orderlist.tpl, because "+" get through with them. I give this parameter a higher priority in the customerorderview.php. Does that seem wrong to you, @crevillo?

Of course fixing the bug (?) with $Params would be the better solution, but I don't know the consequences of doing that...

Copy link
Contributor

Choose a reason for hiding this comment

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

@pbek. no, the patch is good. forget my comment. just thought that now this line
could throw a php notice or something now $Params['Email'] won't be set in this new urls.

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'm not very happy with that patch, because the real problem lies in $Params (and a missing urlencode()). ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you have in mind changing it to?

as for Params, it kind of still need to be there for bc, like is the case now in 6f5f894

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 think I fail to understand what you are talking about. :)
Changing what to? And what "bc"?

@pbek
Copy link
Contributor Author

pbek commented Sep 11, 2015

I created an issue now, because I get no reaction on the pull request...
https://jira.ez.no/browse/COM-19862

@SylvainGuittard
Copy link
Contributor

Moved to eZ Publish Project (EZP): https://jira.ez.no/browse/EZP-24798

@pbek
Copy link
Contributor Author

pbek commented Oct 8, 2015

any news here?

@andrerom
Copy link
Contributor

@pbek might want to move ca0268d into own branch against master ;)

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

@andrerom I just realised that if you push changes to your own fork an open pull request gets updated automatically... Since I want to create a pull request for fixing PHP 7.2 deprecations warning/errors I had to remove my fork, since you cannot create a new one...

@pbek pbek closed this Sep 20, 2018
@andrerom
Copy link
Contributor

andrerom commented Sep 20, 2018

hmm, should be possible, you just need to use different branches, or?

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

ah, that sounds right. so I could have created another branch from master and work with that one, I will try that for my PHP 7.2 pull request

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

Yes, thank you. I guess it's best always to create a branch for a new pull request!

@andrerom
Copy link
Contributor

andrerom commented Sep 20, 2018

Pro tip: When you fork something, remove master from your fork so local master tracks "upstream" only, and you don't need to keep on updating your own master. As a result instead you always start new work from the "real" master from the upstream project ;)

Doesn't fit in all cases, but in most it simplifies things and forces you to think in terms of one branch per feature as well ;)

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

Do you mean my locally checked out master branch or the remote master branch (of my fork)?

@pbek
Copy link
Contributor Author

pbek commented Sep 20, 2018

I meanwhile created #1390 and #1391. ;)

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