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

FIX: "Undefined index: HTTP_HOST" for no web server #60

Merged
merged 5 commits into from
Sep 5, 2018
Merged

FIX: "Undefined index: HTTP_HOST" for no web server #60

merged 5 commits into from
Sep 5, 2018

Conversation

gddh
Copy link
Contributor

@gddh gddh commented Aug 24, 2018

  • Added a null guard for the dsq_filter_rest_url
  • Added functionality tests to ensure dsq_filter_rest_url does not error when _SERVER host is not set.

Address: #57

Derrick Lin added 5 commits August 24, 2018 10:41
…ests to ensure dsq_filter_rest_url does not error when _SERVER host is not set.
…ests to ensure dsq_filter_rest_url does not error when _SERVER host is not set.
@wedamija wedamija merged commit 04e9f8e into disqus:master Sep 5, 2018
@SeBsZ
Copy link

SeBsZ commented Dec 14, 2018

Hi @gddh and @wedamija ,
can this fix please be released in a new release? We're using composer with wpackagist and we can't currently pull in the version of disqus-wordpress-plugin that contains this fix. Thank you!

@jaryszek
Copy link

jaryszek commented Dec 14, 2018 via email

@SeBsZ
Copy link

SeBsZ commented Dec 14, 2018

@jaryszek what do you mean? It's right here in this thread #60

@jaryszek
Copy link

jaryszek commented Dec 14, 2018 via email

@SeBsZ
Copy link

SeBsZ commented Dec 14, 2018

Actually this was fixed in less than 1 month from me opening the issue. It just hasn't been released in a proper release.

@jaryszek
Copy link

jaryszek commented Dec 14, 2018 via email

@archon810
Copy link

Let me ping some folks who can help push this through.

@SeBsZ
Copy link

SeBsZ commented Dec 31, 2018

@gddh, I realized the fix is not entirely complete. Even with this fix I kept getting more notices:

[31-Dec-2018 11:04:37 UTC] PHP Notice: Undefined index: host in .../wp-content/plugins/disqus-comment-system/admin/class-disqus-admin.php on line 163

Here's my suggested fix, where I check for the presence of the 'host' key in $rest_url as well:

public function dsq_filter_rest_url( $rest_url ) {
        $rest_url_parts = parse_url( $rest_url );
        if ( array_key_exists( 'host', $rest_url_parts ) ) {
            $rest_host = $rest_url_parts['host'];
            if (array_key_exists('port', $rest_url_parts)) {
                $rest_host .= ':' . $rest_url_parts['port'];
            }

            $current_host = isset($_SERVER['HTTP_HOST']) ? $_SERVER['HTTP_HOST'] : $rest_host;

            if ($rest_host !== $current_host) {
                $rest_url = preg_replace('/' . $rest_host . '/', $current_host, $rest_url, 1);
            }
        }

        return $rest_url;
    }

@jaryszek
Copy link

jaryszek commented Dec 31, 2018 via email

@archon810
Copy link

@wedamija @gddh The 2nd part of the fix is still unreleased and needed. Please consider fixing this fully.

@archon810
Copy link

@dmatt @Hovspian @tterb We still get lots of notices from line 162. Can you please finally push through the fix to this 3-year-old bug?

@dmatt
Copy link
Contributor

dmatt commented May 21, 2021

Thanks @archon810 and @SeBsZ. Here is the suggested fix which we will pull in soon 03c66dd. In the meantime, feel free to create your own PR and ping me on it if you would prefer to be the author. Do you have reproduction steps for forcing $rest_url to have no host? In my testing of running a script via CLI the host key was still present. Thanks.

@SeBsZ
Copy link

SeBsZ commented May 24, 2021

Hi @dmatt,
Thanks for the fix. To reproduce, simply set these in wp-config.php:

define('WP_HOME', 'https://' );
define('WP_SITEURL', 'https:///wordpress');

We're setting these dynamically in wp-config, not in wp_options, so when there is no host present, Disqus will throw the notice.
Please let me know when the fix has been pulled in, thank you!

@dmatt
Copy link
Contributor

dmatt commented May 24, 2021

Thanks for that tip @SeBsZ, I've reproduced the notice now and will ping you when the fix is pulled in.

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

Successfully merging this pull request may close these issues.

6 participants