-
Notifications
You must be signed in to change notification settings - Fork 21
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
Rails 7.2 support #31
base: main
Are you sure you want to change the base?
Conversation
@@ -39,7 +39,7 @@ def create_table_definition(*args, **options) | |||
end | |||
|
|||
# override | |||
def new_column_from_field(table_name, field) | |||
def new_column_from_field(table_name, field, _definitions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required by this change to ActiveRecord: https://github.com/rails/rails/pull/48241/files#diff-d1bec6fcf0cf6444eb5c2227f551c124419c572f1615aacf4a9894da4412f5a4R178
@@ -22,6 +22,10 @@ def new_column_definition(name, type, **options) | |||
|
|||
column | |||
end | |||
|
|||
def valid_column_definition_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required by this change to ActiveRecord: https://github.com/rails/rails/pull/46178/files#diff-fac852e063eee718c2af44dec6f80b1b46f52f8aae7936e5eea6b2eab08cf2f7R590
@@ -66,7 +66,7 @@ class Mysql2RgeoAdapter < Mysql2Adapter | |||
# http://postgis.17.x6.nabble.com/Default-SRID-td5001115.html | |||
DEFAULT_SRID = 0 | |||
|
|||
def initialize(connection, logger, connection_options, config) | |||
def initialize(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required by this change to ActiveRecord: https://github.com/rails/rails/pull/44591/files#diff-538a9bf3450c3aed556a0d2eacc26839a60becd744cda8c0a247dcb06aa0f02fR44
@@ -137,6 +137,10 @@ def quote(value) | |||
end | |||
end | |||
|
|||
def with_connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required by this change to ActiveRecord: https://github.com/rails/rails/pull/51162/files#diff-22750093c2231cef8c58e7e94a0a9266267d33bd990c607475e55fe285fccb8aR45
I only saw this with_connection
method getting called by tests in this gem that were dumping the schema. I'm sure something more sophisticated could be created for this, however it fixed the failing tests.
@@ -1,3 +1,5 @@ | |||
# frozen_string_literal: true | |||
|
|||
require "active_record/connection_adapters/mysql2rgeo_adapter" | |||
|
|||
ActiveRecord::ConnectionAdapters.register('mysql2rgeo', 'ActiveRecord::ConnectionAdapters::Mysql2RgeoAdapter', 'active_record/connection_adapters/mysql2rgeo_adapter') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes the following error when running rake test
DEPRECATION WARNING: Database configuration specifies 'mysql2rgeo' adapter but that adapter has not been registered. Rails 7.2 has changed the way Active Record database adapters are loaded. The adapter needs to be updated to register itself rather than being loaded by convention. Ensure that the adapter in the Gemfile is at the latest version. If it is, then the adapter may need to be modified. See: https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters.html#method-c-register (called from establish_test_connection at /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:22)
/Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_adapters.rb:85:in `resolve': Database configuration specifies 'mysql2rgeo' adapter but that adapter has not been registered. Ensure that the adapter in the Gemfile is at the latest version. If it is, then the adapter may need to be modified. (ActiveRecord::AdapterNotFound)
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/database_configurations/database_config.rb:18:in `adapter_class'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/database_configurations/database_config.rb:30:in `validate!'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_adapters/abstract/connection_handler.rb:268:in `resolve_pool_config'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_adapters/abstract/connection_handler.rb:116:in `establish_connection'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_handling.rb:53:in `establish_connection'
from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:22:in `establish_test_connection'
from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:28:in `<class:SpatialModel>'
from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:27:in `<top (required)>'
from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/basic_test.rb:3:in `require'
from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/basic_test.rb:3:in `<top (required)>'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:12:in `require'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:12:in `block (2 levels) in <main>'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:11:in `each'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:11:in `block in <main>'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:5:in `select'
from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:5:in `<main>'
@@ -134,7 +134,7 @@ def test_point_to_json | |||
obj = SpatialModel.new | |||
assert_match(/"latlon":null/, obj.to_json) | |||
obj.latlon = factory.point(1.0, 2.0) | |||
assert_match(/"latlon":"POINT\s\(1\.0\s2\.0\)"/, obj.to_json) | |||
assert_match(/"latlon":"POINT\s\(1(\.0)?\s2(\.0)?\)"/, obj.to_json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes this test failure:
1) Failure:
BasicTest#test_point_to_json [/Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/basic_test.rb:137]:
Expected /"latlon":"POINT\s\(1\.0\s2\.0\)"/ to match "{\"id\":null,\"latlon\":\"POINT (1 2)\",\"latlon_geo\":null}".
I didn't research what caused the change with this decimal formatting
Merged with latest changes in main, fixed merge conflicts |
Hello. Will this PR be reviewed? |
Changes necessary to build the gem and get passing tests with Rails 7.2
Note: The CI build fails with Ruby 2.7. Would the maintainers of https://github.com/stadia/activerecord-mysql2rgeo-adapter be open to the idea of dropping support for Ruby 2.x?