Skip to content

Conversation

@keyurdg
Copy link
Contributor

@keyurdg keyurdg commented Aug 9, 2016

No description provided.

@nikic
Copy link
Member

nikic commented Aug 9, 2016

I'm not sure I understand this change. Is this a root-cause fix or is it only symptomatic? This will (unless I misunderstood) still leak the dbh until the end of the request, which is still a leak.


ZEND_BEGIN_MODULE_GLOBALS(pdo)
zend_long global_value;
HashTable *pdo_pdbh_to_delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing globals would likely breach non core PDO drivers. 7.1 is already in feature freeze, not talking about 7.0, so were a no go. However, from what I see, pdo_pdbh_to_delete is not required to be accessed by drivers. Thus, using a global ZEND_TLS variable instead of a global would be suitable and wouldn't breach binary compat.

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, are you saying that ABI breaks for 7.1 are no longer acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feature freeze implies no ABI/ABI changes, at least without a weighty reason and consideration. Should be to agree by RMs, just in case. Not relevant here anymore. My accent was more on mentioning the alternative way without breaching globals.

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@weltling If that's the case, can we please have an announcement on internals? I assumed that ABI breaks (not necessarily source breaks) are fine until the final release, similar to how it has been with previous versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was an announce from Davey http://news.php.net/php.internals/94330 . But i don't think it was just ok with the previous versions as well, AFAIR (and how i handled it) the practice is always to freeze both API/ABI, like fe https://fedoraproject.org/wiki/Updates_Policy#Beta_to_Pre_Release and other projects do. But there's always room for an exception, sometimes a breach is acceptable or even unavoidable, so @dshafik and @krakjoe would be in charge for that.

Thanks.

@keyurdg
Copy link
Contributor Author

keyurdg commented Aug 9, 2016

See new PR #2067 for a ABI compliant implementation.

@keyurdg keyurdg closed this Aug 9, 2016
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.

3 participants