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

Error in the old 1.12 version #79

Open
JohnSmall opened this issue Sep 26, 2018 · 6 comments
Open

Error in the old 1.12 version #79

JohnSmall opened this issue Sep 26, 2018 · 6 comments

Comments

@JohnSmall
Copy link

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
Copy link
Member

GBH commented Sep 26, 2018

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
Copy link
Author

OK, I'll dig into it.

@JohnSmall
Copy link
Author

JohnSmall commented Sep 27, 2018

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
Copy link
Member

GBH commented Sep 27, 2018

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
Copy link
Author

OK thanks

JohnSmall pushed a commit to JohnSmall/comfy-blog that referenced this issue Oct 1, 2018
@JohnSmall
Copy link
Author

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

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

No branches or pull requests

2 participants