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

Add selectbox for server name #460

Merged
merged 1 commit into from
Jan 20, 2022
Merged

Conversation

ossinkine
Copy link
Contributor

I've added select box for selecting server name but not just a text field.

I've implemented it for MongoDB only because PDO stores SERVER data in a text field and I have no idea how to select one of parameters from it.

Also an index is required for performance purpose.

@ossinkine
Copy link
Contributor Author

@glensc if this is ok for you I'll try to add tests

@glensc
Copy link
Contributor

glensc commented Jan 18, 2022

Are you able to rebase against current 0.20.x, if so, add row to compatibility matrix that was added to readme in #461

if not, I'll rebase myself and force push its branch

@ossinkine
Copy link
Contributor Author

I'll do

@ossinkine ossinkine force-pushed the select-server-name branch 3 times, most recently from da8f381 to 06618b2 Compare January 18, 2022 20:30
@ossinkine
Copy link
Contributor Author

I've added a test for a new searcher functionality and also added a row to the matrix you requested

README.md Outdated
@@ -271,11 +272,13 @@ storage.)
| Searcher::getAllWatches() | ✓ | ✓ [#435] |
| Searcher::truncateWatches() | ✓ | ✓ [#435] |
| Searcher::stats() | ✗ [#305] | ✓ |
| Searcher::getAllServerNames() | ✓ | ✗ [#460] |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| Searcher::getAllServerNames() ||[#460] |
| Searcher::getAllServerNames() |[#460] ||

/**
* Get all the known server names.
*
* @return array|false array of server names or false if not supported
Copy link
Contributor

Choose a reason for hiding this comment

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

i think array|null is better typing, then you can use ?array as native return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add native type because it's not used in other methods

Copy link
Contributor

Choose a reason for hiding this comment

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

please add the type as asked.

current minimum PHP version is 7.2 so all language features compatible with that version can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@ossinkine
Copy link
Contributor Author

@glensc Please review

@glensc glensc added this to the 0.20.1 milestone Jan 20, 2022
@glensc glensc merged commit 22f031c into perftools:0.20.x Jan 20, 2022
@ossinkine
Copy link
Contributor Author

@glensc when do you plan to release 0.20.1 to be able to use this feature?

@glensc
Copy link
Contributor

glensc commented Feb 1, 2022

I was hoping someone helps out with automatic releases:

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