Add Images to Engines by Config

[jgwmaxwell]

Hi,

Would you welcome a pull request for a modification to Page Images that allows you to set the engines/models you want to have page engines by an initializer or config option?

I've forked a copy and keep adding bits to it as I want things to have images associated with them, which made me think that doing it by initializer would be perfect.

Any thoughts?

Cheers,

John

[parndt]

@jgwmaxwell I'd definitely welcome a more generic approach to implementing this extension! Number 1 request is always "Can I add this to X model?"

[jgwmaxwell]

Perfect, I've got it open now so will get on with it. Do you have a preferred way of me doing it? Hard code the attachment to Page and Blog, and then use initializer to add any other options?

[parndt]

If you wouldn't mind rewiring it so that it could be completely generic…. that'd be the ultimate!
It's really based on how much work you feel like getting yourself into ;-)

What do you think?

Thanks!

[jgwmaxwell]

I don't think there is too much difference between configuring some options generically and configuring them all generically in terms of work? Since this is essentially Rails - we can be opinionated and keep DHH happy by setting a default config options for Pages/Blog to be included. That would also help with upgrading users, as its less likely to be forgotten when the gem upgrades.

Happy to help out.

[parndt]

I like your style

[jgwmaxwell]

Wait for the mangled code and lousy tests ;)

[parndt]

With bated breath!

[jgwmaxwell]

@parndt Ok, it's well past my bedtime, but I've hashed together functionality that works, and it's fully generic. As it's late at night, and I wanted a proof of concept, this is completely untested - I'll start again with a TDD implementation tomorrow.

I've added a config var called "mountings", which is an array of Strings representing the model that the image is to be mounted to. This is set by default to

self.mountings = ["Refinery::Page", "Refinery::Blog::Post"]

In the initializer that Page Images already creates you can just add the custom engines/models.

config.mountings << "Refinery::MyGreatEngine::MyModel"

And this will set up the association and the tabs. I'll fix all the holes tomorrow, but what do you think of it as the basis for an implementation?

https://github.com/cloudhaven/refinerycms-page-images/tree/configured

It's in a new "configured" branch.

[parndt]

@jgwmaxwell very promising!

I don't like all the "try" code (I think things should actually fail on the user if they mount incorrectly) and I don't like the case statement hack (but probably neither do you!)

Excited to see the TDD implementation!

Thanks!

For others: cloudhaven/refinerycms-page-images@master...configured

[jgwmaxwell]

Ha! I agree about stuff failing if people are daft enough to stick in options that don't exist, but then thought maybe being nice is better so put the tries in after all. The case statement makes me feel dirty inside, but I just wanted to check the idea worked.

What do you think about passing in the string representations of the models?

[parndt]

Passing in strings is OK, what happens if you pass in the actual class
references? My main worry is that it will make it evaluate the model
class too early which has implications on asset precompilation etc.

Rather than the case statement could people tell you the location of
their tab code etc? Or would you prefer truly convention over
configuration?

Guessing where stuff is is okay provided that it's a convention and is
implemented elegantly I think. And the user can find out when they're
wrong easily.

[jgwmaxwell]

The model has to be evaluated by the time that

Refinery::ModelName.send :has_many_page_images

is called on it, I'm not sure that it makes a difference, both routes are going to hit the model at that point - arguably the strings keep the model out of it for longer? Wouldn't passing the class references require them to be available at that moment, rather than when we use them?

Sure, you could pass in an array of parameters to tell model and tab location, which would be a lot better than guessing them in 99% of situations.

config.mountings << ["Refinery::Blog::Post", "Refinery::Blog::Tab"]
config.mountings << "Refinery::Blog::Post" #this one could be guessed since it isn't declared

I was mainly thinking that this is such a simple Engine - one config option, one choice of what it does - that it would be a shame to over complicate it for some less technical users?

I don't mind configuration at all - I learned to programme writing spaghetti PHP (the shame) - but I'm increasingly of the mindset that convention is on the whole better, simply as it allows you to focus on the "doing" more. Possibly the optional declaration/fallback guessing is the way forward?

[parndt]

I think that you're right, conventions would be best. Let's use the
classes not strings.

[parndt]

Using conventions does mean you'll have to document this well :-)

[jgwmaxwell]

Haha, what? Write "Words" about what the code does? As well as tests? Awwwww, this is no fun! Can't they just guess? ;)

Ok, so classes not strings, simply declaring the class? I did just think I was being a tool in working out the engine that the model belonged to - rather than guessing, I should just be asking the model! I've been writing code for 36 hours, I get a little slow.

So, if it is always added to Refinery::Page (controlled by a boolean config var maybe?) then everything else should identify correctly through Model.parent_name.

Is there a reason why Refinery::Page isn't actually Refinery::Pages::Page?

[parndt]

There is a reason and it's mainly just laziness on the part of the core developers wanting to release Refinery 2.0

Shame on them.

You should get some sleep!

[parndt]

I believe this was implemented by #81