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

Check that host key exists before using in dsq_filter_rest_url #106

Merged
merged 2 commits into from
May 26, 2021

Conversation

dmatt
Copy link
Contributor

@dmatt dmatt commented May 25, 2021

Description

In environments where the $rest_url does not contain a host, like when running PHP scripts on the command line via Supervisord, php warns with a notice. To prevent these notices, this PR adds a check for the existence of 'host' before trying to use it in the dsq_filter_rest_url function.

Motivation and Context

Fixes issue #57, discussed more in #60.

How Has This Been Tested?

  1. Build the docker image using these instructions https://github.com/disqus/disqus-wordpress-plugin#local-testing
  2. Run bash in the docker container with docker exec -it disqus-wordpress-plugin_wordpress_1 /bin/bash
  3. Create a script file my-script.php with the following
<?php
print("Hello!");
?>
  1. Edit wp-config.php to remove the host:
define('WP_HOME', 'https://' );
define('WP_SITEURL', 'https:///wordpress');
  1. run a script to confirm the absence of the PHP notice Undefined index: HTTP_HOST" for no web server with docker exec -it disqus-wordpress-plugin_wordpress_1 /bin/bash wp eval-file my-script.php

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • All new and existing tests passed.

@dmatt dmatt requested review from gddh, hlgott1593 and tterb May 25, 2021 18:05
@hlgott1593 hlgott1593 merged commit 6d43e80 into master May 26, 2021
@hlgott1593
Copy link
Contributor

Tested with reproduction steps

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.

2 participants