Fix wrong behavior with numeric slugs #368

Closed
wants to merge 3 commits into
from

Projects

None yet

3 participants

@kshnurov
kshnurov commented Feb 9, 2013

Instead of responding with 404 on non-existent slug (e.g. '1001-qwe')
friendly_id redirects to the object with id = 1001.

It happens because of friendly_id? returning nil (True is the id is
definitely friendly, false if definitely unfriendly, else nil.), and it's not checked later.

kshnurov added some commits Feb 9, 2013
@kshnurov kshnurov Fix wrong behavior with numeric slugs
Instead of responding with 404 on non-existent slug (e.g. '1001-qwe')
friendly_id redirects to the object with id = 1001.

It happens because of friendly_id? returning nil (# True is the id is
definitely friendly, false if definitely unfriendly, else nil.)
6e7a1a8
@kshnurov kshnurov Fix wrong slug increment
If slug is starting with digits, increment is working wrong, taking
that digits as a sequence number.
6b8b2be
@kshnurov kshnurov referenced this pull request Feb 9, 2013
Closed

Wrong slug increment #367

@parndt parndt commented on the diff Feb 28, 2013
lib/friendly_id/finder_methods.rb
@@ -14,7 +14,11 @@ module FinderMethods
# @see FriendlyId::ObjectUtils
def find_one(id)
return super if id.unfriendly_id?
- where(@klass.friendly_id_config.query_field => id).first or super
+ if id.friendly_id?
+ where(@klass.friendly_id_config.query_field => id).first or raise ActiveRecord::RecordNotFound
+ else
+ where(@klass.friendly_id_config.query_field => id).first or super
+ end
@parndt
parndt Feb 28, 2013 Collaborator

A bit of code duplication going on here. Perhaps this should be:

where(@klass.friendly_id_config.query_field => id) or id.friendly_id? ? raise ActiveRecord::RecordNotFound : super

Or even:

where(@klass.friendly_id_config.query_field => id) or if id.friendly_id?
  raise ActiveRecord::RecordNotFound
else
  super
end

The point I'm trying to make is that we should avoid repeating ourselves. :-)

@parndt parndt commented on the diff Feb 28, 2013
lib/friendly_id/history.rb
@@ -89,17 +89,28 @@ module FinderMethods
# Search for a record in the slugs table using the specified slug.
def find_one(id)
return super(id) if id.unfriendly_id?
- where(@klass.friendly_id_config.query_field => id).first or
- with_old_friendly_id(id) {|x| where(:id => x).first} or
- find_one_without_friendly_id(id)
+ if id.friendly_id?
+ where(@klass.friendly_id_config.query_field => id).first or
+ with_old_friendly_id(id) {|x| where(:id => x).first} or
+ raise ActiveRecord::RecordNotFound
+ else
+ where(@klass.friendly_id_config.query_field => id).first or
+ with_old_friendly_id(id) {|x| where(:id => x).first} or
+ find_one_without_friendly_id(id)
+ end
@parndt
parndt Feb 28, 2013 Collaborator

Same as my comments above, there is far too much duplication going on here.

@parndt parndt commented on the diff Feb 28, 2013
lib/friendly_id/history.rb
end
# Search for a record in the slugs table using the specified slug.
def exists?(id = false)
return super if id.unfriendly_id?
- exists_without_friendly_id?(@klass.friendly_id_config.query_field => id) or
- with_old_friendly_id(id) {|x| exists_without_friendly_id?(:id => x)} or
- exists_without_friendly_id?(id)
+ if id.friendly_id?
+ exists_without_friendly_id?(@klass.friendly_id_config.query_field => id) or
+ with_old_friendly_id(id) {|x| exists_without_friendly_id?(:id => x)}
+ else
+ exists_without_friendly_id?(@klass.friendly_id_config.query_field => id) or
+ with_old_friendly_id(id) {|x| exists_without_friendly_id?(:id => x)} or
+ exists_without_friendly_id?(id)
@parndt
parndt Feb 28, 2013 Collaborator

Same as my comments above, there is far too much duplication going on here.

@parndt parndt commented on the diff Feb 28, 2013
lib/friendly_id/slug_generator.rb
@@ -31,7 +31,11 @@ def last_in_sequence
end
def extract_sequence_from_slug(slug)
- slug.split("#{normalized}#{separator}").last.to_i
+ if slug.include? "#{normalized}#{separator}"
+ slug.split("#{normalized}#{separator}").last.to_i
+ else
+ return 0
+ end
@parndt
parndt Feb 28, 2013 Collaborator

Doesn't an empty string or nil evaluate to 0 already?

> "".split("foo").last
 => nil
> "".split("foo").last.to_i
 => 0
@parndt
Collaborator
parndt commented Feb 28, 2013

Thanks for sending the PR, I've added some review comments ❤️

@norman
Owner
norman commented Apr 16, 2013

I agree with @parndt that this isn't ready to be pulled. Closing for now as stale.

@norman norman closed this Apr 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment