Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jruby: output not properly converted back to erb #74

Closed
rthbound opened this issue Nov 28, 2012 · 35 comments
Closed

Jruby: output not properly converted back to erb #74

rthbound opened this issue Nov 28, 2012 · 35 comments

Comments

@rthbound
Copy link

Using jruby-head, deface outputs erb that is missing the leading <% or <%=...

This Gist shows the output (pasted from firefox)

Swapping to MRI resolves the problem (confirming it's a jruby thing)

@BDQ
Copy link
Member

BDQ commented Nov 30, 2012

That's interesting, I'll have a play with JRuby and see if I can see whats doing wrong, thanks for details.

@BDQ
Copy link
Member

BDQ commented Nov 30, 2012

In case you're interested, this is where it must be going wrong:

https://github.com/spree/deface/blob/master/lib/deface/parser.rb#L53

@benjamintanweihao
Copy link

+1. I'm also seeing this problem.

@BDQ
Copy link
Member

BDQ commented Dec 17, 2012

I'm installing jruby to take a look!

@BDQ
Copy link
Member

BDQ commented Jan 2, 2013

The problem comes from Nokogiri which apparently parsers / returns different output when run under jruby (as it uses a different java-based parser).

@benjamintanweihao
Copy link

Thanks for spending the time on this @BDQ . I'll see if it's possible to use the C ext version of Nokogiri on JRuby.

@BDQ
Copy link
Member

BDQ commented Jan 3, 2013

@benjamintanweihao - would you mind bumping to the latest master and doing some testing, I've made one small change that seems to fix the majority of failures.

I still need to change a lot of specs, as nokogiri on jruby doesn't guarantee the order of attributes (like it does on mri), so specs are failing as attrs are coming back in unexpected order.

@benjamintanweihao
Copy link

I've just tested it, still go no. Here's an example of the error I'm getting:

ActionView::Template::Error (/Users/rambo/.rvm/gems/jruby-1.7.1/gems/spree_core-1.1.4/app/views/spree/layouts/spree_application.html.erb:16: syntax error, unexpected tRPAREN

');@output_buffer.safe_concat(' ');@output_buffer.append= ( head) );@output_buffer.safe_concat('
^):
13: <%= stylesheet_link_tag "application", :media => "all" %>
14: <%= javascript_include_tag "application" %>
15: <%= csrf_meta_tags %>
16: <%= head) %>
17: <%= render :partial => 'spree/shared/head' %>

@BDQ
Copy link
Member

BDQ commented Jan 3, 2013

What version of Spree are you using?

On Thursday 3 January 2013 at 11:53, Benjamin Tan Wei Hao wrote:

I've just tested it, still go no. Here's an example of the error I'm getting:
ActionView::Template::Error (/Users/rambo/.rvm/gems/jruby-1.7.1/gems/spree_core-1.1.4/app/views/spree/layouts/spree_application.html.erb:16: syntax error, unexpected tRPAREN
');@output_buffer.safe_concat(' ');@output_buffer.append= ( head) );@output_buffer.safe_concat('
^):
13: <%= stylesheet_link_tag "application", :media => "all" %>
14: <%= javascript_include_tag "application" %>
15: <%= csrf_meta_tags %>
16: <%= head) %>
17: <%= render :partial => 'spree/shared/head' %>


