Erubi support #2090

Merged
merged 5 commits into from Mar 13, 2017

Conversation

Projects
None yet
3 participants
@adam12
Contributor

adam12 commented Nov 14, 2016

Definitely not ready for merging, but looking for some feedback as I am still mostly unfamiliar with the Padrino codebase.

A couple items I feel I need some guidance on:

  • Best places to insert tests for this? Tested via existing code.
  • Does this require a generator? I noticed there is an Erubis dependency generator in padrino-gen. Replaced Erubis with Erubi.
  • I've had to temporarily comment out a block of code that re-loads erb to have my handler actually called (d53641f). Can we check if there is a Tilt handler registered and skip the require? what's the best way to approach this? No longer relevant.
  • I had to duplicate configuration for the engine configuration in two different keys. Looks like a smell to me - open to alternatives. (dce2cc3) No longer relevant.
  • Am I using SafeBuffer correctly? I believe i am.
  • Raw output is being escaped when it shouldn't be (<%== should not be escaped) Added test case and fix.

Finally, Erubi supports an interesting block-capture method in a yet-to-be released gem version. I'm not sure if this is something that should be supported or not.

Issue #2089

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 15, 2016

Member

Best places to insert tests for this?

Temporarily it could be tested by using environment variable

if ENV['STDLIB_ERB']
puts "=> Using stdlib ERB engine"
else
gem "erubis", ">= 2.7.0"
end

Does this require a generator? I noticed there is an Erubis dependency generator in padrino-gen.

Yes. It should add gem 'erubi' to the project Gemfile, nothing more.

I've had to temporarily comment out a block of code that re-loads erb to have my handler actually called (d53641f). Can we check if there is a Tilt handler registered and skip the require? what's the best way to approach this?

That code is fail-safe for when there's no rendering engine gem in the project Gemfile. It should work the same with this code not commented out.

I had to duplicate configuration for the engine configuration in two different keys. Looks like a smell to me - open to alternatives. (dce2cc3)

I suggest to solve one problem at a time. Register erb extension like ERB and Erubis renderers do, then solve potential problems of having different ERB dialects with different extensions.

Am I using SafeBuffer correctly?

No. A renderer should utilize SafeBuffer#safe_concat.

Member

ujifgc commented Nov 15, 2016

Best places to insert tests for this?

Temporarily it could be tested by using environment variable

if ENV['STDLIB_ERB']
puts "=> Using stdlib ERB engine"
else
gem "erubis", ">= 2.7.0"
end

Does this require a generator? I noticed there is an Erubis dependency generator in padrino-gen.

Yes. It should add gem 'erubi' to the project Gemfile, nothing more.

I've had to temporarily comment out a block of code that re-loads erb to have my handler actually called (d53641f). Can we check if there is a Tilt handler registered and skip the require? what's the best way to approach this?

That code is fail-safe for when there's no rendering engine gem in the project Gemfile. It should work the same with this code not commented out.

I had to duplicate configuration for the engine configuration in two different keys. Looks like a smell to me - open to alternatives. (dce2cc3)

I suggest to solve one problem at a time. Register erb extension like ERB and Erubis renderers do, then solve potential problems of having different ERB dialects with different extensions.

Am I using SafeBuffer correctly?

No. A renderer should utilize SafeBuffer#safe_concat.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 15, 2016

Member

I tinkered with Erubi a bit and I think there's no easy way to fix #1785 in this implementation.

My SafeErubi code:

module Padrino
  module Rendering
    class SafeErubi < Erubi::Engine
      private

      def add_text(text)
        @src << " #{@bufvar}.safe_concat '" << text.gsub(/['\\]/, '\\\\\&') << "';" unless text.empty?
      end
    end

    class ErubiTemplate < Tilt::ErubiTemplate
      def precompiled_preamble(*)
        "__in_erb_template = true\n" << super
      end
    end
  end
end

Tilt.register(Padrino::Rendering::ErubiTemplate, :erb)

Padrino::Rendering.engine_configurations[:erb] = {
  :bufval => "SafeBuffer.new",
  :bufvar => "@_out_buf",
  :engine_class => Padrino::Rendering::SafeErubi,
}
Member

ujifgc commented Nov 15, 2016

I tinkered with Erubi a bit and I think there's no easy way to fix #1785 in this implementation.

My SafeErubi code:

module Padrino
  module Rendering
    class SafeErubi < Erubi::Engine
      private

      def add_text(text)
        @src << " #{@bufvar}.safe_concat '" << text.gsub(/['\\]/, '\\\\\&') << "';" unless text.empty?
      end
    end

    class ErubiTemplate < Tilt::ErubiTemplate
      def precompiled_preamble(*)
        "__in_erb_template = true\n" << super
      end
    end
  end
end

Tilt.register(Padrino::Rendering::ErubiTemplate, :erb)

Padrino::Rendering.engine_configurations[:erb] = {
  :bufval => "SafeBuffer.new",
  :bufvar => "@_out_buf",
  :engine_class => Padrino::Rendering::SafeErubi,
}
@adam12

This comment has been minimized.

Show comment
Hide comment
@adam12

adam12 Nov 15, 2016

Contributor

Which part of #1785 - the value returned to a string from a block?

Contributor

adam12 commented Nov 15, 2016

Which part of #1785 - the value returned to a string from a block?

@adam12

This comment has been minimized.

Show comment
Hide comment
@adam12

adam12 Nov 19, 2016

Contributor

@ujifgc I noticed you started an ERb branch that looks similar to Erubi. Should I hold off on this branch?

Contributor

adam12 commented Nov 19, 2016

@ujifgc I noticed you started an ERb branch that looks similar to Erubi. Should I hold off on this branch?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 19, 2016

Member

I just tinkered with an internal implementation of opinionated ERB. It has no settings, so it's not a replacement for ERB, Erubis or Erubi.

This PR can be fully functional after the release of Erubi with my latest PR jeremyevans/erubi#1

Member

ujifgc commented Nov 19, 2016

I just tinkered with an internal implementation of opinionated ERB. It has no settings, so it's not a replacement for ERB, Erubis or Erubi.

This PR can be fully functional after the release of Erubi with my latest PR jeremyevans/erubi#1

@adam12

This comment has been minimized.

Show comment
Hide comment
@adam12

adam12 Feb 9, 2017

Contributor

Just to update this,

I suspect we'll be able to simplify or forget about this pull request, as soon as rtomayko/tilt@b7e2652 is shipped (if it's not already).

Contributor

adam12 commented Feb 9, 2017

Just to update this,

I suspect we'll be able to simplify or forget about this pull request, as soon as rtomayko/tilt@b7e2652 is shipped (if it's not already).

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 7, 2017

Member

Tilt 2.0.6 has been released.

Member

ujifgc commented Mar 7, 2017

Tilt 2.0.6 has been released.

adam12 added some commits Nov 14, 2016

@adam12

This comment has been minimized.

Show comment
Hide comment
@adam12

adam12 Mar 10, 2017

Contributor

Updated for new Tilt/Erubi versions and all tests are now passing.

I'm using it locally and it appears to work, outside of <%==. I've had to resort to <%= SafeBuffer.new("THE_STRING") %>. I'm pretty sure this is a bug but not sure where the fix has to be in order for it to be correct. Thoughts @ujifgc ?

Contributor

adam12 commented Mar 10, 2017

Updated for new Tilt/Erubi versions and all tests are now passing.

I'm using it locally and it appears to work, outside of <%==. I've had to resort to <%= SafeBuffer.new("THE_STRING") %>. I'm pretty sure this is a bug but not sure where the fix has to be in order for it to be correct. Thoughts @ujifgc ?

+ @src << " #{bufvar}.safe_concat '" << escape_text(text) << "';" unless text.empty?
+ end
+
+ def escape_text(text)

This comment has been minimized.

@ujifgc

ujifgc Mar 10, 2017

Member

The #escape_text definition was argued against by Jeremy because it's a local escaping of a local single quote.

https://github.com/jeremyevans/erubi/blob/cb3e65a209aea4a540acc7d7e3bc9b4554a87180/lib/erubi.rb#L170-L172

@ujifgc

ujifgc Mar 10, 2017

Member

The #escape_text definition was argued against by Jeremy because it's a local escaping of a local single quote.

https://github.com/jeremyevans/erubi/blob/cb3e65a209aea4a540acc7d7e3bc9b4554a87180/lib/erubi.rb#L170-L172

This comment has been minimized.

@adam12

adam12 Mar 10, 2017

Contributor

I'm assuming he did this to save the extra method call. I'll update.

@adam12

adam12 Mar 10, 2017

Contributor

I'm assuming he did this to save the extra method call. I'll update.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 10, 2017

Member

#add_expression_result and #add_expression_result_escaped are called for = and == tokens depending on @escape setting. One of the methods should escape the argument, the other should add it as is.

As we have SafeBuffer there is no need to call @escapefunc: SafeBuffer escapes the input (if needed) on #concat and assumes it's properly escaped (or safe) on #safe_concat.

I have no idea why it passes the tests but it's a bug that @escapefunc is called.

Member

ujifgc commented Mar 10, 2017

#add_expression_result and #add_expression_result_escaped are called for = and == tokens depending on @escape setting. One of the methods should escape the argument, the other should add it as is.

As we have SafeBuffer there is no need to call @escapefunc: SafeBuffer escapes the input (if needed) on #concat and assumes it's properly escaped (or safe) on #safe_concat.

I have no idea why it passes the tests but it's a bug that @escapefunc is called.

@adam12

This comment has been minimized.

Show comment
Hide comment
@adam12

adam12 Mar 10, 2017

Contributor

I believe this is ready. It would be great if some people could try it out.

@ujifgc Let me know if I need to change anything.

Contributor

adam12 commented Mar 10, 2017

I believe this is ready. It would be great if some people could try it out.

@ujifgc Let me know if I need to change anything.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 10, 2017

Member

Awesome!

Member

ujifgc commented Mar 10, 2017

Awesome!

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Mar 11, 2017

Member

Cool, I'm going to review this great changes.
Thank you!

Member

namusyaka commented Mar 11, 2017

Cool, I'm going to review this great changes.
Thank you!

@ujifgc ujifgc merged commit c9a31eb into padrino:master Mar 13, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 13, 2017

Member

Thanks @adam12!

Merged and fixed for testing legacy libs.

Member

ujifgc commented Mar 13, 2017

Thanks @adam12!

Merged and fixed for testing legacy libs.

@ujifgc ujifgc referenced this pull request Mar 13, 2017

Closed

Erubi Support #2089

@adam12

This comment has been minimized.

Show comment
Hide comment
@adam12

adam12 Mar 13, 2017

Contributor

Thanks :)

Contributor

adam12 commented Mar 13, 2017

Thanks :)

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Mar 14, 2017

Member

Awesome 👍

Member

namusyaka commented Mar 14, 2017

Awesome 👍

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