-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Allow CTE on more CTE safe functions #10771
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually “Request changes” to make the concerns more visible to other reviewers: #10771 (comment)
This reverts commit 8e31c3e.
This reverts commit 069fa1a.
This was accidentally added in phpGH-7780, but since it takes a callable argument, this flag is useless on this function. Closes phpGH-10859.
This reverts commit c6c4f21.
Okay I went through the whole list in detail now. Two functions need to be removed:
I also see you added an XFAIL in one of the tests, I think it's better to not XFAIL the test. Instead, just disable opcache on that test. That way, we can still catch bugs. Other than the remarks above I think this PR looks good (but someone else should double check). |
There are test failures because you forgot the PHP tags in the SKIPIF section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now. I'll wait a few days to see if more feedback comes in, but if not I'll do do the merge.
As a side note, I think that the better solution for the observers is to simply disable SCCP optimisation for those tests and revert the changes done to them, but I will do that during the merge.
I agree. |
Merged manually, thanks. |
No description provided.