Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added curl-options to configuration #251

Merged
merged 1 commit into from
Feb 19, 2016
Merged

Added curl-options to configuration #251

merged 1 commit into from
Feb 19, 2016

Conversation

wachterjohannes
Copy link
Contributor

@dantleech
Copy link
Contributor

Some test failures: https://travis-ci.org/doctrine/DoctrinePHPCRBundle/jobs/107765874 also need to apply the styleci fixes...

@dbu
Copy link
Member

dbu commented Feb 8, 2016

looks sane to me. can you please also create the accompagning doc PR on the doc repository?

@dbu
Copy link
Member

dbu commented Feb 8, 2016

the test failure is assumptions about the default configuration that no longer are true. can you look into those please?

@wachterjohannes
Copy link
Contributor Author

@dbu fixed tests and style-ci. where can i find the docs repository?

@lsmith77 lsmith77 added review and removed wip/poc labels Feb 8, 2016
@dbu
Copy link
Member

dbu commented Feb 12, 2016

the documentation is in the symfony-cmf docs: https://github.com/symfony-cmf/symfony-cmf-docs/edit/master/bundles/phpcr_odm/configuration.rst

please also use a versionadded construct to tell which jackalope-jackrabbit version starts supporting this. i wonder if we need to update the version requirement for jackalope-jackrabbit here, but probably not: unless you configure this option, there will be no error... and if you update your project to get this configuration option, you likely also updated jackalope to get the version that supports it.

if (array_key_exists('curl_options', $session['backend']) && count($session['backend']['curl_options'])) {
$curlOptions = array();
foreach ($session['backend']['curl_options'] as $option => $value) {
$curlOptions[constant('CURLOPT_'.$option)] = $value;
Copy link
Member

Choose a reason for hiding this comment

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

is constant() case insensitive or do people use to write the constant names in all caps? if the later, i wonder if we should not force them to write the CURLOPT_ part themselves as well, to make things more straightforward?

should we also allow numeric options here and only resolve if the option is a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would be possible! it would also be possible to uppercase the $option that should be enough.

Copy link
Member

Choose a reason for hiding this comment

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

i would prefer to have them write the full CURLOPT_XX thing and require it to be uppercase. the less variation and magic, the easier to understand.

and lets check is_int to allow people using php configuration to use the constants themselves.

@wachterjohannes
Copy link
Contributor Author

@dbu i have fixed the config and opened a docs PR

@dbu
Copy link
Member

dbu commented Feb 18, 2016

thx! can you please add a note to the CHANGELOG.md and squash the commits?

btw, agree with the switch from HEREDOC to NOWDOC (man, needed some time to find those terms :-) )

@wachterjohannes
Copy link
Contributor Author

@dbu finished

dbu added a commit that referenced this pull request Feb 19, 2016
Added curl-options to configuration
@dbu dbu merged commit 6d47b03 into doctrine:master Feb 19, 2016
@lsmith77 lsmith77 removed the review label Feb 19, 2016
@dbu
Copy link
Member

dbu commented Feb 19, 2016

i released 1.3.2 with this change.

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.

4 participants