Rails 5.0.1 support #13302
Rails 5.0.1 support #13302
Conversation
| @@ -405,6 +405,9 @@ def load_schema! | |||
|
|
|||
| def define_virtual_attribute(name, cast_type, uses: nil, arel: nil) | |||
| attribute_types[name] = cast_type | |||
| if ActiveRecord::VERSION::MAJOR >= 5 | |||
| _default_attributes[name] = ActiveRecord::Attribute.uninitialized(name, cast_type) | |||
There was a problem hiding this comment.
This seems fine to me.. underscore notwithstanding, I think writing to _default_attributes is at a pretty similar level of internalness as attribute_types. (It bypasses the define_default_attribute private helper, but fundamentally, setting up those two hashes is what define_attribute does, and this method is intended to parallel that.)
No need for the version conditional, though -- unless it's fundamentally changed since I left it, this whole file is only expected to work on 5.0+.
|
ugh. looks like there are still a few errors here. This is going to be tricky |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
chrisarcand
force-pushed
the
rails-5-0-1-support
branch
2 times, most recently
from
fcf5dbd to
506b6e7
Compare
chrisarcand
force-pushed
the
rails-5-0-1-support
branch
2 times, most recently
from
317e111 to
74bf0af
Compare
chrisarcand
force-pushed
the
rails-5-0-1-support
branch
from
74bf0af to
f88fd0a
Compare
|
This pull request is not mergeable. Please rebase and repush. |
This reverts commit 475f1e9.
There's no reason ActionCable should be specified independently of Rails as railties locks them at exact versions. That is, actioncable is included and loaded in the stack regardless of being set here, and setting action cable like this actually forces the entire framework at 5.0.1 anyway.
This overwrites the default method for this by adding an additional check to the proc that is defined with the AttributeSet::Builder to check to see if the key should be provided a default from the `_default_attributes` hash to not only check to see if it is a column from the database, but also a virtual_attribute. The functionality added by: rails/rails@3e37a8b#diff-6c8fd220c92c2b25aa29891b59d00c04 was meant to help vanilla Rails' "virtual attributes" by assigning a default value on model instantiation. This is not what used to be done for any attributes (database or otherwise) besides the primary key for that model. What this does is includes `virtual_attributes` as attributes that aren't defined with a default value, along the database column attributes.
In the following: rails/rails#25146 Rails added a regression to making nested joins with references to those joins in the WHERE clause, and this causes an error when running this test suite in Rails 5.0.1. This has been addressed in Rails with: rails/rails#27598 And has been merged into master, but it is likely that Rails 5.0.1 will not be pulled from Rubygems, and the ManageIQ will fail to run tests properly with that version of Rails. Because of that, and to allow the tests to function in the same way, the simplest thing to do is to stringify the keys manually avoid the error for now. This is against the convention (most Rails devs prefer symbols), but this allows the tests to remain as they were.
chrisarcand
force-pushed
the
rails-5-0-1-support
branch
from
f88fd0a to
4e96ee4
Compare
|
Checked commits chrisarcand/manageiq@4a7b1ee~...4e96ee4 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 spec/models/manager_refresh/save_inventory/graph_of_inventory_collections_targeted_refresh_spec.rb
|
| # Display errors in both development.log and STDOUT | ||
| Rails.logger.warn(errstr) | ||
| warn(errstr) | ||
| end |
| @@ -386,6 +386,14 @@ def virtual_attribute_names | |||
| end | |||
| end | |||
|
|
|||
| def attributes_builder | |||
| @attributes_builder ||= ::ActiveRecord::AttributeSet::Builder.new(attribute_types, primary_key) do |name| | |||
| unless columns_hash.key?(name) || virtual_attribute?(name) | |||
There was a problem hiding this comment.
I would normally whine about unless with an || but since the original rails code has unless... I'm good with keeping it as close to the original code. I'll just say my brain still needs to convert this to an if.
There was a problem hiding this comment.
+1 to original code thought and even still I think it looks good 😄 If there were a negation in there it would be a whole different story.
Rails 5.0.1 support (cherry picked from commit fa1c0df)
|
Euwe backport details: |
The new patch release of Rails sadly introduced a large regression (even as a patch release) due to our use of Active Record internals for virtual attributes. These changes allow ManageIQ to run on the latest.
See individual commits for details.