savonrb / savon-multipart

Heavy metal Ruby SOAP client with multipart support
MIT License
17 stars 48 forks source link

Rails 6 support? #27

Closed madzhuga closed 2 years ago

madzhuga commented 2 years ago

I am updating Rails 5 app to Rails 6 and facing gems versions conflicts:

Bundler could not find compatible versions for gem "mail":
  In snapshot (Gemfile.lock):
    mail (= 2.7.1)

  In Gemfile:
    rails (= 6.1.4) was resolved to 6.1.4, which depends on
      actionmailbox (= 6.1.4) was resolved to 6.1.4, which depends on
        mail (>= 2.7.1)

    savon-multipart (= 2.1.1) was resolved to 2.1.1, which depends on
      mail (~> 2.5.4)

Any advice? This gem has no updates for 2 years. Does savon still need it or maybe it support multipart messages without this gem now?

olleolleolle commented 2 years ago

Could you try making a PR with the changes?

madzhuga commented 2 years ago

@olleolleolle I did no changes to savon or savon-multipart. I just updated main app from Rails 5 to Rails 6.

madzhuga commented 2 years ago

it appears savon-multipart build on https://rubygems.org/gems/savon-multipart has dependency to main 2.5.4 while here in master branch hardcoded version is removed.

madzhuga commented 2 years ago

I had to patch Savon::Multipart::Response was:

      def parse_xop
...
        xop_elements.each do |xop_element|
          href = xop_element.attributes['href'].to_s
          cid = href[4..-1]
          data = @parts.find { |p| p.header['content-id'].to_s == "<#{cid}>" }.body.to_s
...
      end

The cid here is CGI encoded, so it has %40 instead of @ etc. I had to patch it to be

part = @parts.find { |p| p.header['content-id'].to_s == "<#{cid}>" || p.header['content-id'].to_s == "<#{CGI.unescape(cid)}>" }

I know that this fix is ugly but maybe repo owners will prepare better fix knowing all implementation details.

olleolleolle commented 2 years ago

@madzhuga Hi! Does the PR #21 help with the issue that you are having?

madzhuga commented 2 years ago

@olleolleolle yep, it looks the same fix. Strange it is known issue for 5 years and it's still not merged.

olleolleolle commented 2 years ago

@madzhuga Cool, then I have corroboration that it's a good fix. Appreciate the test!

olleolleolle commented 2 years ago

@madzhuga A new version has now been released, please do test-drive it, and see if it works. A mishap made it so that the release happened to include a feature addition in it.

pcai commented 2 years ago

Closing this since #21 has been merged and v2.1.2 should allow rails 6