Merged
Conversation
This PR started because the documentation on how to configure an Http client was out of date. However, as I investigated further, I found problems. GuzzleHttp, a client mentioned in the documentation, for example - it probably works fine when executed from a browser, but it does not work for `https:` requests from the Windows command line. See guzzle/guzzle#2601, where a user notes that Guzzle's own program to demonstrate how to use it doesn't work, a result that I can confirm is still true. A linked explanation says that the solution is to explicitly specify a path to a CA bundle. For starters, this is impractical from our perspective. One possible solution is to change a php.ini option which is not needed for any other purpose, and which probably needs to change frequently - a burden on users who follow that route. An alternative solution is to use a method `request` to specify the path to the certificate store; this also will need to change from time to time, and, worse, the only method defined in ClientInterface is `sendRequest`, so using this solution isn't client-agnostic, which is a stated goal of PHPOffice@7cb4884. Additionally, it is not clear why an external interface is needed rather than a call to file_get_contents, used elsewhere in PhpSpreadsheet, and not requiring a path to a certificate store. I also believe that automatically evaluating WEBSERVICE for any arbitrary argument is not a good idea. I am adding a domain whitelist which the user must populate. For domains not in the whitelist, the calculation will revert to `oldCalculatedValue`, which is good enough for pass-through purposes, which probably encompasses most cases. That is how Excel behaves by default - it disables WEBSERVICE calls when it opens a spreadsheet which contains them. For cases where the user adds a new WEBSERVICE call, there is a choice of whitelisting the domain, or getting the result in some other way and using `setCalculatedValue` to store it. Finally, when a WEBSERVICE call *is* evaluated, it will now accept a cell-address argument rather than just a literal string as is now the case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR started because the documentation on how to configure an Http client was out of date. However, as I investigated further, I found problems. GuzzleHttp, a client mentioned in the documentation, for example - it probably works fine when executed from a browser, but it does not work for
https:requests from the Windows command line. See guzzle/guzzle#2601, where a user notes that Guzzle's own program to demonstrate how to use it doesn't work, a result that I can confirm is still true. A linked explanation says that the solution is to explicitly specify a path to a CA bundle. For starters, this is impractical from our perspective. One possible solution is to change a php.ini option which is not needed for any other purpose, and which probably needs to change frequently - a burden on users who follow that route. An alternative solution is to use a methodrequestto specify the path to the certificate store; this also will need to change from time to time, and, worse, the only method defined in ClientInterface issendRequest, so using this solution isn't client-agnostic, which is a stated goal of 7cb4884. Additionally, it is not clear why an external interface is needed rather than a call to file_get_contents, used elsewhere in PhpSpreadsheet, and not requiring a path to a certificate store.I also believe that automatically evaluating WEBSERVICE for any arbitrary argument is not a good idea. I am adding a domain whitelist which the user must populate. For domains not in the whitelist, the calculation will revert to
oldCalculatedValue, which is good enough for pass-through purposes, which probably encompasses most cases. That is how Excel behaves by default - it disables WEBSERVICE calls when it opens a spreadsheet which contains them. For cases where the user adds a new WEBSERVICE call, there is a choice of whitelisting the domain, or getting the result in some other way and usingsetCalculatedValueto store it.Finally, when a WEBSERVICE call is evaluated, it will now accept a cell-address argument rather than just a literal string as is now the case.
This is:
Checklist: