If specify strict: :default explicitly, do not set sql_mode.#17654
If specify strict: :default explicitly, do not set sql_mode.#17654rafaelfranca merged 1 commit intorails:masterfrom
strict: :default explicitly, do not set sql_mode.#17654Conversation
There was a problem hiding this comment.
Perhaps check for if @config[:strict] ? What if someone passes strict: false
There was a problem hiding this comment.
Agreed, we should allow the user to pass in strict: false.
There was a problem hiding this comment.
Because the next line is variables['sql_mode'] = strict_mode? ? 'STRICT_ALL_TABLES' : '',
when strict: true it becomes variables['sql_mode'] = 'STRICT_ALL_TABLES',
when strict: false it becomes variables['sql_mode'] = ''.
|
One comment, this seems fine otherwise. |
|
IIUC, this changes the default behaviour so that we no longer default to strict mode. That seems not okay, and even if we do think it's a good idea, it would be 5.0 material. |
|
Indeed, this changes the default behaviour so that we no longer default to strict mode. However, strict_mode should be specified explicitly rather than implicit default. This is because, if MySQL settings are being carried out properly by the administrator, the application should not overwrite the sql_mode. In practice, I had that do not set the sql_mode by |
|
It does not change the default behavior by kamipo@a7df5af. How does if this? |
kamipo
force-pushed
the
strict_mode_explicitly
branch
from
a7df5af to
7961f4f
Compare
kamipo
changed the title
strict: :default explicitly, do not set sql_mode.
kamipo
force-pushed
the
strict_mode_explicitly
branch
2 times, most recently
from
3b3edb8 to
7961f4f
Compare
|
I squashed this PR. This PR is no longer change the default behavior. |
If specify `strict: :default` explicitly, do not set sql_mode.
|
I believe this needs a CHANGELOG entry, something like this would be very helpful to understand how it works! :) |
Add CHANGELOG entry for #17654
|
Should this be backported? |
|
Is there any workaround? If so it is better to not backport. |
7 participants
Related with #17370.