using 'url' option with embed() method no longer makes a link

[gBokiau]

Hey guys,

haven't been able to minutely follow all the recent commits (I've seen some discussion about a url parameter, don't know if this is at all related)

Since MediaHelper now extends HtmlHelper function, linking an image to an url no longer works; HtmlHelper::link() now calls MediaHelpers::url() instead of Helper::url().

Linking images to urls is done by adding an 'url' parameter in the embed() options array. https://github.com/bmcclure/CakePHP-Media-Plugin/blob/cake-2.0/View/Helper/MediaHelper.php#L185

The created link thus has an empty href attribute

[ndm2]

Good spot, that was my fault, I've missed that one. I'll see what I can do to fix this.

Somehow I fear that there's no nice way to solve the initial problem. Extending the HtmlHelper was mainly ment to solve backwards compatibility problems. A nice side effect was that this would come handy in the future too, though that should really not block a fix.

We could go back to extending AppHelper and loading HtmlHelper additionally. The BC problems where $_attributeFormat is not available on Helper could be solved by dynamically defining it in the MediaHelper constructor. This was actually my first attempt on solving that problem, but I ditched it in favor of extending HtmlHelper, which seemed like the "cleaner" solution to me.

[bmcclure]

Good points. I like that MediaHelper extends HtmlHelper now (it seems to make more sense), but I can see where the difficulty comes in.

Would it make sense to rename MediaHelper::url() to something more specific since it serves somewhat of a different purpose than the url() function that other helpers utilize? Or does it have to override url() to prevent the default behavior of that method?

[ndm2]

I think from a technical point of view there was no need to overwrite this method, it could have been named anything. I guess it's just that url() is pretty straight forward and people were used to it.

It's also nice to not have too many unrelated methods in the class, ie the original Helper::url() functionality is of no use in the public Media helper API. This is actually something that bugs me little bit with extending HtmlHelper, it exposes a huge load of unrelated functionality to the public Media helper API.

Renaming would solve the problem too, but it would be a breaking change.

[bmcclure]

Very good points. I'm good with the way it's solved in https://github.com/bmcclure/CakePHP-Media-Plugin/pull/50 unless you see any future implications with going that route.

[ndm2]

Well, now that I see all the implications I start to think that extending HtmlHelper wasn't the best idea, not only, but especially because of the url() bug and the aforementioned pollution of the helper API.

The initial problem was with the flow triggered by the MediaHelper::_parseAttributes() method, which relies on Helper::_parseAttributes(). I'll have to do some further testing, but it looks like that as of 2.x it's not necessary anymore to override the _parseAttributes() implementation at all, instead (as suggested by @gBokiau) configuring the HtmlHelper using HtmlHelper::loadConfig(), and consequently using HtmlHelper::useTag() instead of all the calls to sprintf() and _parseAttributes() should do the trick.

This would decouple the two helpers, not relying on any internal functionality anymore (which is always a good thing if you ask me), and as a side effect it would also fix the problem where MediaHelper::_parseAttributes() overrides $_minimizedAttributes, resulting in only autoplay, controls, autobuffer and loop being treated as boolean attributes.

If it works that way, then this seems cleaner to me, and it would be backwards as well as possibly 3.x compatible too. I'll check that and post the code, a patch, or even submit a PR so you can have a look at it.

[bmcclure]

Great info! Just to make sure I understand, with what you're proposing we'd go back to extending AppHelper and simply utilize HtmlHelper internally similarly to how that was structured before?

[ndm2]

Yep, it would be similar to how it was before, it would extend AppHelper and utilize HtmlHelper internally.

[ndm2]

I have a PR ready, #51, please check and let me know what you think.

[bmcclure]

Thanks again! I'm in favor of the approach taken in https://github.com/bmcclure/CakePHP-Media-Plugin/pull/51. If @gBokiau is in agreement, I say we move forward with that.

[gBokiau]

I agree, let's move forward with this approach.

Would it also be a good time to consider removing other incongruencies? I'm thinking of :

• Deprecating and reducing ::url() since it's just an alias to webroot()
• remove call to h() at https://github.com/ndm2/CakePHP-Media-Plugin/blob/23962043c1e2f06fee20d2a20db8afd3cf88fd0e/View/Helper/MediaHelper.php#L258 (_sources() already escaped it). Sorry, was wrong about that. Shouldn't other urls be escaped too ?
• I would, personally, favour using Helper::assetUrl instead of copying _encodeUrl()

…I can also look at those issues at a later time

Thanks guys

[ndm2]

I'd say improvements are always welcome, please don't hesitate to come up with suggestions :+1:.

These three things have some pitfalls that might not be obvious at first sight.

• While I do agree that from a functional point of view url and webroot could for sure be merged, as the only difference is the $full parameter, I do see at least a small "problem" with this. Once one of these methods would finally be removed, the original method would add more unrelated functionality to the Media helper API. On the other hand this shouldn't block useful changes, and this one method that all helpers expose anyways, is rather insignificant in contrast to what the HtmlHelper added, so...
• The encoding applied in _sources() (respectively in url()) is different to the one used by h, the former uses percent-encoding to make sure the URL is safe in URL contexts (in so far as this is possible), while the latter uses entity-encoding for safe use in HTML contexts. Additional entity-encoding is necessary as _encodeURL() can only handle the path components of a URL, see also #42.
• Helper::assetUrl() implements a lot of unrelated functionality, it's purpose is to handle plugin and theme assets properly, so it has the potential to break things in the Media helper, and it doesn't encode absolute URLs. Also Helper::_encodeUrl() was first introduced with CakePHP 2.2.4, so this wouldn't be backwards compatible.

btw, maybe we should move discussing this into a separate ticket?

[bmcclure]

Cool, I think we're all at least in agreement on the changes in https://github.com/bmcclure/CakePHP-Media-Plugin/pull/51 and that they will address this issue.

@gBokiau Your mentioned improvements are definitely something that should be further discussed, but I think we should probably merge https://github.com/bmcclure/CakePHP-Media-Plugin/pull/51 and close out this issue, and handle those in separate tickets (or one separate ticket if they would make the most sense to be implemented together)

[bmcclure]

The PR has been merged, so this issue should be resolved.

@gBokiau: I still encourage you to create new issues for your other suggestions so we can discuss them further, don't take my closing this issue as disregarding those. Thanks!