Don't pollute Controllers with conflicting instance variables

[paulspringett]

The store_current_location! method is using a @page instance variable that pollutes across all controllers, regardless if they're anything to do with Refinery or not.

def store_current_location!
  return super if admin?

  session[:website_return_to] = refinery.url_for(@page.url) if @page && @page.persisted?
end

Something like @page is arguably a common variable name and shouldn't be defined by Refinery. I would suggest using something like @_refinery_page to avoid conflicts. Would this be a big change? If it's realistic I'll fork, patch and submit a pull request.

Alternatively, would it be possible to scope this variable definition to the Refinery controllers only?

Thanks,

Paul

[parndt]

This would be a huge change. However, I think that we should just get rid of store_current_location! and the functionality it's used for.

[paulspringett]

Could you scope store_current_location! to only run in Refinery's controllers? I presume you're adding it into the ApplicationController? Does Refinery have a BaseController or similar? Could it be added to that?

[paulspringett]

Would Refinery::AdminController do?

[parndt]

If it's only in the Admin then the functionality makes no sense.

[paulspringett]

That makes sense.

So is this before filter actually needed? If I submit a pull request removing it would this be something supported by the Refinery team?

[parndt]

To master branch, yes, I think so.
But please remove all related code too (anything that uses the location that gets stored, for example).

[paulspringett]

Ok thanks

[parndt]

There may be tests that use this functionality too, but I guess they'll fail if the functionality disappears so it should be obvious :-)

Thanks!! :)

[parndt]

Fixed by a06bdaa and 6ec5141 thanks to @aayalur

[paulspringett]

@parndt Sorry for not getting this done, I couldn't get the tests to run which was a sticking point. For future reference, are there any docs for this? I tried the usual rake spec but no tests ran, just some output about a "dummy app"?

[parndt]

Hi @paulspringett no problem!
There is in fact a guide for this which runs you through the process. Because Refinery is isolated from Rails it has to run a dummy app which is a basic Rails installation.

[paulspringett]

@parndt that's what I was looking for! Thanks.