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

Upgrade carrierwave to version 2.1 #7213

Merged
merged 47 commits into from Jan 29, 2021

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Jan 24, 2021

🎩 What? Why?

Upgrade the carrierwave version from 1.3.1 to 2.1 in order to fix the deprecation warnings regarding Ruby 2.7 upgrade

carrierwave-1.3.1/lib/carrierwave/sanitized_file.rb:316: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
carrierwave-1.3.1/lib/carrierwave/mounter.rb:113: warning: deprecated Object#=~ is called on TrueClass; it always returns nil

📌 Related Issues

Link your PR to an issue

  • Related to #?
  • Fixes #?

Testing

This PR does not add any functionality to main Decidim Application. Is targeting Carrierwave upgrade to reduce the number of deporecations generated by Ruby 2.7 upgrade.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

Carrierwave change log

2.1.0 - 2020-02-16

Added
Fixed
  • Fix Ruby 2.7 deprecations(@mshibuya 9a37fc9e)
  • Fix S3 path-style URL for host with dots for buckets that are placed in other regions than us-east-1(@Bonias #2439)
  • Make MiniMagick::Image constant absolute to prevent misleading 'uninitialized constant' error(@p8 #2437)

2.0.2 - 2019-09-28

Fixed

2.0.1 - 2019-08-31

Fixed

2.0.0 - 2019-08-18

No changes.

2.0.0.rc - 2019-06-23

Added
  • Append, reorder, and remove-single-file feature for multiple file uploader(@mshibuya #2401)
  • Allow retrieval of uploader index within uploaders(@mshibuya #1771)
  • Add ability to customize downloaders(@mshibuya #1636)
  • Support internationalized domain names for downloader(@mshibuya #2086)
  • Support authenticated_url for Aliyun provider(@Nitrino #2381)
  • Support passing options to authenticated_url for OpenStack provider(@stanhu #2377)
  • Support authenticated_url for AzureRM provider(@Nitrino #2375)
  • Allow custom expires_at when building an authenticated_url(@stephankaag #2397)
Changed
  • [BREAKING CHANGE] Use the storage given by storage configuration also for cache_storage unless explicitly specified(@mshibuya 629afecb)
  • Improve Fog initialization(@mshibuya #2395)
  • [BREAKING CHANGE] Multiple file uploader now keeps successful files on update, only discarding failed ones(@mshibuya 7db9195d)
  • [BREAKING CHANGE] #remote_#{column}_urls= was changed to preserve precedent updates(@mshibuya 8f18a95b)
  • #serializable_hash now returns string for version keys(@schovi #2246)
  • Use the MimeMagic gem to inspect file headers for the mime type. This allows for mitigation of CVE-2016-3714, in combination with a content_type_whitelist(@locriani #1934)
  • Replace mime-types dependency with mini_mime to save memory(@bradleypriest #2292)
  • Delegate MiniMagick processing to ImageProcessing gem(@janko #2298)
  • Handle ActiveRecord transaction correctly, not storing or removing files on rollback(@skosh #2209)
Deprecated
  • fog_provider configuration was deprecated and has no effect, just adding fog providers to Gemfile will load them(@mshibuya ca201ee2)
  • CarrierWave::Uploader::Base#sanitized_file was deprecated, use #file instead(@mshibuya 28190e99)
Removed
Fixed

♥️ Thank you!

Bump develop to next release version
@alecslupu alecslupu reopened this Jan 24, 2021
@alecslupu alecslupu mentioned this pull request Jan 24, 2021
12 tasks
@alecslupu alecslupu changed the title Feature/upgrade carierwave2.1 Upgrade carierwave to version 2.1 Jan 24, 2021
@alecslupu alecslupu marked this pull request as ready for review January 24, 2021 19:52
@@ -50,6 +58,7 @@ def attributes
.merge(highlighted_content_banner_attributes)
.merge(omnipresent_banner_attributes)
.merge(colors_attributes)
.delete_if { |_k, val| val.is_a?(Decidim::ApplicationUploader) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in the class initializer we assign the object logo to the form. We assign it, as if we do not perform that step, the images in the form will be lost when updating other fields.
In this particular case we delete the file to prevent multiple processing of the same file. This is a workaround that i have found in decidim codebase, added via: #7026

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks!

decidim-core/app/uploaders/decidim/application_uploader.rb Outdated Show resolved Hide resolved
decidim-core/decidim-core.gemspec Outdated Show resolved Hide resolved
@mrcasals
Copy link
Contributor

Also, @alecslupu I see some code that seems to be unrelated to the upgrade to carrierwave 2, but related to #7189 and #7191. Could you split that into different PRs, please?

Thanks!

@mrcasals mrcasals added this to Pending review from Product in Maintainers via automation Jan 25, 2021
@mrcasals mrcasals moved this from Pending review from Product to In Review / Blocked / Waiting for feedback in Maintainers Jan 25, 2021
@alecslupu
Copy link
Contributor Author

Also, @alecslupu I see some code that seems to be unrelated to the upgrade to carrierwave 2, but related to #7189 and #7191. Could you split that into different PRs, please?

Thanks!

Hello @mrcasals,

Indeed, i see there are few changes related to #7211.

@mrcasals mrcasals changed the title Upgrade carierwave to version 2.1 Upgrade carrierwave to version 2.1 Jan 25, 2021
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Hi @alecslupu! There's still some code unrelated to the carrierwave upgrade, can you move it to another PR please?

@alecslupu alecslupu force-pushed the feature/upgrade-carierwave2.1 branch from 4b759c0 to e57d7e0 Compare January 26, 2021 06:44
@alecslupu alecslupu force-pushed the feature/upgrade-carierwave2.1 branch from e57d7e0 to 3e1b6a7 Compare January 26, 2021 06:49
@alecslupu alecslupu force-pushed the feature/upgrade-carierwave2.1 branch from 35b19a3 to ff2d91a Compare January 26, 2021 08:16
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks!

@mrcasals mrcasals merged commit 0b6ab57 into decidim:develop Jan 29, 2021
Maintainers automation moved this from In Review / Blocked / Waiting for feedback to Done Jan 29, 2021
andreslucena pushed a commit that referenced this pull request Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants