-
Notifications
You must be signed in to change notification settings - Fork 46
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
Remove redundant api call for retreiving metadata #204
Conversation
Why are we calling here `$this->api->getMetadata()` instead of directly the `$this->inner->getMetadata()`? It makes a unnecessary api call to the API.
Bump, is there something I could do? |
Hi! Sorry, we've missed this PR. We keep API call because data can be change by some other actor. It's not your case, probably, but still it is the case for someone else. |
Add getInner() function.
Thanks for the reply! What about 6f6dbb1 so I could use the following code to retrieve the meta data without additional requests: $extraMetadata = $file->getInner()->getMetadata(); |
In this case we think it's better to create a method that returns existing data. And additional method (not in FileInfoInterface, but in FileApi) that updates it. We can't merge this change because it goes against current architecture and will confuse other library users. But it looks that it works for you. |
I understand that using a To add a little more context: |
Now I see where this question is come from. Thanks for the flysystem, btw! Our PHP dev tries to protect current architecture and not introduces other approaches when it's possible not to. Or, even if we explain it in the docs, people will stumble upon it every time. If we accept your PR, we will ruin consistency. :-( Why can't you use I'm sorry that we still can't really understand what stays in your way. |
Thanks for the reply! FYI, |
When you get the list of files, you also get metadata right away, aren't you? https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File/operation/filesList (I'm sorry, I'm not a dev myself and don't know any of PHP, and I pass your messages to our PHP dev. But this I want to understand for myself. Because maybe we should improve our API). |
Yes. When you get files (or a specific file) you will get the metadata, that is available in Here is an example that only used the official Uploadcare package: $configuration = \Uploadcare\Configuration::create('<public_key>', '<secret_key>');
$api = new \Uploadcare\Api($configuration);
$fileInfo = $api->file()->fileInfo('<uuid>');
dump($fileInfo); // This variable already includes the metadata, see screenshot. If I add the following code: dump($fileInfo->getMetadata()); However. This is requested through an (extra) API call, instead of just accessing it from $this->inner->getMetadata(); 0da2c0e fixes that. |
Hi @Casmo It sounds reasonable, but my concern is consistency. What if the metadata has been changed in the meantime? But still, it sounds very reasonable. I think we should do this:
And then we can release it. |
What can we do to release this? We're actually waiting for this, because we have 100.000 files and don't want to do 100.000 API calls (because we need the metadata per file). |
Hi Mark. But you've already saw the answer three weeks ago.. We were waiting for your commits to release it. Or do you want us to do it? We thought it would be impolite to take it from you. |
I'll fix it, soon! |
Use inner for metadata.
@rsedykh I changed the code to the original PR. Could you give me a direction on how you want me to update the documentation? The current documentation already gives the current metadata. This is in the documentation (https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File-metadata): $configuration = Uploadcare\Configuration::create((string) $_ENV['UPLOADCARE_PUBLIC_KEY'], (string) $_ENV['UPLOADCARE_SECRET_KEY']);
$api = (new Uploadcare\Api($configuration))->file();
$fileInfo = $api->fileInfo('1bac376c-aa7e-4356-861b-dd2657b5bfd2');
echo \sprintf("File %s metadata:\n", $fileInfo->getUuid());
foreach ($fileInfo->getMetadata() as $metaKey => $metaItem) {
echo \sprintf("%s: %s\n", $metaKey, $metaItem);
} |
Hi. We've checked the code, it works. I wasn't talking about that documentation, I was just wondering if we have to change something in README.MD. But probably not. I'll accept this PR today and finish everything else (e.g. CHANGELOG.MD) and bump the lib version. |
Why are we calling here
$this->api->getMetadata()
instead of directly the$this->inner->getMetadata()
? It makes a unnecessary api call to the API.Description
When I use the following code it will make an api request for each file:
Checklist