bmcclure / CakePHP-Media-Plugin

A CakePHP (2.0) plugin enabling transfer/manipulation/embedding of files in 23 ways.
Other
60 stars 24 forks source link

Revamp MediaHelper #51

Closed ndm2 closed 10 years ago

ndm2 commented 10 years ago

As described in https://github.com/bmcclure/CakePHP-Media-Plugin/issues/49#issuecomment-28238733, this patch switches back to extending AppHelper, and ditches manual tag generation and overriding Helper::_parseAttributes() in favor of using HtmlHelper::useTag() and utilizing config files (as suggested by @gBokiau in https://github.com/bmcclure/CakePHP-Media-Plugin/pull/50#issuecomment-28200398) to configure HtmlHelper with proper tags and minimizable attributes.

It should be backwards compatible and resolve #50.

bmcclure commented 10 years ago

I like this approach. It's fairly substantial, but it seems relevant to the issue at hand, and @ndm2 has convinced me that going back to extending AppHelper is the cleanest way to go.

@ndm2, based on the last two comments are you planning to add another commit to this PR to load both configs if a custom one is specified? I have not tested these changes, but after looking over the code I do not see any other issues with this.

And finally, just to confirm, @gBokiau, are you in agreement that this is a good approach, since I believe it would prevent merging https://github.com/bmcclure/CakePHP-Media-Plugin/pull/50? Are there any changes/fixes in your existing PR that aren't covered by this one?

ndm2 commented 10 years ago

Yes, I'm going to add another commit that adds support for loading both configurations, just wanted to wait until you had a look at the whole thing too.

bmcclure commented 10 years ago

With the latest commit added, I don't see any further issues that would keep me from merging this in.

I think to the extent that we can keep backwards compatibility without a lot of extra work/code, we should. Down the line if features or changes are introduced that make the plugin no longer compatible all the way back to 2.0.x (for a good enough reason, of course), we could revisit cleaning it up a bit and removing some things that would no longer be necessary.

ndm2 commented 10 years ago

Sounds good to me.