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

If specify `strict: :default` explicitly, do not set sql_mode. #17654

Merged
merged 1 commit into from May 28, 2015

Conversation

@kamipo
Copy link
Member

commented Nov 17, 2014

Related with #17370.

@@ -810,7 +810,7 @@ def configure_connection
# Make MySQL reject illegal values rather than truncating or blanking them, see
# http://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html#sqlmode_strict_all_tables
# If the user has provided another value for sql_mode, don't replace it.
unless variables.has_key?('sql_mode')
if @config.has_key?(:strict) && !variables.has_key?('sql_mode')

This comment has been minimized.

Copy link
@egilburg

egilburg Nov 18, 2014

Contributor

Perhaps check for if @config[:strict] ? What if someone passes strict: false

This comment has been minimized.

Copy link
@sgrif

sgrif Nov 18, 2014

Contributor

Agreed, we should allow the user to pass in strict: false.

This comment has been minimized.

Copy link
@kamipo

kamipo Nov 18, 2014

Author Member

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'] = ''.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2014

One comment, this seems fine otherwise.

@matthewd

This comment has been minimized.

Copy link
Member

commented Nov 18, 2014

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.

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2014

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 strict: false, but no longer can do it by #16065.

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2014

It does not change the default behavior by kamipo@a7df5af. How does if this?

@kamipo kamipo force-pushed the kamipo:strict_mode_explicitly branch from a7df5af to 7961f4f May 27, 2015

@kamipo kamipo changed the title If do not specify strict_mode explicitly, do not set sql_mode. If specify `strict: :default` explicitly, do not set sql_mode. May 27, 2015

@kamipo kamipo force-pushed the kamipo:strict_mode_explicitly branch 2 times, most recently from 3b3edb8 to 7961f4f May 27, 2015

@kamipo

This comment has been minimized.

Copy link
Member Author

commented May 27, 2015

I squashed this PR. This PR is no longer change the default behavior.

rafaelfranca added a commit that referenced this pull request May 28, 2015
Merge pull request #17654 from kamipo/strict_mode_explicitly
If specify `strict: :default` explicitly, do not set sql_mode.

@rafaelfranca rafaelfranca merged commit c68e45d into rails:master May 28, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:strict_mode_explicitly branch May 28, 2015

@chancancode

This comment has been minimized.

Copy link
Member

commented May 30, 2015

I believe this needs a CHANGELOG entry, something like this would be very helpful to understand how it works! :)

kamipo added a commit to kamipo/rails that referenced this pull request May 30, 2015
kamipo added a commit to kamipo/rails that referenced this pull request May 30, 2015
kamipo added a commit to kamipo/rails that referenced this pull request May 30, 2015
eileencodes added a commit that referenced this pull request May 30, 2015
@zzak

This comment has been minimized.

Copy link
Member

commented Jun 1, 2015

Should this be backported?

/cc @rafaelfranca @matthewd

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Jun 1, 2015

Is there any workaround? If so it is better to not backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.