silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

FIX: Ensure attributes are stored against the Image record (fixes #513) #514

Closed kinglozzer closed 1 year ago

kinglozzer commented 1 year ago

Proposed solution to #513 - pushing to see if CI builds pass.

Currently calling Image::setAttributes() will return a DBFile instance instead of a modified copy of the original image, this changes that behaviour. Both Image and DBFile implement the AssetContainer interface so this isn’t a BC break

kinglozzer commented 1 year ago

Anyone have any feedback on this? It’s still causing me issues 😞

michalkleiner commented 1 year ago

I'm not against it, but as Guy said on the issue, it's hard to foresee the potential implications and what this could break, mainly about the backend.

Could you try a mixed solution where we keep the method and update the implementation to e.g.

/**
     * Create an equivalent DBFile object
     * @return AssetContainer
     */
    private function copyImageBackend(): AssetContainer
    {
        $file = clone $this;

        $backend = $this->getImageBackend();
        if ($backend) {
            $file->setImageBackend($backend);
        }

        return $file->setOriginal($this);
    }

and then in the setAttribute keep it as it was and add the cache clear call. Can you test if that would still work for you? If so, I'd be probably ok with that.

kinglozzer commented 1 year ago

Thanks @michalkleiner, I’ve pushed up a new commit with what you’ve suggested

michalkleiner commented 1 year ago

Thanks for the update @kinglozzer. Don't want to be too annoying, but do you think we can add a test for this that would fail with the original implementation?

kinglozzer commented 1 year ago

Have added a simple test that fails with the original implementation

michalkleiner commented 1 year ago

Legend, will try to have a look later today and get this in.

GuySartorelli commented 1 year ago

Released as 1.12.1