Added function to set global curl-options #130
Conversation
wachterjohannes
changed the title
|
@lsmith77 @dbu @dantleech what do you think? |
|
looks good to me |
| if ($forceHttpVersion10) { | ||
| $this->addCurlOptions(array(CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_0)); | ||
| } elseif (array_key_exists(CURLOPT_HTTP_VERSION, $this->curlOptions)) { | ||
| unset($this->curlOptions[CURLOPT_HTTP_VERSION]); |
There was a problem hiding this comment.
no need for the array_key_exists check, just unset right away. unsetting something that does not exist is no error.
|
+1. version two of jackalope-jackrabbit should use the HTTPlug client abstraction layer instead of requiring a specific version of guzzle... once we have that, users could use the HTTPlug plugins to do such things and the code on our side will be simplified a lot. |
|
@dbu this would be great. i will test this on sulu to be sure that everything is fine. should i also add a testcase for that? would it be possible to release that as a hotfix? |
|
i think the option is here mostly because testing reveiled problems with that http 1.0 option. so we do have a bit of tests. if you have ideas for more testing, sure. but i hope we eventually get to version 2 and then those tests become obsolete, so its not that important. can you simplify that unset call? then its ready to merge, once you confirm it works in sulu. and we can tag a patch version, sure. |
|
@dbu i have to be sure that i can inject the parameter from droctrine-phpcr bundle but i am on the way and i think i can finish the PR before lunch (: |
|
@dbu now this should be ready |
| @@ -44,6 +44,7 @@ class RepositoryFactoryJackrabbit implements RepositoryFactoryInterface | |||
| 'jackalope.logger' => 'Psr\Log\LoggerInterface: Use the LoggingClient to wrap the default transport Client', | |||
| Session::OPTION_AUTO_LASTMODIFIED => 'boolean: Whether to automatically update nodes having mix:lastModified. Defaults to true.', | |||
| 'jackalope.jackrabbit_force_http_version_10' => 'boolean: Force HTTP version 1.0, this can in solving problems with curl such as https://github.com/jackalope/jackalope-jackrabbit/issues/89', | |||
| 'jackalope.jackrabbit_curl_options.*' => 'mixed: Additional global curl-options', | |||
There was a problem hiding this comment.
i would prefer a single option jackalope-jackrabbit_curl_options that contains a hashmap. then users can also use the curl constants to define the options. and you don't need the loop to extract and separate options.
There was a problem hiding this comment.
i also thought about that but the current doctrine phpcr-bundle cannot handle this. there you have to define the parameters as an array of scalar https://github.com/doctrine/DoctrinePHPCRBundle/blob/master/DependencyInjection/Configuration.php#L165
There was a problem hiding this comment.
maybe add a separate configuration map for curl options then? the bundle config does not need to mirror the internal structure of jackalope.
There was a problem hiding this comment.
do you think that i should should move this functionality into the bundle?
There was a problem hiding this comment.
i would add a separate arrayNode next to parameters and call it curl_options. then the DI extension class would gather that information and put the array into that field of $parameters.
There was a problem hiding this comment.
sounds good i also will create a PR there
|
@lsmith77 how should we handle release management with this one? master is at 1.3 and has the version label feature. 1.2.2...master same with jackalope itself jackalope/jackalope@1.2.7...master we could start a 1.2 branch from 1.2.7 and make this PR version 1.2.8 - but i feel its not a bugfix so it should go to master. but we probably don't want to release jackalope 1.3 right away, or do we? |
|
I agree this is a feature so it should go into 1.3 |
|
hmm this sounds good but we should have this feature soon to fix our tests (: |
|
agreed. @wachterjohannes you could also depend on jackalope 1.3@dev until its released, would that work? |
|
i currently try to configure our composer that we can go to dev-master. |
|
@wachterjohannes as the branch alias is at 1.3, you should use |
|
so last thing to wrap this up: can you please add a line to CHANGELOG.md ? then we are good to merge! |
Added function to set global curl-options
|
cool, thanks! |
Added possibility to set global curl-options. Sulu has some problems with the curl-connection cache. At some points jackrabbit seems to crash when reusing a connection. I have added the CURLOPT_FORBID_REUSE option to curl-handler and it seems to work. according to @dantleech we decided to add a function which allows to add curl-options to the client (which would replace the force-http-version).
Link to a failing testcase: https://travis-ci.org/sulu-io/sulu/jobs/107715661#L952