comfy / comfy-blog

Blog Engine for ComfortableMexicanSofa (Rails 5.2+)
MIT License
106 stars 86 forks source link

Error in the old 1.12 version #79

Open JohnSmall opened 6 years ago

JohnSmall commented 6 years ago

I have to use the old 1.12 version because the new one requires Comfy >=2.0.0 which requires Rails >= 5.2, which I don't have time to upgrade to. I'm on Rails 5.0.2

I assumed the old version 1.12 of comfy blog would work with Comfy 1.12, but it doesn't

Expected behavior

When creating a new blog I should not get dumped into an error page

Actual behavior

  ArgumentError in Comfy::Admin::Blog::Blogs#new
 gems/comfy_blog-1.12.3/app/views/comfy/admin/blog/blogs/_form.html.haml where line #5 raised
wrong number of arguments (given 0, expected 1)
# List of available application layouts
  def self.app_layouts_for_select(view_paths)
    view_paths.map(&:to_s).select { |path| path.start_with?(Rails.root.to_s) }.flat_map do |full_path|
      Dir.glob("#{full_path}/layouts/**/*.html.*").collect do |filename|
        filename.gsub!("#{full_path}/layouts/", '')

Steps to reproduce

  1. Install Comfy blog 1.12 in a working Comfy 1.12 system
  2. Generate and run migrations
  3. Go to the admin page and click on 'Blogs' in the left menu
  4. Click the 'New Blog' button. You get dumped into the error page

System configuration

Rails version: 5.0.2 CMS version: 1.12 Ruby version: 2.4.0

GBH commented 6 years ago

Yeah. It's possible that something got out of sync in 1.12. If you can fix the issue and make a PR I'll release 1.12.4. Thanks!

JohnSmall commented 6 years ago

OK, I'll dig into it.

JohnSmall commented 6 years ago

There's quite a lot of errors in this version. The test suite only finds the first 4 errors, all related to this issue. Was there a time when the test suite ran without errors?

Sample of one of the errors when running the suite

ActionView::Template::Error: wrong number of arguments (given 0, expected 1)
comfortable_mexican_sofa (1.12.11) app/models/comfy/cms/layout.rb:48:in `app_layouts_for_select'

Seems it's looking for something in the main comfy app but it's not there.

Line 5 in the admin/blog/blogs/_form.html.haml is the problem

 - if (options = Comfy::Cms::Layout.app_layouts_for_select).present?

I fixed this one by changing it to

 - if (options = Comfy::Cms::Layout.app_layouts_for_select(lookup_context.view_paths)).present?

That gets me past the admin problem. But then I have other issues.

  1. the path to the blog has two forward slashes in it my-site//blog/etc
  2. the path a blog post isn't relative to the site, so the browser sees it as a new host, which it can't find e.g.

    <a href="//blog/choice-blog/this-is-the-first-blog-post">This is the first blog post</a>

Note the //blog which makes the browser think it's a host not a relative path name

Both these errors are related to the // before the path to the blogs. But I can't see where that's coming from.

There must be a missing test or this error would not have crept in.

GBH commented 6 years ago

I'm going to take a look a bit later. Pretty sure // problem was fixed long time ago. Maybe only for 2.0 though.

JohnSmall commented 6 years ago

OK thanks

JohnSmall commented 6 years ago

I've just sent a PR to fix this problem. But not the //