Skip to content

Conversation

adambaratz
Copy link
Contributor

Full notes are in the bug report I created:
https://bugs.php.net/bug.php?id=71667

The efree() calls could be removed from dblib_stmt.c because free_statement(), which is part of the pdo extension, will do this cleanup before the functions in dblib_stmt.c get called.

I realize that README.GIT-RULES says PRs should be done against PHP-5.6, but that branch is missing changes that my changes are built on top of. I'm happy to update this PR if anyone has guidance there. I'm going to look at master separately and submit another bug/PR if needed.

@adambaratz adambaratz changed the title Bug #71667: have pdo_dblib emulate how mssql extension names "computed" columns, plug memory leak Bug #71667: have pdo_dblib emulate how mssql extension names "computed" columns, plug memory leak (PHP-5.6) Feb 26, 2016
@weltling
Copy link
Contributor

@adambaratz, thanks for the PR. Looks like i've accidentially broke mergeability base by pushing a parallel patch. I've checked with your patch and adjusted the leak fixing part.

The part with the changing the column names should be discussed. In general, such behavior change should not happen in the middle of a stable series. Please open another PR for this issue, if you think it is critical. I'm going to close this one for now.

Btw ext/pdo_dblb is currently missing active maintanance. It would be great to see someone to step in for at least the regular maintanance.

Thanks!

@php-pulls
Copy link

Comment on behalf of ab at php.net:

The leak fixing part is applied to 5.6+. Thanks for your work!

@php-pulls php-pulls closed this Feb 29, 2016
@MiRacLe-RPZ
Copy link
Contributor

@weltling, "computed" column was added in 5.6.16 (with my PR #1386), i think applying this patch safe for stable releases...

@adambaratz
Copy link
Contributor Author

Thanks for that note @MiRacLe-RPZ. I created a #1780 for discussion of making this change to 5.6. FYI, #1777 is still around for making this change to master.

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.

4 participants