Update for RefineryCMS 2.0.0 #56

Merged
merged 9 commits into from Feb 3, 2012

Conversation

Projects
None yet
5 participants
@robyurkowski
Contributor

robyurkowski commented Feb 3, 2012

To be perfectly clear, this is not ready to be pulled yet (though it is very close). I mucked something up or didn't namespace the InquirySettings yet, so you can't change who is notified. But I wanted to open this pull request initially so someone else doesn't go gallivanting off, tilting at windmills like I have here.

I'm hoping to fix the rest of this real quick tomorrow morning.

@robyurkowski

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Feb 3, 2012

Contributor

Okay. This should be ready to go.

Contributor

robyurkowski commented Feb 3, 2012

Okay. This should be ready to go.

- unless ::Refinery::Inquiry.table_exists?
- create_table ::Refinery::Inquiry.table_name, :force => true do |t|
+ unless ::Refinery::Inquiries::Inquiry.table_exists?
+ create_table :refinery_inquiries_inquiries, :force => true do |t|

This comment has been minimized.

@ugisozols

ugisozols Feb 3, 2012

Member

We failed to think through this namespacing stuff because it just doesn't make sense to have table named refinery_inquiries_inquiries.

@ugisozols

ugisozols Feb 3, 2012

Member

We failed to think through this namespacing stuff because it just doesn't make sense to have table named refinery_inquiries_inquiries.

This comment has been minimized.

@robyurkowski

robyurkowski Feb 3, 2012

Contributor

Actually, this is a bad example to look at. It's a lot more poignant and helpful when you get into things like the portfolio -- then you have a table like :refinery_portfolio_entries.

@robyurkowski

robyurkowski Feb 3, 2012

Contributor

Actually, this is a bad example to look at. It's a lot more poignant and helpful when you get into things like the portfolio -- then you have a table like :refinery_portfolio_entries.

This comment has been minimized.

@ugisozols

ugisozols Feb 3, 2012

Member

Yeah it totally makes sense when engine has more than one model but imho we need to come with solution when there's only one model.

@ugisozols

ugisozols Feb 3, 2012

Member

Yeah it totally makes sense when engine has more than one model but imho we need to come with solution when there's only one model.

This comment has been minimized.

@robyurkowski

robyurkowski Feb 3, 2012

Contributor

IMO, better to be consistent -- if you have to inspect the database, you should be able to see refinery__. Keep in mind that in this engine, we could have called Inquiries simply Contacts. It's only by basis of the name of the resource that it looks funny.

@robyurkowski

robyurkowski Feb 3, 2012

Contributor

IMO, better to be consistent -- if you have to inspect the database, you should be able to see refinery__. Keep in mind that in this engine, we could have called Inquiries simply Contacts. It's only by basis of the name of the resource that it looks funny.

@ugisozols

This comment has been minimized.

Show comment
Hide comment
@ugisozols

ugisozols Feb 3, 2012

Member

Great work @robyurkowski 💗

Member

ugisozols commented Feb 3, 2012

Great work @robyurkowski 💗

ugisozols added a commit that referenced this pull request Feb 3, 2012

@ugisozols ugisozols merged commit 280db3e into refinery:rails-3-1 Feb 3, 2012

@dmoose

This comment has been minimized.

Show comment
Hide comment
@dmoose

dmoose Feb 21, 2012

This commit breaks existing code without warning since it replaces the migration with a new one with a different table name but the migration name remains the same so it will not even replace the existing migration.

There should be more thought given to this. At least some sort of warning, but it would seem to make a lot more sense to continue to use the old table name.

dmoose commented on a971e7d Feb 21, 2012

This commit breaks existing code without warning since it replaces the migration with a new one with a different table name but the migration name remains the same so it will not even replace the existing migration.

There should be more thought given to this. At least some sort of warning, but it would seem to make a lot more sense to continue to use the old table name.

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Feb 21, 2012

Contributor

