-
Notifications
You must be signed in to change notification settings - Fork 736
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
Rewrite MySQLnd statistic page #3357
Conversation
b21f688
to
599f0fa
Compare
Thanks, I am in the process of reviewing this but there are a lot of changes here and it has raised many questions for me. I hope to get back to you with something tomorrow or the day after tomorrow. |
No worries, I also had many questions while rewriting this and expected this to take a while to review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR. I appreciate how much work you have put into this. I like how the page looks now and I am happy with most of your changes. I have highlighted some typos and answered your TODOs. I have also left some TODOs for myself as I investigate these statistics. I'll need some time to do this and in the meantime you can fix the issues I identified.
I have a gripe with this whole feature. I would love it if we could deprecate the statistics and remove them soon. I think I lost some of my sanity while reading this page. But that is off-topic.
<!-- TODO: XInclude/sync description of INI setting | ||
ini.mysqlnd.net-cmd-buffer-size with this statistic --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the link not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, when I had a link at the INI setting description it was just a copy paste from it, so the INI setting description might need a rewrite, which is why I put that TODO comment here
Thank you for the review! I tried to address each review comment in a separate commit so it's easy to figure out what changed. Still missing a couple as it seemed you needed to think about them some more. :) |
Made the suggested changes |
@kamil-tekiela do you have any more comments? I would like to merge this sooner than later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address these last remarks of mine and then you can merge, I will look at the rest of it sometime in the future once I have a moment.
- Remove personalization - Improve document structure - DocBook 5.2 conformance
This reverts commit 2f7be48.
This is probably easier by comparing to the actual live page: https://www.php.net/manual/en/mysqlnd.stats.php
The only things that really ought to be looked at, are my
TODO
comments.