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

return error code on FAIL in ajax subscribe #432

Merged
merged 3 commits into from Nov 3, 2018

Conversation

Projects
None yet
5 participants
@maltfield
Contributor

maltfield commented Nov 2, 2018

See bug report (quoted below link)

This is apparently a long-standing issue with the community-recommended way for users to be able to add subscribers to phplist from an external domain using ajax in phplist3 as described in this thread:

The code in the zip file attached to the above thread uses jquery's ajax function, and it defines a function success() that's triggered when the ajax call succeeds and a error() function that's triggered when the ajax call failed. The problem is: phplist merely prints "FAIL" in the response when the subscribe attempt fails, but the status code remains a 200.

IMHO, the ajax form should not have to search for the string "FAIL" in the response in order to detect a failure. Instead, it should check the status code of the HTTP response from the server. This can be achieved, for example, by throwing an exception in PHP--which will make PHP return a 500 Internal Server Error.

Here is the relevant code for handling ajax subscription queries on phplist's side:

  • case 'asubscribe': //# subscribe with Ajax
    $_POST['subscribe'] = 1;
    if (isset($_GET['email']) && !isset($_POST['email'])) {
    $_POST['email'] = $_GET['email'];
    }
    foreach (explode(',', $GLOBALS['pagedata']['lists']) as $listid) {
    $_POST['list'][$listid] = 'signup';
    }
    $_POST['htmlemail'] = 1; //# @@ should actually be taken from the subscribe page data
    $success = require 'admin/subscribelib2.php';
    $result = ob_get_contents();
    ob_end_clean();
    if (stripos($result, $GLOBALS['strEmailConfirmation']) !== false ||
    stripos($result, $pagedata['thankyoupage']) !== false
    ) {
    if (!empty($pagedata['ajax_subscribeconfirmation'])) {
    $confirmation = $pagedata['ajax_subscribeconfirmation'];
    } else {
    $confirmation = getConfig('ajax_subscribeconfirmation');
    }
    if (empty($confirmation)) {
    echo 'OK';
    } else {
    echo $confirmation;
    }
    exit;
    } else {
    echo 'FAIL';
    }
    break;

My recommendation is to add the line:

throw new Exception( "Error: Subscribe attempt failed!" );

after the "echo 'FAIL';" line here:

maltfield and others added some commits Sep 15, 2018

Michael Altfield
Added a line to throw an exception when the phplist ajax subscribe en…
…dpoint fails to add a new subscriber. Throwing this exception causes php to return a 500 Internal Server Error, rather than the existing functionality which just returns a 200 error (merely containing 'FAIL' in the body of the response). Consequently, the ajax client thinks the failure is actually a success, since the http status code is still a 200.

This change makes the response more robust and easier for ajax clients (such as the jquery example linked below) to catch errors from phplist.

 * https://discuss.phplist.org/t/ajax-subscribe-api/974

For more information, please see the following phplist bug report:

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

@samtuke samtuke requested a review from xh3n1 Nov 3, 2018

@samtuke

This comment has been minimized.

Contributor

samtuke commented Nov 3, 2018

@maltfield Thanks for the PR 👍

@michield

This comment has been minimized.

Member

michield commented Nov 3, 2018

👍

@xh3n1

xh3n1 approved these changes Nov 3, 2018

@samtuke samtuke merged commit e01b68f into phpList:master Nov 3, 2018

@bramley

This comment has been minimized.

Contributor

bramley commented Nov 4, 2018

This change doesn't appear to have the expected outcome, causing a 500 error to be returned.

throw new Exception( "Error: Subscribe attempt failed!" );

In my test the status was still 200.
image
The web server is running LiteSpeed, so whether an unhandled php exception causes a server error might be dependent on the environment. The usual way of returning a specific status code though is the header() function.
But returning 500 for the case of someone already subscribed seems a bit excessive.

A better solution would be to have a configurable message that is returned, similar to that for a successful request
image
Then the client doesn't need to know whether the subscription was successful or not. It just displays the returned text.

@maltfield

This comment has been minimized.

Contributor

maltfield commented Nov 7, 2018

@bramley thanks for the testing & feedback! For us, this definitely returns 500 on our setup with an apache backend. I wonder if the difference in your environment is your php config or web server (LiteSpeed)?

I thought about setting the header--which would be the most exact way to achieve this desired result (and I do believe it's a must that the return code to be non-200 on error), but I intentionally avoided that solution since--iirc--calling header() must be done before any output is sent, and could therefore break things either as they are now or in the future.

@maltfield

This comment has been minimized.

Contributor

maltfield commented Nov 7, 2018

@bramley did you test this in dev or production? Try setting display_errors to 0 in your php config and see if it's still returning a 200. Production, of course, should have display_errors set to off.

Per https://www.reddit.com/r/PHP/comments/4iubg1/why_doesnt_php_send_a_500_status_code_if_an/

@maltfield

This comment has been minimized.

Contributor

maltfield commented Nov 7, 2018

Another possible option would be to use http_response_code(), which--at least from the documentation--looks like it might not have the same "must call before output" limitations as header()

@bramley

This comment has been minimized.

Contributor

bramley commented Nov 7, 2018

The site is shared hosting so is quite restricted in changing the php configuration, but display_errors is off.

phplist starts output buffering at the start of index.php, so header() would be ok (which is how I tested it previously when the exception did not have the desired effect) but http_response_code() should be ok too.

@michield

This comment has been minimized.

Member

michield commented Nov 8, 2018

Yes, it should be ok to use header()

maltfield added a commit to maltfield/phplist3 that referenced this pull request Nov 23, 2018

on some systems, an exception automatically returns a 500 error. On o…
…thers, it does not. Rather than use header(), we add a line to set the response code more robustly with http_response_code(). For more info, see my previous PR related to this change here:

 * phpList#432

@maltfield maltfield referenced this pull request Nov 23, 2018

Open

Ajax fail do500 #445

@maltfield

This comment has been minimized.

Contributor

maltfield commented Nov 23, 2018

@bramley I reorganized my sandbox and created a new PR using http_response_code(). Please verify that it does return a 500 for you.

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