Reply to this email directly or view it on GitHub (#74 (comment)).

@benjamintanweihao
Copy link

Spree 1.1.4.

@BDQ
Copy link
Member

BDQ commented Jan 3, 2013

Do you have a custom override adding that head method? I don't see that in stock 1.1.4?

@benjamintanweihao
Copy link

You are right. I removed the head, but the problem manifests itself in other places.

SyntaxError in Spree/products#show

Showing /Users/rambo/.rvm/gems/jruby-1.7.1/gems/spree_core-1.1.4/app/views/spree/products/_image.html.erb where line #2 raised:

/Users/rambo/.rvm/gems/jruby-1.7.1/gems/spree_core-1.1.4/app/views/spree/products/_image.html.erb:2: syntax error, unexpected tASSOC

Here's the code for _image.html.erb:

<% if image %> 
  <%= image_tag image.attachment.url(:product), :itemprop => "image" %>
<% else %>
  <%= product_image(@product, :itemprop => "image") %>
<% end %>

And the error message:

@output_buffer.safe_concat('  ');@output_buffer.append= ( image_tag product), :itemprop => "image" );@output_buffer.safe_concat('
                                                                                         ^
Extracted source (around line #2):

1: <% if image %> 
2:   <%= image_tag product), :itemprop => "image" %>
3: <% else %>
4:   <%= link_to(product_image(@product, :itemprop => "image"), "#") %>
5: <% end %>
Trace of template inclusion: /Users/rambo/.rvm/gems/jruby-1.7.1/gems/spree_core-1.1.4/app/views/spree/products/_image.html.erb, /Users/rambo/.rvm/gems/jruby-1.7.1/gems/spree_core-1.1.4/app/views/spree/products/show.html.erb

@BDQ
Copy link
Member

BDQ commented Jan 7, 2013

Ok, cool this is something I can work with. I'll report back shortly.

@BDQ
Copy link
Member

BDQ commented Jan 10, 2013

@benjamintanweihao I'm having trouble replicating this, can you give me the override[s] you're applying to spree/products/_image

@swrobel
Copy link

swrobel commented Feb 20, 2013

Spree 1.3.2 stock doesn't work on jruby 1.7.2 because of deface. Is this being worked on?

@BDQ
Copy link
Member

BDQ commented Feb 21, 2013

@swrobel - yes I do intend fixing this, just haven't gotten around to it yet.

The problem stems from the fact the Nokogiri uses a completely different parser when running on jruby, and it doesn't behave the same as libxml on MRI. So there's lot of the internals that need to be made aware of the differences.

There's a couple of fixes already on master that you could point your Gemfile at, and if you could find an example of the failure (using just core code + a sample override) that would help me a lot?

@BDQ
Copy link
Member

BDQ commented Mar 23, 2013

It's taken 4 months, but I think I've got jruby working. I've testing both 1.6.2 and 1.7.3 in both 1.8 and 1.9 modes and the specs are passing for me.

Anyone still interested can use gem 1.0.0.rc2 to test, and please report back.

@BDQ BDQ closed this as completed Mar 23, 2013
@benjamintanweihao
Copy link

Thank you! Appreciate the hard work.

@rthbound
Copy link
Author

Thanks alot @BDQ !

@swrobel
Copy link

swrobel commented Apr 9, 2013

@BDQ have you tried running spree on jruby? I still can't get it to work.

@BDQ
Copy link
Member

BDQ commented Apr 9, 2013

@swrobel, yes it's down to one issue, can you do a gist of the exception you're seeing so I can confirm it's the same as me locally?

@BDQ BDQ reopened this Apr 9, 2013
@swrobel
Copy link

swrobel commented Apr 9, 2013

With stock spree 1.3.2 app using sample data https://gist.github.com/swrobel/711843d313918c5e4591 which is essentially the same as what's reported on spree/spree#2748

With my views (I'm not using any deface, but spree still seems to try) I'm essentially getting the same behavior as spree/spree#2633

spree_application.html.erb: https://gist.github.com/swrobel/f95ab76ce605a6486821
Output: https://gist.github.com/swrobel/9a8837afecfa33b6254b

LMK if you need more!

@swrobel
Copy link

swrobel commented Apr 9, 2013

For kicks I tried removing the data-hooks from my layout, but it still closes head & opens body in the same place.

@BDQ
Copy link
Member

BDQ commented Apr 10, 2013

@swrobel thanks for all the feedback, so you are seeing two issues now:

  1. Seen in this gist: https://gist.github.com/swrobel/711843d313918c5e4591 , where now nokogiri under java is not reverting escaped double quotes within the erb block, so the erb is left with junk like: %22sixteen%22 . This is the same problem I'm now seeing locally.

  2. Did you try the libxml downgrade to see the Weird templating behaviour spree#2633 is still a problem on Jruby?

@swrobel
Copy link

swrobel commented Apr 10, 2013

Regarding #2, I don't believe nokogiri for jruby depends on libxml, although I never upgraded so my MRI nokogiri depends on 2.7.8 and I've never seen the issue on it.

→ nokogiri -v
# Nokogiri (1.5.9)
    ---
    warnings: []
    nokogiri: 1.5.9
    ruby:
      version: 1.9.3
      platform: java
      description: jruby 1.7.3 (1.9.3p385) 2013-02-21 dac429b on Java HotSpot(TM) 64-Bit Server VM 1.6.0_43-b01-447-11M4203 [darwin-x86_64]
      engine: jruby
      jruby: 1.7.3
    xerces: Xerces-J 2.9.0
    nekohtml: NekoHTML 1.9.12

@BDQ
Copy link
Member

BDQ commented Apr 11, 2013

Ofcourse you're correct libxml will have no effect under Jruby, odd the same issue appears with two separate parsing libraries.

@zjuric
Copy link

zjuric commented Apr 24, 2013

Is there any progress regarding this issue?

@BDQ
Copy link
Member

BDQ commented May 2, 2013

So I've pushed another fix for this issue and confirmed Spree is working with JRuby + Deface....proof: http://d.pr/i/xlzv

I used this branch of spree/spree: https://github.com/spree/spree/tree/jruby and updated the Gemfile to use spree/deface master

Anyone else care to test please?

@BDQ BDQ closed this as completed May 2, 2013
@swrobel
Copy link

swrobel commented May 2, 2013

@BDQ hate to be the bearer of bad news, but I'm still seeing this issue in both my application and stock spree 1-3-stable spree/spree#2633

Try running the html through http://validator.w3.org/ an you'll see it go nuts...

@parndt
Copy link

parndt commented May 2, 2013

@swrobel did you use the jruby branch mentioned by @BDQ?

@swrobel
Copy link

swrobel commented May 2, 2013

@parndt no but I looked at the commit and it's really no different than what I already had in my Gemfile. I just updated my Gemfile to use deface/master

@BDQ
Copy link
Member

BDQ commented May 2, 2013

@swrobel spree/spree#2633 is a different issue, this issue was erb tags being escaped incorrectly by jruby, which was causing exceptions preventing any page from rendering.

Can you confirm your pages are indeed rendering, and it;s just the head / body tags that are messed up?

@BDQ
Copy link
Member

BDQ commented May 2, 2013

@swrobel - Can you open a new ticket on deface to replicate 2633 for jruby. There's too many issues listed in the comments on this ones.

@swrobel
Copy link

swrobel commented May 2, 2013

I can confirm with both stock spree & my app that views are now rending. This is an improvement from before.

Issue for the </head> tag placement opened here: #84

@BDQ
Copy link
Member

BDQ commented May 3, 2013

@swrobel great, thanks for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants