Remove warning for serialize in rails 7.1+

[jwoodrow]

There was a warning that kept popping up because of how globalize was using serialize and the changes that are in effect in Rails 7.1 and will be permanently changed in Rails 7.2

DEPRECATION WARNING: Passing the class as positional argument is deprecated and will be removed in Rails 7.2.

Please pass the class as a keyword argument:

serialize :metadata, type: Object

This PR uses the ActiveRecord conditions used to display the deprecation message and uses it to handle the detection between coder/type

Also uses the build_column_serializer private method to build the coder assigned to globalize_serialized_attributes

[jwoodrow]

This is what ActiveRecord does to display the deprecation message & make the function still work

[jwoodrow]

decided to use the build_column_serializer private method on the class

[jwoodrow]

tests are working on the fork

[jpawlyn]

Thanks for working on this @jwoodrow 👍🏼 . For the last commit fix tests we are seeing all tests fail in our app with the message:

Psych::DisallowedClass:
       Tried to dump unspecified class: ActiveSupport::HashWithIndifferentAccess

Not sure why this is but will add further details if we discover the reason. Without that commit, everything looks to be working fine.

[jwoodrow]

That's odd especially because I haven't changed anything special and I haven't used any with_indifferent_access either 🤔

[jpawlyn]

Investigating the error above, it seems that when the ActiveStorage::Blob class is being loaded, the coder set in ActiveRecord::Store:

https://github.com/rails/rails/blob/6b93fff8af32ef5e91f4ec3cfffb081d0553faf0/activerecord/lib/active_record/store.rb#L108

is being overwritten by the class ActiveRecord::Coders::YAMLColumn with the call to options.merge(coder: ... before the call to super but this doesn't happen in the previous commit.

My tests fail when an instance of ActiveStorage::Blob is created. Not sure what the solution is though 🤷🏼 .

This is undoubtedly not the solution but when I added the following the tests did pass:

        if options[:coder].blank?
          if class_name_or_coder == ::JSON || [:load, :dump].all? { |x| class_name_or_coder.respond_to?(x) }
            options = options.merge(coder: class_name_or_coder, type: Object)
          else
            options = options.merge(coder: default_column_serializer, type: class_name_or_coder)
          end
        end

        super(attr_name, **options)

[jwoodrow]

This is undoubtedly not the solution but when I added the following the tests did pass:

Kind of makes sense, if coder was provided by keyword chances are that there shouldn't be a need to determine a default value for coder or type 🤔

[jwoodrow]

After trying to update one of our codebases that use globalize I finally encountered the same error with ActiveStorage::Blob. After adding the if options[:coder].blank? the specs still pass and the error no longer seems to happen so I've updated the code

[arikarim]

any Update on merging this?

[thomasdarde]

Hi Any update on this issue ? It's not clear what is required for checks to pass ?

[jpawlyn]

Hi Any update on this issue ? It's not clear what is required for checks to pass ?

We switched over to Mobility following this recommendation and that worked out pretty well and was fairly seemless.

[ilvez]

We switched over to Mobility following this recommendation and that worked out pretty well and was fairly seemless.

Thanks, how did I not notice this suggestion?! There is also migration guide: https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize

EDIT: One thing that this migration should mention is that with_translations is missing feature. Some hints I picked up from here: shioyama/mobility#390 (comment) . Some of the queries, which were using with_translations(locale), I just split into regular queries like:

      .where(model_translations: { locale: })
      .where.not(model_translations: { name: nil })

.. or something similar.

[Splines]

We switched over to Mobility following this recommendation and that worked out pretty well and was fairly seemless.

That's one option, thanks for pointing it out. It'd be awesome if this PR could be finalized/merged such that those who want to upgrade to Rails 7.1 and only after that switch to mobility in a separate step could do so without all these deprecation warnings.

Edit: At MaMpf, we switched from globalize to mobility recently and the transition was really easy and it works like a charm. Can recommend to do so as well.