Upgrade to Rails 4.1 #244
Upgrade to Rails 4.1 #244
Conversation
|
Tests pass! The site has not yet spontaneously broken (… since I fixed tests) while I use it in debug! I'll go poke around a bit more to figure out if it's actually sufficient. Edit: Poked around a bit more and yep! Stuff is not obviously broken. |
This file is explicitly mentioned in the 4.0 -> 4.1 upgrade and it says if you want to use the new JSON format to set :hybrid here, to transparently migrate cookies; I'm not sure if this file was just ignored before, and we weren't using JSON, or what? If I run the server without this change it crashes when I try loading the site, and with this change it works.
(Fix recognizing galleries_icons relation with protected attributes)
Throne3d
force-pushed
the
upgrade/rails-4.1
branch
from
25c6bd0
to
18134a5
Compare
|
So this should probably be poked at a bit more manually before we go pushing it. I guess. But tests do seem to pass, and so I'm considering it no longer a WIP. (Annoyingly, the upgrade docs hardly mentioned these things, if it did at all. Oh well.) |
| @@ -10,7 +10,7 @@ gem 'browser' | |||
| gem 'coffee-rails' | |||
| gem 'exception_notification' | |||
| gem 'gon' | |||
| gem 'haml-rails', '~> 0.4.0' | |||
| gem 'haml-rails' | |||
There was a problem hiding this comment.
Did we have a specific HAML version for any particular reason?
There was a problem hiding this comment.
| @@ -107,7 +107,7 @@ def posts_from_relation(relation, no_tests=true, with_pagination=true) | |||
| posts = posts.paginate(page: page, per_page: 25) if with_pagination | |||
| posts = posts.no_tests if no_tests | |||
|
|
|||
| if (with_pagination && posts.total_pages <= 1) || posts.count <= 25 | |||
| if (with_pagination && posts.total_pages <= 1) || posts.count(:all) <= 25 | |||
There was a problem hiding this comment.
Otherwise it calls COUNT(posts.*, board.name AS board_name, …) which triggers a syntax error. See rails/rails#10710
|
Quick note from email where I can see exactly zero code: do we want to get
to strict params 100% before we do 4.1? Or do you prefer this first?
(You're doing the work, you can pick xD <3)
…On Wed, May 24, 2017 at 2:46 PM, Edward Jones ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/controllers/application_controller.rb
<#244 (comment)>:
> @@ -107,7 +107,7 @@ def posts_from_relation(relation, no_tests=true, with_pagination=true)
posts = posts.paginate(page: page, per_page: 25) if with_pagination
posts = posts.no_tests if no_tests
- if (with_pagination && posts.total_pages <= 1) || posts.count <= 25
+ if (with_pagination && posts.total_pages <= 1) || posts.count(:all) <= 25
Otherwise it calls COUNT(posts.*, board.name AS board_name, …) which
triggers a syntax error. See rails/rails#10710
<rails/rails#10710>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#244 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAs60vw076X55ED9Ab-Ye6D82eQwkhZAks5r9Hr3gaJpZM4Nlb9->
.
|
|
Hmm, I think the warning about 4.0 not being properly supported bug me more than the lack of "proper" strong params. |
| @@ -1,3 +1,3 @@ | |||
| # Be sure to restart your server when you modify this file. | |||
|
|
|||
| Rails.application.config.action_dispatch.cookies_serializer = :json | |||
| Rails.application.config.action_dispatch.cookies_serializer = :hybrid | |||
There was a problem hiding this comment.
For my personal edification, whyfor?
There was a problem hiding this comment.
I think this commit message answers why? The details are basically "I'm not sure why this file existed before, I don't think it applied, this should be better?
"906de238867e9806c7d894d16ccc23937bd8fcd5
There was a problem hiding this comment.
Ah, didn't see the commit message, thanks!
I expect it to take a bit before we actually merge this, seeing as we only recently got to Rails 4.0, but thought I'd make some progress on it anyway.