-
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
Avoid prefixing columns when false
is assigned to column-prefix
#1238
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3477 We use Jira to track the state of pull requests and the versions they got |
Looking good: 👍 @CentaurWarchief as usual, I have to ask for tests. EDIT: the fix is flawed for now. See inline comments |
@@ -833,6 +842,6 @@ protected function evaluateBoolean($element) | |||
{ | |||
$flag = (string)$element; | |||
|
|||
return ($flag === true || $flag == "true" || $flag == "1"); |
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.
Why did you remove the simple boolean true
case?
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.
Because (string) $element
never will be true. They're different types.
$ php -r 'var_dump((string) 'true' === true);' // bool(false)
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.
Good point
: null; | ||
|
||
$useColumnPrefix = isset($embeddedMapping['use-column-prefix']) | ||
? $this->evaluateBoolean( |
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.
No need for the line-wrap for the parameters here
…="true"` and `use-column-prefix="false"` xml mappings
…pped-embeddables' Close #1238
@CentaurWarchief merged, thanks! |
http://www.doctrine-project.org/jira/browse/DDC-3293
As the issue on JIRA wasn't updated since September/2014, here's a PR.
@Ocramius suggested on issue about introducing a new attribute called
use-column-prefix
, I'd say that keeping the same attribute as in others drivers would be better.That's it.