tobyhinloopen / bbcode

BBcode parser designed to be used with Ruby on Rails, capable of converting BBcode-formatted strings to HTML or any other format you like
12 stars 4 forks source link

`locals.with_indifferent_access` is called twice #2

Open stex opened 7 years ago

stex commented 7 years ago

Hey!

I hope you haven't abandoned the gem yet as it is really useful due to its customisability :)

I'm currently using it for a case which is not exactly a default configuration: I need to extract BBCodes from a string and replace them with a placeholder while capturing the content, e.g.

s  = "foo\n [html id='something']bar[/html]baz"
ls = {}
s.as_bbcode.to :enhanced_string, ls
#=> "foo\n PLACEHOLDERbaz"
ls
#=> {:PLACEHOLDER => {:type => :html, ...}}

The main problem here is that the locals are not passed to the elements as is, but rather after being converted to a HashWithIndifferentAccess - and that's done twice:

As I've written my own handler, it's quite easy to override the locals= method to use the original hash, but as it's done again in #to, I need to actually monkey-patch the method in my application.

Wouldn't it be sufficient to completely rely on the handler's attribute setter?

tobyhinloopen commented 7 years ago

If I understand correctly, you're using locals as a mutable hash to update from your handler and you're using the mutated locals map to extract information.

I'm thinking of adding a method to Bbcode.AbstractHandler that converts the document's content to a string, and changing the result = handler.get_document.content.to_s line in lib/bbcode/base.rb#to to something like result = handler.format_result. The default implementation would be something like get_document.content.to_s. You can then override format_result to return anything you like, in your case something like ["foo\n PLACEHOLDERbaz", {:PLACEHOLDER => ...}] That way, you don't need to rely on mutating locals and you have any state you like within the handlers.

You can still mutate locals this way, but you can return the mutated locals (which is a copy of the input param) instead of using the hash you used as input param.

This won't break any existing usage of the gem and I think it will fix your problem.

What do you think? @Stex

stex commented 7 years ago

@tobyhinloopen Thanks for your quick response!

If I understand correctly, you're using locals as a mutable hash to update from your handler and you're using the mutated locals map to extract information.

Exactly. I need to not only replace the BBCodes, but also make the original information available in the scope which called the BBCode parsing.

I think that would indeed help me. I'd still have to mutate the locals as each tag might behave a bit differently and the result therefore could not be created purely in the format_result method. The ability to return the new locals hash as well as the resulting string would at least mean that I don't have to pass in a global hash any more.

An example of what I'm currently doing:

Bbcode.register_handler :enhanced_post, Bbcode::PostHandler.new(
  html: lambda {|elem, locals|
    "#{locals[:placeholder_prefix]}_#{locals[:placeholder_idx] += 1}".tap do |placeholder|
      (locals[:elements] ||= {})[placeholder] = {
        content: elem.content,
        attributes: elem.attributes
      }
    end
  }
)

[1] pry(main)> s = "asdf\n[html id='asdf']asdfasdfasdfasdf[/html]\nasdfasdf"
=> "asdf\n[html id='asdf']asdfasdfasdfasdf[/html]\nasdfasdf"
[2] pry(main)> ll = {placeholder_prefix: 'PLACEHOLDER', placeholder_idx: 0}
=> {:placeholder_prefix=>"PLACEHOLDER", :placeholder_idx=>0}
[3] pry(main)> s.as_bbcode.to :enhanced_post, ll
=> "asdf\nPLACEHOLDER_1\nasdfasdf"
[4] pry(main)> ll
=> {:placeholder_prefix=>"PLACEHOLDER", :placeholder_idx=>1, :elements=>{"PLACEHOLDER_1"=>{:content=>["asdfasdfasdfasdf"], :attributes=>{"id"=>"asdf"}}}}
tobyhinloopen commented 7 years ago

See branch get-result-from-handler for an example. You can override the result method to return something like [super, @locals]

stex commented 7 years ago

@tobyhinloopen This works fine for me, I basically just removed my monkey-patched version of Bbcode::Base and overrode result in my own handler. Thanks for your help, looking forward to seeing this merged into master :)

I had to change my handler slightly, even though I'm not yet sure why:

(locals[:elements] ||= {})[placeholder] = {}

lead to the locals having the following key/value pair: {:elements => {}}

The following code however works completely fine, I'll have to look into that sometime, might be related to ActiveSupports HashWithIndifferentAccess

locals[:elements] ||= {}
locals[:elements][placeholder] = {}