-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improve exception message for DQL single-valued association path expression to an inverse side (unsupported) #6325
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.
Even though it's a very tiny BC break it LGTM. @Ocramius should we document this in the v2.6 changelog?
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.
LGTM besides nitpicking. As @lcobucci mentioned, we need an entry in UPGRADE.md
for this, since it still counts as BC Break.
* @return QueryException | ||
*/ | ||
public static function associationPathInverseSideNotSupported() | ||
public static function associationPathInverseSideNotSupported($pathExpr) |
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 know the caller side uses abbreviations, but please don't abbreviate here.
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 was following this example. Are you fine with the inconsistency?
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.
If you can add it to the patch, could also rename the variable there please? 👍
@@ -204,13 +204,15 @@ public static function overwritingJoinConditionsNotYetSupported($assoc) | |||
} | |||
|
|||
/** | |||
* @param object $pathExpr |
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.
Is there really nothing more detailed than object
? :| Sounds like the SqlWalker
internal API could need a DTO
And yep, a test for the static method is needed :-\ |
…meter to the exception named constructor
Hello,
I have added path expression to the exception about single-valued association to an inverse side being not supported. The path expression makes it easier to identify the issue among many other expression in potentially complex query.
Seems like many people had an issue with this exception.
For a query:
The exception message before:
The exception message after: