Skip to content

PHPC-1431, PHPC-1435: Fix Session::startTransaction() param and arginfo #1015

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 3 commits into from
Aug 29, 2019

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Aug 28, 2019

@jmikola jmikola requested a review from alcaeus August 28, 2019 15:46
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM. The signature claims a default value of null, so this makes sense. You could add a test to explicitly pass null to the method, but it's also fine without one.

@jmikola
Copy link
Member Author

jmikola commented Aug 28, 2019

You could add a test to explicitly pass null to the method, but it's also fine without one.

I checked if we had any other phpt tests for passing null to option array params elsewhere and didn't find any. Not sure that's worth a ticket to go and add tests for every method.

Side note: the proto comments above each method are purely for internal documentation. The important data is actually the method's arg info, which I just realized is incorrect. That is relevant for querying the extension API via Reflection

@jmikola jmikola requested a review from alcaeus August 28, 2019 19:48
@jmikola
Copy link
Member Author

jmikola commented Aug 28, 2019

Note to self: reference PHPC-1435 in second commit before merging.

@jmikola jmikola changed the title PHPC-1431: Accept null for Session::startTransaction() options PHPC-1431, PHPC-1435: Fix Session::startTransaction() param and arginfo Aug 28, 2019
@alcaeus
Copy link
Member

alcaeus commented Aug 29, 2019

The important data is actually the method's arg info, which I just realized is incorrect.

After double-checking, I should've seen that. I checked the ZEND_ARG_INFO call and didn't see anything about it being optional or not. ZEND_ARG_ARRAY_INFO makes more sense 👍

@alcaeus alcaeus force-pushed the phpc-1431 branch 2 times, most recently from e875ea7 to 0a86456 Compare August 29, 2019 11:49
@alcaeus
Copy link
Member

alcaeus commented Aug 29, 2019

@jmikola I've taken the liberty of amending your commit message and fixing the failing test. Since the behaviour will be different on PHP 5 and PHP 7, I've split the test.

@alcaeus alcaeus force-pushed the phpc-1431 branch 2 times, most recently from 98bab6c to c139797 Compare August 29, 2019 12:14
Copy link
Member Author

@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.

I appear to be unable to approve my own pull request, but LGTM just the same.

===DONE===
<?php exit(0); ?>
--EXPECTF--
OK: Got TypeError
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 find it very interesting that the arg info is what actually changed behavior here. This suggests that PHP might raise the TypeError before the method is even called (perhaps similar to how type hints would be checked in userland code).

PHP 7.x apparently used to emit a warning just like PHP 5 in the original version of session-startTransaction_error-003.phpt, and I expect that probably originated in the zend_parse_parameters() call. Perhaps @sgolemon can confirm if this is an accurate assessment.

if ($e === null) {
echo "FAILED: Expected $exceptionname thrown, but no exception thrown!\n";
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Can I get a "what what" for object calisthenics 🙌

jmikola added a commit that referenced this pull request Aug 29, 2019
@jmikola jmikola merged commit d5e0779 into master Aug 29, 2019
@jmikola jmikola deleted the phpc-1431 branch August 29, 2019 14:45
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