In D7 the chart_copy() method does not work correctly. It seems it has not been rewritten to accomodate for the change in the file system (and some methods does not exist in Drupal 7).

CommentFileSizeAuthor
#13 chart-copy-issue-1303706-comment-13.patch2.06 KBlsolesen
#11 chart-copy-issue-1303706-comment-11.patch2.2 KBlsolesen
#2 chart-copy-issue-1303706.patch2.07 KBlsolesen

Comments

lsolesen’s picture

lsolesen commented
Status: Needs review » Active

Gives a fatal error because a D6 function is called twice in the function.

lsolesen’s picture

lsolesen commented
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new chart-copy-issue-1303706.patch2.07 KB

Attached path that fixes chart_copy and upgrades the function to D7.

13rac1’s picture

13rac1 commented
Status: Active » Needs review

The code for chart_copy() looks good.

I cannot apply the code change to chart_url() since it will cause problems for non-HTTPS sites. Is there an issue with that code?

-    return url('//' . $uri, array('query' => $data, 'external' => TRUE));
+    return url('https://' . $uri, array('query' => $data, 'external' => TRUE));
lsolesen’s picture

lsolesen commented

@eosrei Originally the code was set to https:in earlier version of chart

The current code will not result in a valid link, as no protocol i stated. At the moment only Google is implemented, and they serve as https? That is the reason I added https in front of $uri. chart_copy will not work otherwise.

13rac1’s picture

13rac1 commented

If that is the case, it is a separate problem and should have a separate issue. Both HTTP and HTTPS work with Google Image Charts:

lsolesen’s picture

lsolesen commented

Yeah I know Google serves both ways. Maybe the protocol should be a setting in the module then? If you think that is a better approach, I can create a new issue and make a patch for that!

13rac1’s picture

13rac1 commented

Can you show me an example where url('//'); doesn't work? I've had no problem with it.

lsolesen’s picture

lsolesen commented

file_get_contents

13rac1’s picture

13rac1 commented

Ah... Only there. Interesting it doesn't affect anything else. The ideal method to correct this is to detect what the current page is loading as and adjust the protocol accordingly.

lsolesen’s picture

lsolesen commented

Ok. I will try to change the patch. Working on it now.

lsolesen’s picture

lsolesen commented
StatusFileSize
new chart-copy-issue-1303706-comment-11.patch2.2 KB

Attached patch where the protocol is guessed from $_SERVER['HTTPS']

13rac1’s picture

13rac1 commented
Status: Needs review » Needs work

Please review this previous issue regarding HTTPS vs HTTP for chart: #587742: Use protocol relative Google Chart API URL. The code requires an isset() at the minimum.

lsolesen’s picture

lsolesen commented
StatusFileSize
new chart-copy-issue-1303706-comment-13.patch2.06 KB

Added new patch where the protocol is added in chart_copy instead.

lsolesen’s picture

lsolesen commented
Status: Needs work » Needs review
13rac1’s picture

13rac1 commented
Status: Needs review » Fixed

I fixed the whitespace errors and committed.

chart-copy-issue-1303706-comment-13.patch:38: trailing whitespace.
      return FALSE;      
chart-copy-issue-1303706-comment-13.patch:41: trailing whitespace.
    
chart-copy-issue-1303706-comment-13.patch:47: trailing whitespace.
    else { 
chart-copy-issue-1303706-comment-13.patch:50: trailing whitespace.
    
chart-copy-issue-1303706-comment-13.patch:63: trailing whitespace.
    return $filename;      
warning: 5 lines add whitespace errors.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Pierre.Vriens’s picture

Pierre.Vriens commented
Issue summary: View changes
Parent issue: » #2371075: Chart 7.x-2.x Release