implemented file upload capability#53
Conversation
mghoneimy
left a comment
mghoneimy
left a comment
There was a problem hiding this comment.
Besides the code level comments I have added, we still need:
- Unit tests to cover new functionality added
- A GraphQL API server to be able to test this feature against as an integration test
| * @param Query|QueryBuilderInterface $query | ||
| * @param bool $resultsAsArray | ||
| * @param array $variables | ||
| * @param array $variables |
There was a problem hiding this comment.
why do we have duplicate $variable definition in docblock?
| foreach ($files as $key => $content) { | ||
| $map->{$key} = [ | ||
| $content['path'] | ||
| ]; | ||
|
|
||
| $postDataFields[$key] = [ | ||
| 'name' => $key, | ||
| 'contents' => fopen($content['file'], 'r'), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Please define a new class File to be used to pass file data instead of passing them as array fields.
Monday.com supports files: https://monday.com/developers/v2#mutations-section-add-file-to-column |
| $request = $request->withBody(Psr7\stream_for(json_encode($bodyArray))); | ||
|
|
||
| if (!$files) { | ||
| $request = $request->withBody(Psr7\stream_for(json_encode($bodyArray))); |
There was a problem hiding this comment.
This couples Guzzle tightly to the client. I think stream discovery should be used here. For example with php-http/discovery's \Http\Discovery\Psr17FactoryDiscovery:
\Http\Discovery\Psr17FactoryDiscovery::findStreamFactory()->createStream(\json_encode($bodyArray));
|
Lighthouse also supports file uploads. https://lighthouse-php.com/master/digging-deeper/file-uploads.html#handling-file-uploads It's been awhile since this PR has had any activity. It would be good to get this merged. |
5 participants
No description provided.