Rails 5.1

[lazyatom]

The library itself seems to work with Rails 5.1, but the build needs a few tweaks.

[lazyatom]

Closing and reopening to trigger the build again; it seems to be an intermittent problem with bundler installing rack.

[scytherswings]

@lazyatom out of curiosity, what's blocking this PR from being merged?

[lazyatom]

@scytherswings aside from the build being troubled with bundle installation issues, I think this could be merged...

... but what actually prompted me to make this branch is that in Rails 5.1, calls to to_param that occur within after_save callbacks will raise deprecation warnings. I'm not sure if a) this is actually addressable, and b) whether or not friendly id should address it. The warnings take the form

DEPRECATION WARNING: The behavior of <X> inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after save returned (e.g. the opposite of what it returns now). To maintain the current behavior, use saved_change_to_attribute? instead. (called from to_param at friendly_id/lib/friendly_id/base.rb:261)

where <X> is one of the following: attribute_changed?, changed_attributes, changes, changed, or attribute_change.

If I understand correctly what's going on, this is because the behaviour of attribute_changed? (which gets called within to_param) is going to change in Rails 5.2.

To illustrate, here's a simple model:

class Thing < ActiveRecord::Base
  extend FriendlyId
  friendly_id :name, use: :slugged

  after_save do
    trigger_notification_or_something(thing_url(self))
  end
end

Let's imagine this posts some notification to a service where we want to include the URL to this record (yes, possibly not the best architecture, but bear with me; the main point is that to_param gets called implicitly). Next, we update a record:

thing = Thing.first
thing.name # => 'Old name'
thing.name = 'New name'
thing.save

After the save, the callback is invoked which generates a URL, and because to_param gets called as part of that, friendly id will call attribute_changed? to determine whether or not the slug column was going to be modified as part of the save, so that it can generate the right URL with the right slug.

In Rails 5.1 and earlier, attribute_changed? will return true, and changes[:name] will return ['Old name', 'New name']. In Rails 5.1 specifically, we also get a noisy deprecation warning (actually a bunch) of the form above. In Rails 5.2, attribute_changed? will return false, because, this callback is invoked after the change has been persisted to the database and so, from a fairly defensible position, the value in the in-memory record and the value in the database are no longer different and so... "not changed".

It's likely that the new behaviour is actually what we want; the current implementation of to_param means that we actually return 'Old name' as the sluggable value within the callback, even though once the callback finishes, the record should be reached using a slug based on 'New name'.

However, even aside from that, because calling to_param happens so, so often in Rails applications, this is a real problem for drowning logs in deprecation warnings. In our own application, I've had to silence them in an initializer:

default_deprecation_behaviours = ActiveSupport::Deprecation.behavior
ActiveSupport::Deprecation.behavior = ->(message, callstack) {
  if Rails::VERSION::MAJOR == 5 && Rails::VERSION::MINOR > 1
    raise "Remove friendly_id deprecation silencing patch!"
  end
  unless callstack.find { |l| l.path =~ /gems\/friendly_id/ } &&
    message =~ /The behavior of .* inside of after callbacks will be changing/
    default_deprecation_behaviours.each { |b| b.call(message, callstack) }
  end
}

It could be that nobody else in the world is doing anything in ActiveRecord after_save callbacks that invokes to_param, but I'm a little surprised that this hasn't already come up. Maybe I've missed a duplicate issue?

[scytherswings]

@lazyatom wow thank you for such a well thought out response, I really appreciate it. I had no idea to_param was called so often.
It seems like this change might want to wait till Rails 5.2, but then again I'm not sure when that will be released. I'm sure you know how bad it is to get behind on rails releases. Perhaps your silencer is the right way to go in the meantime. It's not pretty, but until that deprecation is finished I'm not sure how friendly_id is supposed to handle this issue. Stuck between a rock and a hard place it seems.

[elithecho]

Was upgrading and reading around. seems to be attribute_changed? in here.

We'd have to do (correct me if I'm wrong)

if saved_change_to_attribute?(friendly_id_config.query_field)
  diff = saved_changes[friendly_id_config.query_field]
else

Based on the name changes.

[yjukaku]

AFAICT, @choonggg, what we need is will_save_change_to_attribute? since we just want to know if the attribute has changed in memory (right?).

Additionally, if FriendlyID wants to retain backwards compatibility, then we need to be sure that the will_save_change_to_attribute? method exists.

That would make the method definition at

friendly_id/lib/friendly_id/base.rb

Line 262 in e873122

def to_param

    # Either the friendly_id, or the numeric id cast to a string.
    def to_param
      if friendly_id_config.routes == :friendly
        attribute_changed = if self.respond_to?(: will_save_change_to_attribute?)
          will_save_change_to_attribute?(friendly_id_config.query_field)
        else
          attribute_changed?(friendly_id_config.query_field)
        end
        if attribute_changed
          diff = changes_to_save[friendly_id_config.query_field]
          diff.first || diff.second
        else
          friendly_id.presence.to_param || super
        end
      else
        super
      end
    end

@lazyatom Does that seem correct to you?

[yjukaku]

@lazyatom I believe the reason that the attr_changed? method is used in the current to_param method is to make this test pass: https://github.com/norman/friendly_id/blob/master/test/slugged_test.rb#L407

The expected user case is probably something like this:

def update
  if @user.update_attributes(slug: "notvalid")
    #...
  else
    flash[:error] = "Failed update because slug is invalid!"
    redirect_to @user
  end
end

In that case, we want to_param to use the 'old' version of slug. I just submitted a PR to this repo to keep this behavior for Rails 5.1, if you can take a look.

[lazyatom]

Closing this in favour of #862, which includes a patch to the actual library. I've suggested a test that also more concretely demonstrates the issue; I believe the callback behaviour is still broken in Rails 5.1, even if it's fixed in Rails 5.2.