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

Make all belongs_to associations optional #3309

Merged
merged 2 commits into from
Aug 22, 2019

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Aug 22, 2019

Second try of #3260

Since Rails 5.1 belongs_to associations are required by default. We used to disable this option globally with a ActiveRecord class setting. With this we make the Rails 5.1 default setting obsolote and turn this off app wide.

Rails.application.config.active_record.belongs_to_required_by_default # => true
Spree::Base.belongs_to_required_by_default # => false
ActiveRecord::Base.belongs_to_required_by_default # => false

The fun fact is, the tests all pass with this removed. But this has the potential to break stores if their data is not consistent.

We have several options to solve this

  1. We tell our users to clean up the database before running the next Solidus update where this included.
  2. We include this in a mayor version update
  3. We explicitely make all our belongs_to associations optional

I am in favor of the latter, so I made all belongs_to associations optional for now.

Description

Checklist:

With this ActiveRecord class setting we make the Rails 5.1 default
setting obsolote and turn this off app wide.

   Rails.application.config.active_record.belongs_to_required_by_default
   # => true
   Spree::Base.belongs_to_required_by_default # => false
   ActiveRecord::Base.belongs_to_required_by_default # => false

The fun fact is, the tests all pass with this removed.
But this has the potential to break stores if their data is
not consistent.

We have several options to solve this

1. We tell our users to clean up the database before running the next
Solidus update where this included.
2. We include this in a mayor version update
3. We explicitely make all our `belongs_to` associations `optional`

I am in favor of the latter
Since Rails 5.1 `belongs_to` associations are required by default.
We used to disable this option globally. This hack was removed recently.
Since this has the potential to break stores with unconsistent data
we need to make all `belongs_to` optional for now.
@tvdeyen tvdeyen force-pushed the make-belongs-to-optional branch from 6fc24c0 to 56cb3ef Compare August 22, 2019 07:02
@tvdeyen tvdeyen merged commit 82fb316 into solidusio:master Aug 22, 2019
@tvdeyen tvdeyen deleted the make-belongs-to-optional branch August 22, 2019 16:26
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request May 26, 2022
It makes no sense to have dangling option_values without an associated
option_type.

Ref solidusio#3309 & solidusio#3517
cpfergus1 pushed a commit to cpfergus1/solidus that referenced this pull request Aug 25, 2022
It makes no sense to have dangling option_values without an associated
option_type.

Ref solidusio#3309 & solidusio#3517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants