strip_diacritics fails if column is blank

[grimen]

This is such a small fix, so pasting it here:

Raises error:

The error occurred while evaluating nil.unpack

Fixed version:

    def strip_diacritics(string)
      ActiveSupport::Multibyte.proxy_class.new(string).normalize(:kd).unpack('U*').inject([]) { |a, u| 
        if ASCII_APPROXIMATIONS[u]
          a += ASCII_APPROXIMATIONS[u].unpack('U*')
        elsif (u < 0x300 || u > 0x036F)
          a << u
        end
        a
      }.pack('U*')
    rescue
      nil
    end

..which will instead lead to the somewhat nicer exception:

FriendlyId::SlugGenerationError (The slug text is blank.)

...which I believe one should be able to optinalyl skip by using this excellent addition (gets my vote, becuase these are just unnecessary errors IRL):

http://github.com/blythedunham/friendly_id/commit/21ae5137d1a45b4c198669e0b1c0f320b062fba8

[norman]

Thanks for posting this, I'm going to get a fix committed today.

With few exceptions, "rescue nil" is something very much to be avoided except in quick throwaway code, because it swallows any error that possibly happens, and gives you absolutely no hint as to what occured. It's something that really should almost never be used in production code.

However, the problem you're pointing out here is obviously real and needs to be fixed. I think a better way to do this is to assign the output of ActiveSupport::Multibyte.proxy_class.new(string).normalize(:kd) to a variable, and return "" if it's nil rather than simply swallow the errors.

I had looked at the patch from the fork that you referenced and didn't apply it for a few reasons. One, friendly_id now has a ton of config options, and to me that's an indication that I'm already doing something wrong and it needs a little refactoring. :-)

Two, I'm not convinced that this problem is best solved by adding a new feature, because this is a somewhat unusual occurance (it's only been mentioned twice in 2 years), and you can already accomplish the same thing by doing this:

class Post < ActiveRecord::Base
  has_friendly_id :title, :use_slug => true do |text|
    Slug.normalize(text)
  rescue FriendlyId::SlugGenerationError
    # backup slugging code here
  end
end

However, again you point out another real problem: FriendlyId should really document this better because I'm guessing almost nobody knows that this option is available. So I will add that as well.

Thanks for taking the time to give me your feedback and please don't think I'm being a jerk by not including your fix, what you posted was very helpful despite the fact that I want to resolve the issue in a different way.

[grimen]

Hah, no problem - just got one of my little boats floating. =) Agreed. Yea, I actually thinks this is a "documentation bug", because that solution up there would make my boat float too - almost making it a yacht. =D

[grimen]

This issue is still not resolved, and the proposed solution (above) don't work. =(

NoMethodError (You have a nil object when you didn't expect it!
The error occurred while evaluating nil.unpack):
  /opt/local/lib/ruby/gems/1.8/gems/norman-friendly_id-2.1.4/lib/friendly_id/slug.rb:55:in `strip_diacritics'
  /opt/local/lib/ruby/gems/1.8/gems/norman-friendly_id-2.1.4/lib/friendly_id/sluggable_instance_methods.rb:65:in `slug_text'
  /opt/local/lib/ruby/gems/1.8/gems/norman-friendly_id-2.1.4/lib/friendly_id/sluggable_instance_methods.rb:102:in `set_slug'

[grimen]

With:

has_friendly_id :nickname, :use_slug => true, :strip_diacritics => true do |value|
    begin
      ::Slug.normalize(value)
    rescue ::NoMethodError, ::FriendlyId::SlugGenerationError # Value is nil or blank (most probably).
      # skip - or handle it here
    end
  end

I still get:

FriendlyId::SlugGenerationError (The slug text is blank.)

[norman]

Sorry, I've been traveling and haven't had time to look into this until now. I took a longer look at this and really there's no need for a patch to get the functionality you want. You can do something like this:

class Person < ActiveRecord::Base
  has_friendly_id :name, :use_slug => true do |val|
      s = Slug.strip_diacritics(Slug.normalize(val))
      s.blank? ? "EMPTY" : s 
  end 
end

[grimen]

OK, thanks will try it out. But you will patch the ni.unpack error right? It's indeed ugly - should at least be re-raised by friendly_id with an exception that makes sense. Not at least for the cause of conventions and avoiding confusion. As well as my partner I that odd error made us feel like we did something really crazy - but really did not, just had a regular blank/nil value.

[norman]

Yeah, I'm just not going to make it raise anything like I had been thinking to do, but rather return the empty string.

[grimen]

What's the consensus on this one? 0_o This is still an issue for me even with the latest gem - and no clean solution documented. I think this is the type of stuff that should work out of the box, right?

[norman]

Can you send me some sample code that reproduces the error you're having? - Perhaps create a trivial Rails app and send a tarball? I'm not sure why the code I posted here on Oct 7 doesn't work for you.

I definitely see this as an unusual edge case - it's been brought up twice in 2 years. I don't want to add a whole feature just to repair this, given that I think what I posted here should work. But perhaps seeing your code would convince me.

[grimen]

First off even if it can be handled, this error is nasty and should not be possible to get even if you do "correct" or not:

You have a nil object when you didn't expect it! The error occurred while evaluating nil.unpack

With an altered version, on-create this won't work (seems like there's no id to generate to_param yet):

  has_friendly_id :nickname, :use_slug => true, :strip_diacritics => true do |value|
    begin
      ::Slug.strip_diacritics(::Slug.normalize(value))
    rescue ::FriendlyId::SlugGenerationError # Value is nil or blank (most probably).
      self.to_param # or self.id.to_s
    end
  end

If that would have work, this would been DRYer of course:

has_friendly_id :nickname, :use_slug => true, :strip_diacritics => true do |value|
    ::Slug.strip_diacritics(::Slug.normalize(value)) rescue self.to_param
  end

Of course I can make one up with my own generation, but then when one needs to monkey-patch a plugin in every app something is def missing. An obvious fallback would be to just use the default to_param (id) value, but after save.

Still I doubt the filed tickets tells the true story; that this is not a common scenario, it must be a very common scenario with blank/nil fields. What I know is that many Ruby developers rater monkey-patch than file issues. =)

[norman]

What I'm having a hard time understanding is, what input gives rise to this situation? When somebody submits a string with nothing but diacritics?

[grimen]

A user signs up with no "nickname" is just one scenario in my current app. Another one is a uploaded art design without a name, etc.

[norman]

Why aren't you handling that with validation rules in your model?

[grimen]

That's because apps - in my point of view - should not force users to specify data that is partly optional (in this case: nickname is, design name is). I won't get into how to design apps - there are plenty of ways, and I'm not unique in any way. If the plugin is only designed for certain type of apps, say enterprise apps (aka bureaucracy apps), then there's an answer why this is not to be handled. Sorry, but I actually don't really understand the over-conservative look on this at all. =S Handling nil/null value is jis a best practice in any programming environment, and throwing non-saying exceptions like "The error occurred while evaluating nil.unpack" on this high level is not good practice I'm afraid. There's a very simple solution on this, so why be conservative?

1. To avoid the on-create error: perform the slugging after save (now seems to be happening before, which causes the issue)
2. Optionally, handle blank/nil by doing nothing.

[grimen]

Haven't looked into the source much, but could help out with a solution on this of course. The second one is just a one-liner, to make nil => "", which will instead raise the ::FriendlyId::SlugGenerationError - already much better. Still the on-create-issue is a problem.

[norman]

If I didn't think you had a point, this thread wouldn't be so long. :-)

In fact I did add changes to friendly_id to fix what I thought was the problem, but it didn't. So I'm just asking you some questions because obviously I have not been able to fully understand the problem yet.

Really the single best thing you could do to help me would be to fork friendly_id, and add a test or two that shows the problem you're having.

[norman]

And by test, I mean "failing test."

[norman]

Ok, I took a look and basically the patch I added before wasn't being adequately tested, and so it didn't address the issue like I thought it did. I just committed code in the "edge" branch that resolves the issue for once and for all (at least as far as code goes; documentation will be added later as I'm working on a new site and docs for friendly_id.)

I'll release a new gem as soon as I get another few small patches in place.

[grimen]

The edge version don't raise the nil.unpack error which is good, but there seems to be a new issue instead: The Slug::GenerationError can't be catched anymore. Not the same issue as on-create, because then I could return a hard-coded string like so:

begin debugger ::Slug.normalize(value) # fails in slug.rb:88 rescue ::FriendlyId::SlugGenerationError debugger # <<< don't reach this point anymore, very odd 'hsjdhsjhdhd' end

...but this dont work anymore for some reason (worked before with hard-coded string). I think I'll put together a spec or a real live project to show what's happening. Upcoming days if I get some spare-time.