fix jsonify handling of boolean values #3154

Merged
merged 1 commit into from Nov 27, 2014

Conversation

Projects
None yet
3 participants
@afeld
Contributor

afeld commented Nov 26, 2014

true.to_liquid == true, so as_liquid(true) was an infinite loop 馃挘 馃挜 馃憡 Noticed while trying to use jsonify.

@afeld afeld changed the title from fix filter handling of boolean values to fix jsonify handling of boolean values Nov 26, 2014

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 26, 2014

Member

Whoops. Good catch!

Member

parkr commented Nov 26, 2014

Whoops. Good catch!

@parkr parkr added the fix label Nov 26, 2014

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 26, 2014

Member

Maybe JSON.parse(JSON.generate(input))? 馃槢

Mostly not kidding.

Member

parkr commented Nov 26, 2014

Maybe JSON.parse(JSON.generate(input))? 馃槢

Mostly not kidding.

@afeld

This comment has been minimized.

Show comment
Hide comment
@afeld

afeld Nov 26, 2014

Contributor

Yeah, I was kinda thinking something like

if item.to_liquid == item
  item
else
  # ...
end

but figured there were a lot of edge cases that I didn't want to have to worry about 馃槈

Contributor

afeld commented Nov 26, 2014

Yeah, I was kinda thinking something like

if item.to_liquid == item
  item
else
  # ...
end

but figured there were a lot of edge cases that I didn't want to have to worry about 馃槈

@afeld

This comment has been minimized.

Show comment
Hide comment
Contributor

afeld commented Nov 26, 2014

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 26, 2014

Member

Definitely up for the == check you mentioned. Want to add that here? Like this, maybe? I didn't test this:

def as_liquid(item)
  case item
  when Hash
    Hash[item.map { |k, v| [as_liquid(k), as_liquid(v)] }]
  when Array
    item.map{ |i| as_liquid(i) }
  else
    liquidated = item.respond_to?(:to_liquid) ? as_liquid(item.to_liquid) : item
    if liquidated == item
      item
    else
      as_liquid(liquidated)
    end
  end
end
Member

parkr commented Nov 26, 2014

Definitely up for the == check you mentioned. Want to add that here? Like this, maybe? I didn't test this:

def as_liquid(item)
  case item
  when Hash
    Hash[item.map { |k, v| [as_liquid(k), as_liquid(v)] }]
  when Array
    item.map{ |i| as_liquid(i) }
  else
    liquidated = item.respond_to?(:to_liquid) ? as_liquid(item.to_liquid) : item
    if liquidated == item
      item
    else
      as_liquid(liquidated)
    end
  end
end
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 26, 2014

Member

Also, we might not be resolving procs... Hm!

Member

parkr commented Nov 26, 2014

Also, we might not be resolving procs... Hm!

@afeld

This comment has been minimized.

Show comment
Hide comment
@afeld

afeld Nov 27, 2014

Contributor

Not a regression 馃槈 I'll try and refactor and submit a separate PR, but would be great to have this specific fix in.

Are there any types we don't want to resolve? Re: procs, is arbitrary code execution fair game?

Contributor

afeld commented Nov 27, 2014

Not a regression 馃槈 I'll try and refactor and submit a separate PR, but would be great to have this specific fix in.

Are there any types we don't want to resolve? Re: procs, is arbitrary code execution fair game?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 27, 2014

Member

@afeld Whoa, you're up late.

Are there any types we don't want to resolve? Re: procs, is arbitrary code execution fair game?

Yep, any code that is pumped into the system should be executed. In particular, we have the github-metadata plugin which is just a bunch of procs 鈥 would be nice not to have to call them on our side if they don't require arguments. Scanning the Liquid code, it looks like Procs are resolved when they're rendered, but not under-the-hood (so only if they're top-level).

Member

parkr commented Nov 27, 2014

@afeld Whoa, you're up late.

Are there any types we don't want to resolve? Re: procs, is arbitrary code execution fair game?

Yep, any code that is pumped into the system should be executed. In particular, we have the github-metadata plugin which is just a bunch of procs 鈥 would be nice not to have to call them on our side if they don't require arguments. Scanning the Liquid code, it looks like Procs are resolved when they're rendered, but not under-the-hood (so only if they're top-level).

parkr added a commit that referenced this pull request Nov 27, 2014

@parkr parkr merged commit 932cd3b into jekyll:master Nov 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

parkr added a commit that referenced this pull request Nov 27, 2014

@afeld afeld deleted the afeld:jsonify-bool branch Nov 27, 2014

@afeld afeld referenced this pull request Nov 27, 2014

Merged

refactor #as_liquid #3158

cmitchusa added a commit to cmitchusa/jekyll that referenced this pull request Nov 29, 2014

cmitchusa added a commit to cmitchusa/jekyll that referenced this pull request Nov 29, 2014

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.