Skip to content

Conversation

@DerMika
Copy link
Contributor

@DerMika DerMika commented May 19, 2016

Same pull request as done on jpastoor/jira-api-restclient:

On PHP 5.5 +, you get an E_DEPRECATED when trying to upload files. This PR fixes that by using a \CURLFile object to upload (see https://wiki.php.net/rfc/curl-file-upload)

if ($method == 'POST') {
curl_setopt($curl, CURLOPT_POST, 1);
if ($isFile) {
$data['file'] = $this->getCurlValue($data['file']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, I'll fix it

protected function getCurlValue($fileString)
{
$theCurlValue = $fileString;
$regex = "|^@(.*);filename=(.*)$|";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this regex for checking if given value isn't already in correct format that is created by curl_file_create function?

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

Ok, removed all the unneeded stuff. Is this more like it?

if (!function_exists('curl_file_create')) {
return $fileString;
} else {
return curl_file_create(substr($fileString, 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return it, no else needed.

@aik099
Copy link
Member

aik099 commented May 19, 2016

Ok, removed all the unneeded stuff. Is this more like it?

Almost.

@aik099
Copy link
Member

aik099 commented May 19, 2016

In which PHP version @filename notation would stop working for curl uploads?

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

Here is the RFC: https://wiki.php.net/rfc/curl-file-upload

they say

In 5.6, @ option will be switched off by default, but can still be enabled by explicit curl_setopt setting, such as:

and

In future versions, this capability may be removed completely.

But I don't think an actual decision is made to remove this capability yet.

@aik099
Copy link
Member

aik099 commented May 19, 2016

So without your change on PHP 5.6 file upload won't work at all, right?

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

On PHP 5.6 you can still enable the old behaviour with a curl_setopt(CURLOPT_SAFE_UPLOAD, false); but even then you get the E_DEPRECATED - which is the one that was bugging me.

@aik099
Copy link
Member

aik099 commented May 19, 2016

On PHP 5.6 you can still enable the old behaviour with a curl_setopt(CURLOPT_SAFE_UPLOAD, false); but even then you get the E_DEPRECATED - which is the one that was bugging me.

You can enable it, but it's turned off by default and it means that uploads through this JIRA API client won't work anymore unless I merge your fix.

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

Well actually it's currently already enabled by default - see https://github.com/chobie/jira-api-restclient/blob/master/src/Jira/Api/Client/CurlClient.php#L83 :

        if ($isFile) {
            if (defined('CURLOPT_SAFE_UPLOAD')) {
                curl_setopt($curl, CURLOPT_SAFE_UPLOAD, false);
            }

As I said, my PR was just to get rid of the ugly deprecated notice. And also to be future proof once this option goes away in the future.

@aik099
Copy link
Member

aik099 commented May 19, 2016

Then maybe setting CURLOPT_SAFE_UPLOAD to false isn't needed anymore?

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

I would think so, but honestly I haven't investigated if that could cause adverse effects.

@aik099
Copy link
Member

aik099 commented May 19, 2016

OK. Then could you please squash and I'll merge it in.

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

Unless I did something wrong, I believe I've squashed everything already?

@aik099 aik099 merged commit cdc8416 into console-helpers:master May 19, 2016
@aik099
Copy link
Member

aik099 commented May 19, 2016

Merging. Thanks @DerMika .

@DerMika
Copy link
Contributor Author

DerMika commented May 19, 2016

You're welcome, thanks for indulging me :)

@DerMika DerMika deleted the curldeprecated branch May 19, 2016 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants