Skip to content

Declare SNMP properties #6742

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

Closed
wants to merge 3 commits into from
Closed

Conversation

kocsismate
Copy link
Member

No description provided.

@nikic
Copy link
Member

nikic commented Mar 15, 2021

Heh, it looks like ext/pdo_sqlite/tests/bug44327_2.phpt tests the case where there is a column called queryString. I think you can just change the test expectation, but need to do so consistently (i.e. read queryString first in all handlers).

@kocsismate kocsismate force-pushed the stub-pdorow branch 2 times, most recently from a034766 to 2c23552 Compare March 15, 2021 20:02
@nikic
Copy link
Member

nikic commented Mar 16, 2021

The PDO part looks good to me.

@kocsismate
Copy link
Member Author

And have you come to a conclusion about the SNMP related parts?

php-pulls pushed a commit that referenced this pull request Mar 17, 2021
@nikic nikic changed the title Declare PDORow and SNMP properties Declare SNMP properties Apr 26, 2021
@kocsismate
Copy link
Member Author

I think this PR should be fairly ready - apart from the question I had above. Can you please have a look, @nikic ?

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks good, assuming CI is happy...

@kocsismate
Copy link
Member Author

Looks good, assuming CI is happy...

I haven't added the expectation for the new test case, but I'll do so during applying the change.

@kocsismate kocsismate deleted the stub-pdorow branch May 14, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants