Skip to content

Options API: Use cURL instead of file_get_contents #2560

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.

Sign up for GitHub

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

Jump to bottom
Merged
bastianallgeier merged 3 commits into from
May 4, 2020
Merged

Options API: Use cURL instead of file_get_contents #2560

bastianallgeier merged 3 commits into from
May 4, 2020

Conversation

@lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Apr 4, 2020 •
edited
Loading

Describe the PR

file_get_contents() is not really meant for HTTP requests and therefore unreliable with some remote servers. Switching to cURL with our Remote class should make the requests more reliable.

Thanks for the suggestion @rasteiner.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Fixed code style issues with CS fixer and composer fix
  • Added in-code documentation (if needed)

@lukasbestle lukasbestle added this to the 3.4.0 milestone Apr 4, 2020
@lukasbestle lukasbestle self-assigned this Apr 4, 2020
nilshoerrmann
nilshoerrmann reviewed Apr 4, 2020
View reviewed changes
@afbora
Copy link
Member

afbora commented Apr 4, 2020

I agreed with @nilshoerrmann. What if we put into try catch block?

@lukasbestle
Copy link
Member Author

lukasbestle commented Apr 5, 2020

@afbora I agreed with @nilshoerrmann. What if we put into try catch block?

The example you posted in #2330 (comment) doesn't do anything unfortunately as it interrupts the Exception chain only to start a new one. try {} catch {} is for handling errors (basically for stopping the Exceptions from bubbling up), not for triggering errors.

Also, as I explained in my answer to Nils' comment above, file_get_contents() only triggers PHP warnings, which can't be caught by try {} catch {}.

But I think my new solution works quite well to ensure that the Panel shows errors for common issues.

@lukasbestle lukasbestle force-pushed the fix/2330-options-api-curl branch from 18eee64 to c374ef8 Compare April 5, 2020 11:53
@lukasbestle lukasbestle linked an issue Apr 5, 2020 that may be closed by this pull request
Multiselect: better failure handling for options from API #2330
Closed
@bastianallgeier bastianallgeier merged commit 042058a into develop May 4, 2020
@bastianallgeier bastianallgeier deleted the fix/2330-options-api-curl branch May 4, 2020 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bastianallgeier bastianallgeier bastianallgeier approved these changes

+1 more reviewer

@nilshoerrmann nilshoerrmann nilshoerrmann left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lukasbestle lukasbestle

Labels

None yet

Milestone

3.4.0

Development

Successfully merging this pull request may close these issues.

Multiselect: better failure handling for options from API

5 participants

@lukasbestle @afbora @bastianallgeier @nilshoerrmann