I understand your frustration, but before the release of Refinery 2.0, we are trimming migrations all over the code-base, since it is not considered backward compatible, and now is the only legitimate time to clean up the migrations. I'm sorry if it's caused issues with any existing sites you have, but the unstable branch is somewhat unstable.

We're using the new table name because it is standard with the new namespacing format. It seems odd, perhaps, that it should be named refinery_inquiries_inquiries, but it follows the general pattern of refinery_<engine>_<model>. People have suggested renaming the Inquiry model to something else like Contact, and I believe @parndt and @ugisozols have both expressed a willingness to merge a pull request with this change.

Contributor

robyurkowski replied Feb 21, 2012

I understand your frustration, but before the release of Refinery 2.0, we are trimming migrations all over the code-base, since it is not considered backward compatible, and now is the only legitimate time to clean up the migrations. I'm sorry if it's caused issues with any existing sites you have, but the unstable branch is somewhat unstable.

We're using the new table name because it is standard with the new namespacing format. It seems odd, perhaps, that it should be named refinery_inquiries_inquiries, but it follows the general pattern of refinery_<engine>_<model>. People have suggested renaming the Inquiry model to something else like Contact, and I believe @parndt and @ugisozols have both expressed a willingness to merge a pull request with this change.

This comment has been minimized.

Show comment
Hide comment
@soupmatt

soupmatt Feb 21, 2012

Do you guys have any plans for how the migration changes will be handled for those upgrading from refinery 1.0? I've been using edge, so I expect that things will break unexpectedly, but others could be taken by surprise by this once 2.0 is released.

Do you guys have any plans for how the migration changes will be handled for those upgrading from refinery 1.0? I've been using edge, so I expect that things will break unexpectedly, but others could be taken by surprise by this once 2.0 is released.

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Feb 21, 2012

Member
Member

parndt replied Feb 21, 2012

This comment has been minimized.

Show comment
Hide comment
@dmoose

dmoose Feb 22, 2012

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Feb 22, 2012

Contributor

Please feel free to contribute to the changelog. Nobody adds anything to it because nobody knows it's there (something I plan on rectifying).

I agree in principle that we need a way to update between the two versions. I think maybe in this mode, @parndt, we should designate 2.0 as a transitionary release for migrations—just generate new ones that rename tables as dmoose suggests, and issue a warning—and hold off the clean-up to 2.1 or even 3.0. I'd like it clean for 2.0, but if we do want to give people a way to update their apps, this is somewhat necessary, I think.

Contributor

robyurkowski replied Feb 22, 2012

Please feel free to contribute to the changelog. Nobody adds anything to it because nobody knows it's there (something I plan on rectifying).

I agree in principle that we need a way to update between the two versions. I think maybe in this mode, @parndt, we should designate 2.0 as a transitionary release for migrations—just generate new ones that rename tables as dmoose suggests, and issue a warning—and hold off the clean-up to 2.1 or even 3.0. I'd like it clean for 2.0, but if we do want to give people a way to update their apps, this is somewhat necessary, I think.

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Feb 22, 2012

Member
Member

parndt replied Feb 22, 2012

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Feb 22, 2012

Contributor

My personal inclination is that there's too little time between a 1.1 release and a 2.0 release to bother. If we keep migrations messy — add a few more in, even, to manage the transition — then we don't need to force users to update twice to get to 2.0.

Contributor

robyurkowski replied Feb 22, 2012

My personal inclination is that there's too little time between a 1.1 release and a 2.0 release to bother. If we keep migrations messy — add a few more in, even, to manage the transition — then we don't need to force users to update twice to get to 2.0.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Feb 22, 2012

Member

@robyurkowski this should just be called SettingsController now

@robyurkowski this should just be called SettingsController now

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Feb 22, 2012

Contributor

Sheesh, Phil. You're only 19 days late.

Contributor

robyurkowski replied Feb 22, 2012

Sheesh, Phil. You're only 19 days late.

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