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

Do not require ActiveStorage in core #5450

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Oct 24, 2023

Summary

The activestorage adapter is optional and stores might use another attachment adapter (ie. paperclip). We do not have it as dependency in our gemspec.

Since bundler already requires activestorage if the store decided to use activestorage adapter (either via a direct dependency or the transient rails gem) we do not need to require it in our engine.

This removes a LoadError for stores/engines that do not use activestorage.

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.

The activestorage adapter is optional and stores might
use another attachment adapter (ie. paperclip).

Since bundler already requires activestorage if the store
decided to use activestorage adapter (either via a direct
dependency or the transient rails gem) we do not need
to require it in our engine.

This removes a LoadError for stores/engines that do not
use activestorage.
@tvdeyen tvdeyen requested a review from a team as a code owner October 24, 2023 07:15
@tvdeyen tvdeyen added backport-v3.4 Backport this pull-request to v3.4 backport-v4.0 Backport this pull-request to v4.0 backport-v4.1 Backport this pull-request to v4.1 backport-v4.2 Backport this pull-request to v4.2 labels Oct 24, 2023
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #5450 (ea0254b) into main (b2a6f41) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5450      +/-   ##
==========================================
- Coverage   88.91%   88.91%   -0.01%     
==========================================
  Files         614      614              
  Lines       14827    14826       -1     
==========================================
- Hits        13183    13182       -1     
  Misses       1644     1644              
Files Coverage Ξ”
core/lib/spree/core.rb 98.78% <ΓΈ> (-0.02%) ⬇️

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

πŸ’š All backports created successfully

Status Branch Result
βœ… v3.4
βœ… v4.0
βœ… v4.1
βœ… v4.2

Questions ?

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

@tvdeyen tvdeyen deleted the do-not-require-active_storage branch October 25, 2023 09:25
@tvdeyen
Copy link
Member Author

tvdeyen commented Nov 1, 2023

This was wrong. Added a fix in #5479

@elia elia changed the title Do not require active_storage/engine Do not require ActiveStorage in core Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v3.4 Backport this pull-request to v3.4 backport-v4.0 Backport this pull-request to v4.0 backport-v4.1 Backport this pull-request to v4.1 backport-v4.2 Backport this pull-request to v4.2 changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants