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

Fix preferences serialization compatibility with Rails version check #6083

Merged

Conversation

swamp09
Copy link
Contributor

@swamp09 swamp09 commented Jan 20, 2025

This PR modifies the Persistable module in Solidus to check the Rails version using Rails.gem_version instead of inspecting the serialize method parameters. This change is necessary only when using Solidus Globalize, ensuring compatibility with Rails 7.2.

Background

Solidus itself does not have any issues with the current implementation:

if method(:serialize).parameters.include?([:key, :type]) # Rails 7.1+
  serialize :preferences, type: Hash, coder: YAML
else
  serialize :preferences, Hash, coder: YAML
end

However, when using Solidus Globalize, which relies on globalize, an issue arises in Rails 7.2 due to globalize overriding the serialize method:

ArgumentError: wrong number of arguments (given 2, expected 1)

This occurs because the serialize method signature is modified by globalize, causing a mismatch when Solidus inspects its parameters. For reference, see the globalize implementation here: https://github.com/globalize/globalize/blob/main/lib/patches/active_record/rails7_2/serialization.rb

Solution

To avoid conflicts, this PR replaces the method introspection with a Rails version check:

if Rails.gem_version >= Gem::Version.new('7.1')
  serialize :preferences, type: Hash, coder: YAML
else
  serialize :preferences, Hash, coder: YAML
end

This approach ensures compatibility while keeping Solidus independent from dependency-specific overrides.

This issue is currently blocking the CI of the following PR in the solidus_globalize repository: solidusio-contrib/solidus_globalize#153 Fixing this in Solidus will help unblock that PR.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

This PR modifies the Persistable module in Solidus to check the Rails version using Rails.gem_version instead of inspecting the serialize method parameters. This change is necessary only when using Solidus Globalize, ensuring compatibility with Rails 7.2.

## Background
Solidus itself does not have any issues with the current implementation:

```
if method(:serialize).parameters.include?([:key, :type]) # Rails 7.1+
  serialize :preferences, type: Hash, coder: YAML
else
  serialize :preferences, Hash, coder: YAML
end
```

However, when using Solidus Globalize, which relies on globalize, an issue arises in Rails 7.2 due to globalize overriding the serialize method:

```
ArgumentError: wrong number of arguments (given 2, expected 1)
```

This occurs because the serialize method signature is modified by globalize, causing a mismatch when Solidus inspects its parameters.
For reference, see the globalize implementation here:
https://github.com/globalize/globalize/blob/main/lib/patches/active_record/rails7_2/serialization.rb

## Solution
To avoid conflicts, this PR replaces the method introspection with a Rails version check:
```
if Rails.gem_version >= Gem::Version.new('7.1')
  serialize :preferences, type: Hash, coder: YAML
else
  serialize :preferences, Hash, coder: YAML
end
```
This approach ensures compatibility while keeping Solidus independent from dependency-specific overrides.

This issue is currently blocking the CI of the following PR in the solidus_globalize repository:
solidusio-contrib/solidus_globalize#153
Fixing this in Solidus will help unblock that PR.
@swamp09 swamp09 requested a review from a team as a code owner January 20, 2025 10:39
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Jan 20, 2025
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@tvdeyen tvdeyen added the backport-v4.4 Backport this pull-request to v4.4 label Jan 20, 2025
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.58%. Comparing base (3675af2) to head (2bcf076).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6083      +/-   ##
==========================================
+ Coverage   89.33%   89.58%   +0.24%     
==========================================
  Files         730      801      +71     
  Lines       16462    18275    +1813     
==========================================
+ Hits        14706    16371    +1665     
- Misses       1756     1904     +148     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kennyadsl kennyadsl merged commit 06a1148 into solidusio:main Jan 21, 2025
16 checks passed
Copy link

💚 All backports created successfully

Status Branch Result
v4.4

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@swamp09 swamp09 deleted the fix_preferences_seralize_rails_version_check branch January 21, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v4.4 Backport this pull-request to v4.4 changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants