hanami / view

Views, templates and presenters for Ruby web applications
http://hanamirb.org
MIT License
173 stars 80 forks source link

Undefined method `locals' #153

Closed skull-squadron closed 6 years ago

skull-squadron commented 6 years ago

Maybe I'm holding it wrong, but it looks like an API changed or something didn't load properly. Suggestions or is this a bug?

MCE

gem install hanami -v 1.2.0
gem install haml -v 5.0.4
hanami new Bookstore --database=postgres --test=rspec --template=haml
HANAMI_ENV=test bundle exec hanami db create
rake spec

Output

Failures:

  1) Web::Views::ApplicationLayout contains application name
     Failure/Error: let(:rendered) { layout.render }

     NoMethodError:
       undefined method `locals' for #<Hanami::View::Template:0x00007fd3ce6adfc0>
     # ~/.gem/ruby/2.5.1/gems/hanami-view-1.2.0/lib/hanami/view/rendering/layout_scope.rb:233:in `method_missing'
     # ~/.gem/ruby/2.5.1/gems/tilt-2.0.8/lib/tilt/haml.rb:23:in `evaluate'
     # ~/.gem/ruby/2.5.1/gems/tilt-2.0.8/lib/tilt/template.rb:109:in `render'
     # ~/.gem/ruby/2.5.1/gems/hanami-view-1.2.0/lib/hanami/view/template.rb:41:in `render'
     # ~/.gem/ruby/2.5.1/gems/hanami-view-1.2.0/lib/hanami/layout.rb:139:in `render'
     # ./spec/web/views/application_layout_spec.rb:5:in `block (2 levels) in <top (required)>'
     # ./spec/web/views/application_layout_spec.rb:9:in `block (2 levels) in <top (required)>'

Finished in 0.00574 seconds (files took 2.33 seconds to load)
1 example, 1 failure

Versions

Gem Versions

haml (5.0.4) haml-lint (0.999.999) haml-rails (1.0.0) haml_lint (0.28.0) hanami (1.2.0) hanami-assets (1.2.0) hanami-cli (0.2.0) hanami-controller (1.2.0) hanami-helpers (1.2.2) hanami-mailer (1.2.0) hanami-model (1.2.0) hanami-router (1.2.0) hanami-utils (1.2.0) hanami-validations (1.2.2) hanami-view (1.2.0) hanami-webconsole (0.1.0) more...

AlfonsoUceda commented 6 years ago

@steakknife yes, it is a bug because this doesn't happen with erb neither slim, I'll try to check what is happening, thanks for the issue :)

skull-squadron commented 6 years ago

No prob. Shoot the messenger now or later? ;)

PS: I'm gonna have to unshift to my Ruby features wish-list: "protocols"/"interfaces" and gradual-typing (academic paper & demo here) to catch more categories of bugs instead of being caught by the meatcloud (of which I too sport a lighted membership logo like a Lyft driver).

Best of luck... I'm compiling nginx for the n-th time today. ccache to the rescue! :) :v:

jodosha commented 6 years ago

@AlfonsoUceda It looks like #140 introduced this problem. Specifically this line:

https://github.com/hanami/view/pull/140/files#diff-5d90b9ac33da24fadae41a69b3800763R233


If I try to reproduce the problem by using a patched version of hanami-view, it works:

diff --git a/lib/hanami/view/rendering/layout_scope.rb b/lib/hanami/view/rendering/layout_scope.rb
index 5c5fca3..72a79d9 100644
--- a/lib/hanami/view/rendering/layout_scope.rb
+++ b/lib/hanami/view/rendering/layout_scope.rb
@@ -230,7 +230,7 @@ module Hanami
           # that we want to be frozen in the future
           #
           # See https://github.com/hanami/view/issues/130#issuecomment-319326236
-          if @scope.respond_to?(m, true) && @scope.locals.has_key?(m) && layout.respond_to?(m, true)
+          if @scope.respond_to?(m, true) # && @scope.locals.has_key?(m) && layout.respond_to?(m, true)
             layout.__send__(m, *args, &blk)
           elsif @scope.respond_to?(m, true)
             @scope.__send__(m, *args, &blk)

Would you mind to have a look? Thanks.

skull-squadron commented 6 years ago

@jodosha : @AlfonsoUceda 's works.. hopefully there are regression tests already for #130

Haml::TempleEngine: Option :disable_escape is invalid
Haml::TempleEngine: Option :disable_escape is invalid
.

Finished in 0.01375 seconds (files took 3.14 seconds to load)
1 example, 0 failures

I tried to slip a binding.pry at the beginning of that method, but it stack overflowed, probably some sort of pry inspection amplification circular-dependency.

If backwards compat were needed:

          if @scope.respond_to?(m, true) &&
             @scope.respond_to?(:locals) && @scope.locals.key?(m) &&
             layout.respond_to?(m, true)

Otherwise LGTM.

jodosha commented 6 years ago

@steakknife Thanks for getting back.

The reason why PRY failed is due to the behavior of BasicObject. Ruby debuggers expect that Kernel is mixed in to provide inspection facilities. This is true for all the objects that inherit from Object, but not for the ones that inherit from BasicObject.


We need to fix this in the next patch release, so backward compatibility must to be kept, because of SemVer. The fix that @AlfonsoUceda wrote (#140 ) is great, we need to amend it with a safer logic.


I'm gonna write a patch for this.

jodosha commented 6 years ago

@steakknife @AlfonsoUceda I opened a PR to fix the problem. Please read carefully the description, which gives the context for the code change. Thanks.