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

Accept $_GET parameters &list=x,y,z (or just &list=x) and &htmlemail=1 #522

Merged
merged 1 commit into from May 23, 2019

Conversation

Projects
None yet
5 participants
@lwcorp
Copy link
Contributor

commented Apr 28, 2019

Description

$_REQUEST['email'] and $_REQUEST['emailconfirm'] are already allowed, so let's also support the other two fields.

Related Issue

https://mantis.phplist.org/view.php?id=15235

Screenshots (if appropriate):

@michield

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

👍

@suelaP

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@lwcorp sorry for the delay on this. I am trying to debug a problem with my local set up (not related to these changes) to be able and properly test this.

@xh3n1 xh3n1 self-requested a review May 10, 2019

@@ -716,6 +716,10 @@ function ListAvailableLists($userid = 0, $lists_to_show = '')
global $tables;
if (isset($_POST['list'])) {
$list = $_POST['list'];
} elseif (!isset($_POST["subscribe"]) && isset($_GET['list'])) {
$list_value = "signup";
$list_values = explode(",", $_GET["list"]);

This comment has been minimized.

Copy link
@xh3n1

xh3n1 May 13, 2019

Member

@lwcorp Looks good, but I would suggest using (int)$_GET["list"] to make sure that the parameter is Integer.

This comment has been minimized.

Copy link
@lwcorp

lwcorp May 13, 2019

Author Contributor

Thanks, but my feature allows multiple lists, thus the "explode" command.
Please enter the link in the description and you'll see the example of ?p=subscribe&list=x,y,z
Actually, I've just edited the topic to include the examples from inside the link.

This comment has been minimized.

Copy link
@xh3n1

xh3n1 May 13, 2019

Member

but x, y, z are expected to be Integers, right?

This comment has been minimized.

Copy link
@lwcorp

lwcorp May 13, 2019

Author Contributor

True. I guess you can check if it contains only numbers and (optional) commas, like /(\d+,*)+/

This comment has been minimized.

Copy link
@xh3n1

xh3n1 May 22, 2019

Member

@lwcorp yes- that's what I had in mind. Would it be ok with you implementing the change?

This comment has been minimized.

Copy link
@lwcorp

lwcorp May 22, 2019

Author Contributor

How do I edit my commit? All you need to do is change:
} elseif (!isset($_POST["subscribe"]) && isset($_GET['list'])) {
into:
} elseif (!isset($_POST["subscribe"]) && isset($_GET['list']) && preg_match("/^(\d+,)*\d+$/", $_GET['list'])) {

@lwcorp lwcorp changed the title Accept $_GET parameters &list[x]=signup and &htmlemail=1 Accept $_GET parameters &list=x,y,z (or just list=x) and &htmlemail=1 May 13, 2019

@lwcorp lwcorp changed the title Accept $_GET parameters &list=x,y,z (or just list=x) and &htmlemail=1 Accept $_GET parameters &list=x,y,z (or just &list=x) and &htmlemail=1 May 13, 2019

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@xh3n1 Review still in progress?

@xh3n1

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@samtuke Waiting for @lwcorp to add the check, but it's still ok to be merged.

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@lwcorp Are you planning to make the discussed amendment?

@lwcorp

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Not sure I understand. @xh3n1 asked me a yes/no question 8 days ago and I immediately replied to it. @xh3n1 can you follow my answer and tell us how to proceed?

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Just push additional commits to the same branch in order to update this pull request. The new commits will show up on this page automatically. Eg commit and push in the same way you did for the original pull request.

@lwcorp

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

The problem is it's "unknown":
image

Does it mean I have to submit the patch from scratch?

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

That's very strange - no, I'll merge this, then you can create a separate PR without having to re-do the existing changes. Thanks!

@samtuke samtuke merged commit 3ab5ba2 into phpList:master May 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lwcorp

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

It's because I did the original pull directly without forking, hoping it won't require edits since I've been using this code for years.
This time I forked so you can find your requested data verification (plus a similar yet non requested one) at #543.

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Great, thank you. It will be reviewed shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.