Skip to content
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

Fix #7286: StringPrimary no longer accepts aggregate functions as argument #7296

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Jul 4, 2018

866418e (#6500) removed special-casing of aggregate functions.
This was mostly transparent for cases where isAggregateFunction() is used, but not in StringPrimary which doesn't use isAggregateFunction() to detect the function ahead.
Since these functions (still) have separate tokens, they're not recognized as T_IDENTIFIER in StringPrimary, thus not handled as generic FunctionDeclaration.

Fixes #7286.

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

@Majkl578 this seems to be a clean patch but I'm wondering if everything will still work if we use an aggregation function (or any function) as argument to a custom function - which was what removed the code before 🤔

@lcobucci
Copy link
Member

lcobucci commented Jul 5, 2018

@Ocramius any thoughts here?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Jul 5, 2018

@lcobucci Added GH7286Test::testAggregateFunctionInCustomFunction() test, is that what you meant?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Jul 5, 2018

Bah, fails on Postgres, would have to test with pre-2.6 to see what worked or not.

@lcobucci
Copy link
Member

lcobucci commented Jul 8, 2018

@Majkl578 that's exactly what I meant, thanks for adding the test 👍

@lcobucci lcobucci self-assigned this Jul 9, 2018
@lcobucci lcobucci merged commit 36e6a73 into doctrine:2.6 Jul 9, 2018
@lcobucci
Copy link
Member

lcobucci commented Jul 9, 2018

@Majkl578 can you please port it to 3.0-dev?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants