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

Allow fetching extra metadata #1324

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

timwsuqld
Copy link

Fix #1323

Allow fetching of extra metadata so that more customised code can be written that isn't adapter specific

@Nyholm
Copy link
Member

Nyholm commented Jul 18, 2021

Adding methods to an interface is a BC break. I cannot merge this.

@4c0n
Copy link

4c0n commented Nov 11, 2021

@Nyholm @frankdejonge He also explains that in the issue: #1323

I would kind of like this feature as well, as after "upgrading" to version 2 I lost the ability of checking the S3 object version metadata amongst other metadata, are there any plans on starting a new branch/work on a new major version so we can overcome the BC break?

We could also go the less pretty route and not update the interface, but add the methods to the adapters for example.

Let's talk about this 😄

@m-vo
Copy link

m-vo commented Dec 1, 2021

@Nyholm Would you be fine with adding a method to the Filesystem like so?

public function extraMetadata(string $path): array
{
    if (!method_exists($this->adapter, 'extraMetadata')) {
        return [];
    }

    return $this->adapter->extraMetadata($this->pathNormalizer->normalizePath($path))->extraMetadata();
}

With the adapter pattern you're typically only interfacing with the single default implementation, effectively making the adapters a black box. I don't see how your're otherwise able to get the metadata that the StorageAttributes classes can already hold.

An alternative with less magic would be to add a new interface for adapters supporting an extraMetadata function and test against this interface in the Filesystem.

@PaolaRuby
Copy link
Contributor

@m-vo make a PR

@m-vo
Copy link

m-vo commented Dec 27, 2021

@m-vo make a PR

Not if there is no intention for it to get merged?

@fredsal
Copy link

fredsal commented Jan 31, 2022

This would have been great for v3

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Apr 16, 2022
@aedart
Copy link

aedart commented May 9, 2022

Ah... Can we perhaps re-work this to support additional meta-data, without having to introduce a breaking change?
A possible solution would be to extract the the extraMetadata() into its own interface, so that adapters that support it can chose to implement it, e.g.

interface HasMetaData
{
    /**
     * @throws UnableToRetrieveMetadata
     * @throws FilesystemException
     */
   public function extraMetadata(string $path): FileAttributes;
}

As for the filesystem implementation, it does not have to beak it's interface, but simply offer a public method:

class Filesystem implements FilesystemOperator
{
    // ...previous not shown ....

    public function extraMetadata(string $path): array
    {
        $adapter = $this->adapter;
        
        if ($adapter instanceof HasMetaData) {
            return $this->adapter->extraMetadata($this->pathNormalizer->normalizePath($path))->extraMetadata();    
        }
        
        // Default to empty array
        return [];
    }
}

Then, perhaps, in a future major version you could choose to merge the "HasMetaData" interface into the appropriate interfaces.

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.

Allow fetching of extraMetadata
7 participants