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

Support Ruby 3+ officially, remove final Rails 6.1 deprecations, drop test dependency for XML serialization #5347

Merged
merged 5 commits into from
Feb 16, 2021

Conversation

carlosantoniodasilva
Copy link
Member

Adding Ruby 3 to the matrix caused some issues with Rails 6.0 to show up, which required us to point to the 6-0-stable branch for now. We'll switch back to a stable release version once it gets out there.

And in order to remove Rails 6.1 deprecation with ActiveMode::Errors#to_xml, I went ahead and changes the tests that relied on XML serialization to use JSON instead, so we would no longer have that deprecation and could remove the dependency on the extracted activemodel-serializers-xml, simplifying things. Note that this doesn't mean XML isn't supported by Devise, it simply means we're using JSON to test non-navigatable (HTML & such) flows.

And remove dupe entry in the exclude matrix.

In order to get Ruby 3 working we needed to install `rexml` as part of
the test dependencies, only done on the main Gemfile (Rails 6.1) and the
6.0 versions. (which are the only ones supported by Ruby 3.)

Devise itself doesn't require `rexml` as it does nothing with it, but a
dependency we use during tests seem to require it. I was able to track
it down to omniauth-openid -> rack-openid -> ruby-openid requiring it:

    https://github.com/openid/ruby-openid/blob/13a88ad6442133a613d2b7d6601991a84b34630d/lib/openid/yadis/xrds.rb#L1

So while we have tests using omniauth-openid, we'll need this require in
place as well. Ideally that upstream version of ruby-openid should have
it, but it seems that one isn't updated in a while.
This allows us to remove the dependency on the XML serializer provided
by the external `activemodel-serializers-xml` gem, and eliminates the
following deprecation warning:

    DEPRECATION WARNING: ActiveModel::Errors#to_xml is deprecated and
    will be removed in Rails 6.2.

Please note: this does not mean Devise doesn't support XML, it simply
means our test suite will use JSON to test non-navigatable formats
instead of XML, for simplicity. Devise's job is not to test object
serialization, so as long as your objects properly serialize to
XML/JSON/any other format, it should work out of the box.
The test suite was failing on Rails 6.0 + Ruby 3 with errors like:

    Expected "{\"errors\":\"#<ActiveModel::Errors:0x000055f2e6cb8188>\"}"
    to include "{\"errors\":{".

The ActiveModel::Errors object wasn't being serialized to JSON as
expected, and this only happened with that combination of Ruby/Rails.

Upon further investigation, this was caused by a change in Ruby and
fixed in Rails in this PR: rails/rails#39697
(which describes in more details the exact same problem and links to the
Ruby bug tracker with more information).

That fix was backported to 6-0-stable in June 2020, but hasn't been
officially released in a stable version yet: (there have been only
security fixes since then for 6.0)
rails/rails@75f6539

Since the branch contains the fix, I'm pointing directly to it to get
the tests passing. We can't tell if there'll be a new stable 6.0 release
at this point, but hopefully yes, in which case we can go back at
pointing to it.
2.2.10 is causing the dependency resolution on Rails 6-0-stable to fail:

```
  Bundler could not find compatible versions for gem "railties":
    In Gemfile-rails-6-0:
      devise was resolved to 4.7.3, which depends on
        railties (>= 4.1.0)

      rails was resolved to 6.0.3.5, which depends on
        railties (= 6.0.3.5)

      responders (~> 3.0) was resolved to 3.0.1, which depends on
        railties (>= 5.0)
  Took  27.49 seconds
```

https://github.com/heartcombo/devise/runs/1905780158?check_suite_focus=true#step:5:23

The `railties` version 6.0.3.5 should work, given the other two are
using >= declarations, but it fails in 2.2.10.

Downgrading to 2.2.9 works.
@carlosantoniodasilva carlosantoniodasilva self-assigned this Feb 16, 2021
@carlosantoniodasilva carlosantoniodasilva merged commit 0cd72a5 into master Feb 16, 2021
@carlosantoniodasilva carlosantoniodasilva deleted the ca-build branch February 16, 2021 20:35
@waissbluth
Copy link

thanks @carlosantoniodasilva. is there anything else preventing the rails 6.1 support to make it into a release?

@carlosantoniodasilva
Copy link
Member Author

@waissbluth kinda, I was hoping to prepare a few other things since the OmniAuth change that is on master may be considered a bit of a breaking change, but I might go ahead and release what's in there as 4.8 instead. (I'm not ready for a major bump.) Can't give you an ETA for now though. Thanks!

@Dreamersoul
Copy link

@carlosantoniodasilva Thanks for the hard work on devise I really appreciate it! I believe pushing the current update as 4.8 to make rails 6.1.3 work and then making breaking changes to omniauth a 5.0 release, this will make the upgrade path easier for people who are looking to push their 6.1.3 rails app to production! Thanks again

@carlosantoniodasilva
Copy link
Member Author

@Dreamersoul appreciate the feedback.

I don't think any changes are required to run with Rails 6.1 right now, which means the current stable version should work. This just helped ensure our test suite was green with that version of Rails.

That being said, the changes for omniauth aren't as breaking to require a 5.0 release (and all the work involved in reverting those from master to exclude from a next release, etc.), I just wanted to wrap up a few other things to be able to release something like a 4.8 including those changes. Anyway, I'm hoping to cut one soon including this (master) even if I don't get to include everything I wanted.

@carlosantoniodasilva
Copy link
Member Author

I just pushed v4.8 including this change, please give it a try and report back if you have any issues. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants