# Pull requests report (17/7/2013) - Thread: https://externals.io/message/68133 - Original archive: https://news-web.php.net/php.internals/68133 - Started by Lior Kaplan on 2013-07-17T22:47:02+00:00 - Messages: 17 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #68133 — Lior Kaplan — 2013-07-17T22:47:02+00:00 URL: https://externals.io/message/68133#68133 Merged requests (past 7 days): #364 fix invalid variable name at ext/spl/internal/multipleiterator.inc #370 Remove PWS (Personal Web Server) references #377 Add built-in web server to cli usage, and align terminology in cli manpage #378 Fix bug #62665: add curl.cainfo to php.ini #386 Typo fixes Thanks Stas for helping these merges happen. New (last 7 days): #384 HASH_KEY_NON_EXISTANT typo fix #385 mssql.compatability_mode typo fix #387 Wrong value for `FILTER_SANITIZE_FULL_SPECIAL_CHARS` existing (7-14 days): #375 Fixed bug #48770: when `call_user_func()` fails to call parent from inheriting class #381 Fixed #65225: `PHP_BINARY` incorrectly set #382 Added support for not canonicalizing the SASL realm on sasl_binds for LDAP What do you think about closing older PR ( > 28 days) ? Kaplan > Hi, > > Following my suggestion to help with pull requests, I'm going to send a > report about new & ongoing pull requests. > I hope it will help us to handle them more efficiently and lower the > response time for patches. > > Feedback is much appriciated. > > New (last 7 days): > #375 Fixed bug #48770: > when `call_user_func()` fails to call parent from inheriting class > #377 Add built-in web server to > cli usage, and align terminology in cli manpage > #378 Fix bug #62665: > add curl.cainfo to php.ini > #381 Fixed #65225: > `PHP_BINARY` incorrectly set > #382 Added support for not > canonicalizing the SASL realm on sasl_binds for LDAP > > Older (but seems trivial): > #360 Use in > `preg_replace_callback()` using variables by reference and test for bug # > 64979 > #364 fix invalid variable name > at ext/spl/internal/multipleiterator.inc > #370 Remove PWS (Personal Web > Server) references > > Kaplan > > p.s. > Feel free to just approve the merge after reviewing the code, and I'll do > merge myself later. --- ## #68134 — Stas Malyshev — 2013-07-17T23:46:41+00:00 URL: https://externals.io/message/68133#68134 · In reply to: #68133 Hi! > What do you think about closing older PR ( > 28 days) ? I think we shouldn't do this - some of them are for RFCs that are being worked on or discussed or proposed, so unless we know it's a stale patch that is abandoned or will never be merged we shouldn't close them. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #68135 — Levi Morrison — 2013-07-18T00:48:17+00:00 URL: https://externals.io/message/68133#68135 · In reply to: #68134 First of all, thank you Lior for sending this out. Second, thanks Stas for looking over these. > > […] > > I think we shouldn't do this - some of them are for RFCs that are being > worked on or discussed or proposed, so unless we know it's a stale patch > that is abandoned or will never be merged we shouldn't close them. > This is definitely a good reason not to close old Pull Requests. --- ## #68136 — Sherif Ramadan — 2013-07-18T01:21:05+00:00 URL: https://externals.io/message/68133#68136 · In reply to: #68133 > Merged requests (past 7 days): > #364 fix invalid variable name > at > ext/spl/internal/multipleiterator.inc > #370 Remove PWS (Personal Web > Server) references > #377 Add built-in web server to > cli usage, and align terminology in cli manpage > #378 Fix bug > #62665: > add curl.cainfo to php.ini > #386 Typo fixes > > Thanks Stas for helping these merges happen. > > New (last 7 days): > #384 HASH_KEY_NON_EXISTANT typo > fix > #385 mssql.compatability_mode > typo fix > #387 Wrong value for > `FILTER_SANITIZE_FULL_SPECIAL_CHARS` > > existing (7-14 days): > #375 Fixed bug > #48770: > when `call_user_func()` fails to call parent from inheriting class > #381 Fixed > #65225: > `PHP_BINARY` incorrectly set > #382 Added support for not > canonicalizing the SASL realm on sasl_binds for LDAP > > What do you think about closing older PR ( > 28 days) ? > > First off, thanks for all the hard work. PRs aren't getting as much attention as they should, and I'd like to say that I certainly haven't been helping as much as I should with them. What I am noticing though, is that a lot of these newer PRs, or the ones that are getting more attention, seem to be fairly cosmetic changes. I mean, spelling mistakes are all great fixes, and no disrespect to any contribution small or big, but there are certainly a lot of PRs which people have spent a great deal of time working on, clearly, that seem to be getting ignored. If we start closing these strictly because of how long they've been open it would be such a discouragement to everyone who contributed. I sincerely hope this doesn't happen. I'd much rather see people spending more time on reviewing some of the older PRs and seeing if they're worthy of merging or not based on substance rather than just dismiss them based on how long they've been open. I've tried to get in contact with some of the original authors of at least a couple of PRs in the past weeks to see if they were interested in working on an RFC to get their PRs merged as they necessitated some more discussion, but so far have come up empty. So I'll try to help out as much as I can with the ones that can get immediate attention. --- ## #68137 — Yasuo Ohgaki — 2013-07-18T03:21:18+00:00 URL: https://externals.io/message/68133#68137 · In reply to: #68136 Hi Sherif, 2013/7/18 Sherif Ramadan > First off, thanks for all the hard work. PRs aren't getting as much > attention as they should, and I'd like to say that I certainly haven't been > helping as much as I should with them. > We may have bugs ML and PR ML so that any discussion, then it may have more attentions. Bugs ML may have too many mails, but PR ML would have much less mails. It may be good idea that redirect PR discussion to internals, if it is possible. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68138 — Yasuo Ohgaki — 2013-07-18T03:27:24+00:00 URL: https://externals.io/message/68133#68138 · In reply to: #68137 Hi all, 2013/7/18 Yasuo Ohgaki > > 2013/7/18 Sherif Ramadan > > > […] > > We may have bugs ML and PR ML so that any discussion, then it may > have more attentions. > > Bugs ML may have too many mails, but PR ML would have much less mails. > It may be good idea that redirect PR discussion to internals, if it is > possible. > We already have Bugs ML http://www.php.net/mailing-lists.php I subscribed it now. For github, there is notification, but apparently not many people pay attention. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68140 — Sherif Ramadan — 2013-07-18T04:05:24+00:00 URL: https://externals.io/message/68133#68140 · In reply to: #68137 > Hi Sherif, > > 2013/7/18 Sherif Ramadan > > > […] > > We may have bugs ML and PR ML so that any discussion, then it may > have more attentions. > > Bugs ML may have too many mails, but PR ML would have much less mails. > It may be good idea that redirect PR discussion to internals, if it is > possible. > > > Well, some people may not be paying attention to the discussion on github about certain PRs and that's OK. Some people may not fully understand or have enough knowledge in one particular area to be able to help with that PR. Perfectly fine. However, I don't think that THIS is the reason PRs aren't getting their fair share of attention, honestly. For example, look at the oldest PR sitting around for over a year now https://github.com/php/php-src/pull/15 plenty of discussion there. Lots of criticism, suggestions, etc... All of that is wonderful. People are participating! Then all of a sudden the people involved in the discussion seem to mysteriously disappear and nothing happens to the PR. It just sits in limbo indefinitely. This is not the only PR I've seen follow this pattern. I assure you the discussion on github and the mailing lists are not the reason we've got stagnate PRs. --- ## #68148 — Lior Kaplan — 2013-07-18T07:54:21+00:00 URL: https://externals.io/message/68133#68148 · In reply to: #68136 On Thu, Jul 18, 2013 at 4:21 AM, Sherif Ramadan wrote: > > > > […] > > First off, thanks for all the hard work. PRs aren't getting as much > attention as they should, and I'd like to say that I certainly haven't been > helping as much as I should with them. What I am noticing though, is that a > lot of these newer PRs, or the ones that are getting more attention, seem > to be fairly cosmetic changes. I mean, spelling mistakes are all great > fixes, and no disrespect to any contribution small or big, but there are > certainly a lot of PRs which people have spent a great deal of time working > on, clearly, that seem to be getting ignored. If we start closing these > strictly because of how long they've been open it would be such a > discouragement to everyone who contributed. I sincerely hope this doesn't > happen. I'd much rather see people spending more time on reviewing some of > the older PRs and seeing if they're worthy of merging or not based on > substance rather than just dismiss them based on how long they've been open. > > I've tried to get in contact with some of the original authors of at least > a couple of PRs in the past weeks to see if they were interested in working > on an RFC to get their PRs merged as they necessitated some more > discussion, but so far have come up empty. So I'll try to help out as much > as I can with the ones that can get immediate attention. > I'm glad to see that question got everyone's attension (: Indeed we started with the easy ones... there isn't any reason simple patches won't get merged very quickly. We have some PR which the disscussion about them is stuck, the question is who and when do we cut it off and send the author to rework his patch. After some feedback was given, if the author isn't responsive, and no one else wants to handle the patch instead, we can reject and ask them to come back when ready. Also, we have more than a few PR which the patch is OK, but are stuck due to a missing test. This is fair enough, but should also have some rule of thumb for these cases. Seeing a long list of PR waiting for a year isn't much encouraging, I would prefer to see people quickly either have their changes accepted, sent to improve the PR or completly rejected. Don't forget the PR can always be resent and the work isn't lost. Kaplan --- ## #68149 — Sherif Ramadan — 2013-07-18T08:20:06+00:00 URL: https://externals.io/message/68133#68149 · In reply to: #68148 > On Thu, Jul 18, 2013 at 4:21 AM, Sherif Ramadan wrote: > > > […] > > I'm glad to see that question got everyone's attension (: > > Indeed we started with the easy ones... there isn't any reason simple > patches > won't get merged very quickly. > No disagreement about doing what can be done. > > We have some PR which the disscussion about them is stuck, the question is > who > and when do we cut it off and send the author to rework his patch. After > some > feedback was given, if the author isn't responsive, and no one else wants > to > handle the patch instead, we can reject and ask them to come back when > ready. > > As you can see from my example (of which are are numerous others), the author did everything that was asked of them. Yet, surprisingly, some people only seem to have something to add to the discussion when there is a problem (no harm there in pointing out problems with the patch), but are nowhere to be seen when there is nothing left to be critical of (definite bad on our part for not following up after the author did their part). > Also, we have more than a few PR which the patch is OK, but are stuck due > to a > missing test. This is fair enough, but should also have some rule of thumb > for > these cases. > True, a lot of people either don't know how to write tests or don't like to write tests for our test suite. They also might not know how to fix the test in some cases where changing existing tests in necessary. This isn't just limited to contributors not members of the PHP project, but even those that have been long time contributors have come to me with a request to either fix or write the test for them. I'll admit it took me a bit to get the hang of phpt's work flow. > > Seeing a long list of PR waiting for a year isn't much encouraging, I > would > prefer to see people quickly either have their changes accepted, sent to > improve > the PR or completly rejected. Don't forget the PR can always be resent and > the > work isn't lost. > Seeing PRs closed because no one cares to look at them is not ideal either. I agree that we should respond as quickly as possible, but sometimes changes being submitted require a bit more discussion on internals and people seem reluctant to take this step. I can't blame them, internals can be quite intimidating. I, myself, have experience the pain-staking work it takes to get my PRs merged into php-src, in the past. It took me nearly 6 months for some, and some sat stagnate for longer until I decided to close them myself since the people requesting the feedback never seemed to get back to me after feedback was provided. The process of getting PRs merged is definitely an on-going challenge due to limited man-power and factors of impeding communication. No doubt about that. I do not fault anyone in particular. I'd just like to see that everyone gets a fair shot and no one is excluded for the sake of exclusion or quickly drilling down the open PR list. --- ## #68156 — johannes@schlueters.de — 2013-07-18T10:33:59+00:00 URL: https://externals.io/message/68133#68156 · In reply to: #68140 > > I assure you the discussion on github and the mailing lists are not > the > reason we've got stagnate PRs. btw. the same happens with bugs. We're not good to track stuff which isn't in the "special interest area" of individual developers. We have way too few people going through "general" items. johannes --- ## #68164 — Stas Malyshev — 2013-07-18T19:00:08+00:00 URL: https://externals.io/message/68133#68164 · In reply to: #68148 Hi! > Indeed we started with the easy ones... there isn't any reason simple > patches > won't get merged very quickly. There's a very simple reason - there are not enough people with enough knowledge about PHP internals to properly evaluate patches that do it on regular basis. Even the simplest patch takes time, at least if you do it the right way - you have to read it, understand if it's right, pull it, build with it, run tests with it if it has tests, merge it to multiple branches, etc. - this takes time. Time is a scarce resource. > > We have some PR which the disscussion about them is stuck, the question is > who > and when do we cut it off and send the author to rework his patch. After Why we need to "cut off" anything? I mean, if the case is obvious - like the author does not answer the feedback for many months - we could close it - but if the patch is being worked on, why should we remove it from public view? Some things go fast, some things take time. I think we need to go case by case. > Also, we have more than a few PR which the patch is OK, but are stuck due > to a > missing test. This is fair enough, but should also have some rule of thumb > for > these cases. Again, it would not be hard to add tests for many of them, the problem is again - time. For myself, I'd rather spend time on merging properly done patches or fixing bugs than finishing up patches that the original author could finish up. But if some people decide that this is a good way for them to contribute - that'd be great. Otherwise, removing these pulls from public view would only mean they'd never get taken care at all. > Seeing a long list of PR waiting for a year isn't much encouraging, I would > prefer to see people quickly either have their changes accepted, sent to > improve > the PR or completly rejected. Don't forget the PR can always be resent and > the > work isn't lost. The work isn't lost but once PR is closed the chance anybody would look at it again except original author is very low. While if it's open, it still could be a chance somebody would pick it up. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #68168 — Lior Kaplan — 2013-07-18T19:57:57+00:00 URL: https://externals.io/message/68133#68168 · In reply to: #68164 On Thu, Jul 18, 2013 at 10:00 PM, Stas Malyshev wrote: > > […] > thumb > > […] > > Again, it would not be hard to add tests for many of them, the problem > is again - time. For myself, I'd rather spend time on merging properly > done patches or fixing bugs than finishing up patches that the original > author could finish up. But if some people decide that this is a good > way for them to contribute - that'd be great. Otherwise, removing these > pulls from public view would only mean they'd never get taken care at all. Maybe offer the list of needed tests as an easy task/hack so even people not into the internals could contribute. e.g. https://wiki.documentfoundation.org/Easy_Hacks Might be a win win for everybody ? (although some help might still be needed) Kaplan --- ## #68169 — Yasuo Ohgaki — 2013-07-18T20:44:03+00:00 URL: https://externals.io/message/68133#68169 · In reply to: #68164 Hi Stas 2013/7/19 Stas Malyshev > > […] > > There's a very simple reason - there are not enough people with enough > knowledge about PHP internals to properly evaluate patches that do it on > regular basis. Even the simplest patch takes time, at least if you do it > the right way - you have to read it, understand if it's right, pull it, > build with it, run tests with it if it has tests, merge it to multiple > branches, etc. - this takes time. Time is a scarce resource. I completely agree. Why don't we state this fact in bugs.php.net and github that bug/PR will take time to be addresses? It's better to give contributors expectation how long it may take time to be addresses? It's not a solution, but it's better than nothing. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68170 — Levi Morrison — 2013-07-18T21:08:51+00:00 URL: https://externals.io/message/68133#68170 · In reply to: #68169 > Why don't we state this fact in bugs.php.net and github that bug/PR will > take time to be addresses? > > It's better to give contributors expectation how long it may take time to > be addresses? > I personally would recommend against it. They'd be less likely to submit a PR if they have a warning up front that their PR will sit there for a long time. Just my input. --- ## #68171 — Yasuo Ohgaki — 2013-07-18T22:04:26+00:00 URL: https://externals.io/message/68133#68171 · In reply to: #68170 Hi Levi, 2013/7/19 Levi Morrison > Why don't we state this fact in bugs.php.net and github that bug/PR will > > […] > > I personally would recommend against it. They'd be less likely to submit a > PR if they have a warning up front that their PR will sit there for a long > time. > > Just my input. > Expectation control is basic management task. When you order something, you would like to know how long it will take to be delivered. Don't you upset if there aren't any notice for long delivery time? It's better treat contributors nicely, IMHO. If they have better experience, they may contribute again. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68217 — Jakub Zelenka — 2013-07-19T19:32:41+00:00 URL: https://externals.io/message/68133#68217 · In reply to: #68171 Hi, please can you close the pull request https://github.com/php/php-src/pull/317 . It's my old request for array_column bug that has been already fixed. Cheers. Jakub > Hi Levi, > > 2013/7/19 Levi Morrison > > > […] > to > > […] > a > > […] > long > > […] > > Expectation control is basic management task. > When you order something, you would like to know how long it > will take to be delivered. Don't you upset if there aren't any notice > for long delivery time? > > It's better treat contributors nicely, IMHO. > If they have better experience, they may contribute again. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #68221 — Yasuo Ohgaki — 2013-07-19T20:20:27+00:00 URL: https://externals.io/message/68133#68221 · In reply to: #68217 Hi Jakub, 2013/7/20 Jakub Zelenka > Hi, please can you close the pull request > https://github.com/php/php-src/pull/317 . It's my old request for > array_column bug that has been already fixed. Cheers. > Closed. Thank you. -- Yasuo Ohgaki yohgaki@ohgaki.net