Skip to content

PHPLIB-572: Add debugging tools #996

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

Merged
merged 5 commits into from
Oct 25, 2022

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Oct 20, 2022

PHPLIB-572

This PR adds the two debugging scripts from https://github.com/jmikola/buildfest2021 to a new tools directory.

The connect.php script attempts to connect to a specified MongoDB deployment using socket functions, bypassing the extension. This can be used in case of connection errors when using the library.
The detect-extension.php script can be used to debug why the extension isn't being loaded (in any SAPI). I've modified the script compared to the version in the original repository:

  • The script now checks for the existence of an extension file in the extension dir and outputs appropriate messages
  • Replaced PHP_EXTENSION_DIR constant with ini_get('extension_dir'). I didn't know this before, but the constant always contains the default extension directory which may be overridden by the extension_dir ini setting. In that case, the constant directs us to the wrong directory.

I've also added language to the FAQ page to point out these tools and show usage examples.

@alcaeus alcaeus requested a review from jmikola October 20, 2022 14:23
@alcaeus alcaeus self-assigned this Oct 20, 2022
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

@@ -53,6 +53,20 @@ SAPI. Additionally, :php:`php_ini_loaded_file() <php_ini_loaded_file>` and
:php:`php_ini_scanned_files() <php_ini_scanned_files>` may be used to determine
exactly which INI files have been loaded by PHP.

To debug issues with the extension not being loaded, you can use the
``detect-extension`` script provided in the tools directory. You can run this
script from the CLI or include it in a script accessible via your web server.
Copy link
Member

Choose a reason for hiding this comment

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

If we expect people might run these scripts from a web SAPI, would it make sense to conditionally wrap the output in <pre>? Or I suppose people can just do that themselves -- there's really no reason to make these more complicated than they already are.

$version = phpversion($extension);

if (PHP_SAPI !== 'cli') {
echo '<pre>';
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I forgot I added this. It is notably omitted from connect.php, although I don't recall why that is.

Does it make sense to keep this here? Note that we never close this tag later, so we might be better off removing this and suggesting web SAPI users call this like <pre><?php require(...); ?></pre>.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also close the <pre> tag before exiting - the missing closing tag doesn't matter when the script is run on its own, but could cause problems if the script was included in between other "stuff".

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with either of these so long as we do the same for all scripts:

  • Wrap output in <pre></pre> when PHP_SAPI is not cli
  • Ignore any attempt for format the output and just tell users to use <pre> themselves in the docs

Slight preference for the second approach since it makes the scripts less complicated.

If you want to go with the first approach I think the cleanest way to do this would be something like the following so we don't need to worry about multiple exit points:

if (PHP_SAPI !== 'cli') {
    echo '<pre>';
    register_shutdown_function(function() { echo '</pre>'; });
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the existing echo and documented the code. Using register_shutdown_function wouldn't echo the closing pre tag after returning from the corresponding script, but when the entire execution context finishes, which could leave users with potentially broken output. Note that I've documented this for detect-extension.php but not for the connection tool as I believe the latter won't be run in the web SAPI as often as the extension tool.

Copy link
Member

Choose a reason for hiding this comment

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

Using register_shutdown_function wouldn't echo the closing pre tag after returning from the corresponding script, but when the entire execution context finishes

Oh, good point there. This works for me.

@alcaeus alcaeus requested a review from jmikola October 24, 2022 06:52
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@alcaeus alcaeus merged commit 56e8a05 into mongodb:master Oct 25, 2022
@alcaeus alcaeus deleted the phplib-572-debugging-tools branch October 25, 2022 06:10
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