Fix HTTPS and embed region support [#2308785]

ReadSpeaker recommend self-hosting their Javascript library if on HTTPS. Attempting to serve it via their servers results in a mismatched certificate error due to their CDN. This patch forces you to self-host if on HTTPS.

Also, for HTTP, the region the embed is served from is hardcoded to be 'eu'. This patch adds a region selector that lets you choose from ReadSpeakers North American, European or Asia-Pacific servers.

Unfortunately, this patch requires the changes from #2308749: Move block settings variables to admin settings page.

Comment	File	Size	Author
#10	interdiff_8_9.txt	3.01 KB	leandro713
#9	open_readspeaker-https_and_region-2308785-9-7.x.patch	6.19 KB	leandro713
#8	interfiff_5_8.txt	5.57 KB	eduardo morales alberti
#8	open_readspeaker-https_and_region-2308785-8-7.x.patch	5.7 KB	eduardo morales alberti
#5	interdiff_0-7.txt	3.3 KB	eduardo morales alberti
#5	open_readspeaker-fix_https_and_region-2308785-5-7.x.patch	8.74 KB	eduardo morales alberti
	open_readspeaker-fix_https_and_region--0-D7.patch	8.27 KB	torpy

Comments

Comment #1

killua99 commented 5 August 2014 at 20:57

Status:

Needs review

» Needs work

Need to see this patch in live.

I'll test it asap

Comment #2

killua99 commented 7 August 2014 at 11:41

Category:

Bug report

» Task

+++ b/open_readspeaker.module
@@ -51,6 +54,22 @@ function open_readspeaker_theme($existing, $type, $theme, $path) {
+    'version callback' => '_open_readspeaker_shortcircuit_version',

@@ -123,15 +167,24 @@ function theme_open_readspeaker_ui() {
+function _open_readspeaker_shortcircuit_version() {
+  return TRUE;
+}

You're calling this function because you want me to add the real logic about version?

+++ b/open_readspeaker.module
@@ -85,17 +104,33 @@ function open_readspeaker_block_view($delta = '') {
+  if ($use_https) {
+    $library = libraries_detect('readspeaker');

What happen when is FALSE?

+++ b/open_readspeaker.module
@@ -111,11 +146,20 @@ function theme_open_readspeaker_ui() {
+    $embed_url = str_replace(array(
...
+      variable_get('open_readspeaker_region', OPEN_READSPEAKER_DEFAULT_REGION),
...
+      variable_get('open_readspeaker_customparam', ''),

I like more use preg_replace but is ok. And why you don't declare those variables?

+++ b/open_readspeaker.module
@@ -123,15 +167,24 @@ function theme_open_readspeaker_ui() {
+  $output .= '<a rel="nofollow" class="rsbtn_play" title="' . t('Listen to this page using ReadSpeaker') . '" accesskey="L" href="' . $protocol . '://app.readspeaker.com/cgi-bin/rsent?customerid=' . $accountid . '&amp;lang=' . $open_readspeaker_i18n . '&amp;readid=' . $read_id . '&amp;url=' . urlencode($request_path) . '">';

Maybe could be a better idea use the l function instead the HTML markup I'm thinking about that.

This need some small fix. You can work this patch without the "block settings patch" I still don't see the real point to move those settings. When I create this module for the web page that implement this like this way keep with other permission those settings, away for the Site Builder.

Comment #3

torpy commented 12 August 2014 at 23:54

Hey, answers below:

1. I'm using a short-circuit version callback because we can't depend on a particular ReadSpeaker library version. At the moment, the Libraries API doesn't let you specify external libraries without an explicit version or callback.
2. I'm not sure what you mean? The variables are set on the admin form.
3. Good point. It might be better to pull this out into a template too.