Core Dev: Page Controller and Page Editor refactorings

Hi,

I’ve started to do some refactoring/improvment of the strucutre of
page, page_controller, and page_parts. And I plan on continuing in a
similar vein with admin/page/edit.rhtml

The goal of this particular iteration, is to make it possible for Page
subclasses or extensions of the Page class to add has_many,
belongs_to, or has_one etc… relations to Page and NOT have to modify
page_controller. Since PageParts is a has_many of Page, I have moved
all of the logic related to the saving of page parts into an
association extension. Page is setup to discover all association
extensions that respond to :update_association and pass parameters to
and save these association appropriately.

http://dev.radiantcms.org/radiant/attachment/ticket/508/page_controller_refactor.diff
Please give me feedback on the direction I’m going!

thanks,
Jacob

Jacob,

Looks like a good step in the right direction. I like your use of
association extensions. However, wouldn’t it just be easier to do:

Page.new(params[:page])

or

@page.update_attributes(params[:page])

and not have to call update_associations?

Generally the strategy if you take this route is to make an
attr(_writer/_accessor) with the desired name, then an after_save
callback that saves the associated models.

Sean

The equivalent of what you’ve written is currently performed by
abstract_model_controller in handle_new_or_edit_post

as:

model.attributes = params[model_symbol]

which for the case of page is essentially:

@page.attributes = params[:page]

The problem with hooking in to this behavior is that in order to save
the page parts we need to look at:

params[:parts]

Supporting your suggestion would require much more broad sweeping
changes.

Supporting your suggestion would require much more broad sweeping changes.

That is part of the point, though, isn’t it? We should refactor the
views as well so they are more model-oriented. I’d love to be able to
change the routes file to look something like this:

map.with_options :path_prefix => “admin/” do |admin|
admin.resources :pages, :snippets, :layouts, :users, :extensions
end

u.s.w.

Maybe that’s a bit too far down the road, but it’s the direction we
should be going.

Sean

Actually, when you use map.resources, the named route for the index
action would be ‘extensions_url’ (with no parameters).

Sean

change the routes file to look something like this:

map.with_options :path_prefix => “admin/” do |admin|
admin.resources :pages, :snippets, :layouts, :users, :extensions
end

I tried this for the extensions controller, seems it should be simple
enough right?

commented out the extensions routes and added:

map.with_options :path_prefix => “admin/” do |admin|
admin.resources :extensions
end

and I get:

undefined local variable or method `extension_index_url’ for
#<#Class:0x222df3c:0x222df14>

It seems in order to be able use named routes, we have to name each
route explicitly:

It seems that the line:
extension.extension_index ‘admin/extensions’, :action => ‘index’

is what makes “extension_index_url” come to life

and

extension.extension_indexwithanothername  'admin/extensions',

:action => ‘index’

would make “extension_indexwithanothername_url” come to life.

Thoughts on how to get around this?