Skip to content

NewFunctions: Add new OCI functions#545

Merged
wimg merged 1 commit intomasterfrom
php-7.2/new-oci-functions
Dec 21, 2017
Merged

NewFunctions: Add new OCI functions#545
wimg merged 1 commit intomasterfrom
php-7.2/new-oci-functions

Conversation

@jrfnl
Copy link
Copy Markdown
Member

@jrfnl jrfnl commented Dec 18, 2017

@jrfnl jrfnl added this to the 8.1.0 milestone Dec 18, 2017
@jrfnl jrfnl requested a review from wimg December 18, 2017 04:54
@wimg
Copy link
Copy Markdown
Member

wimg commented Dec 18, 2017

Waaaah... didn't see this one before I created my own version. The big question is : should we target 7.0 or 7.1 ?

@jrfnl
Copy link
Copy Markdown
Member Author

jrfnl commented Dec 18, 2017

The big question is : should we target 7.0 or 7.1 ?

I was thinking about that last night as well. The reason I've chosen the 7.1.x versions is that the function is 100% fine if 7.1.7/9 or higher is the "targetVersion" (testVersion) for PHPCompatibility.
With 7.0.xx, 7.1.0 would pass without issue, but in reality does not support the function yet.

@jrfnl
Copy link
Copy Markdown
Member Author

jrfnl commented Dec 18, 2017 •
edited
Loading

There are some more cases like this here and there. We may want to take a principle decision (always use lowest/highest PHP version where the function first appeared/was deprecated in last) and verify that the logic is applied consistently throughout the PHPCompatibility sniffs.

[Edit]: Oh and add it to the contributors guidelines so it's documented.

@jrfnl jrfnl mentioned this pull request Dec 18, 2017
Merged
@wimg wimg merged commit a1de588 into master Dec 21, 2017
@wimg wimg deleted the php-7.2/new-oci-functions branch December 21, 2017 21:21
@wimg wimg mentioned this pull request Dec 21, 2017
Closed
@jrfnl jrfnl mentioned this pull request Dec 21, 2017
Open
@jrfnl
Copy link
Copy Markdown
Member Author

jrfnl commented Dec 21, 2017

I've created a new issue for the "consistency when something is added/removed in multiple versions" question.

@jrfnl
Copy link
Copy Markdown
Member Author

jrfnl commented Dec 23, 2017

And now I know why I didn't pull this before: php/php-src@9379b31#diff-7748eb3bfdd3bf962553f6f9f2723c45

@ghost ghost mentioned this pull request Dec 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wimg wimg wimg approved these changes

Assignees

No one assigned

Labels

Type: enhancement

Projects

None yet

Milestone

8.1.0

Development

Successfully merging this pull request may close these issues.

2 participants

@jrfnl @wimg