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

BUGFIX: Consistently initialize asset sources via createFromConfiguration #4008

Merged

Conversation

grebaldi
Copy link
Contributor

fixes: #3965

The Problem

The case at hand was an asset source that uses a value object to validate the incoming asset source options. I expected to be able to define a promoted constructor property with said value object as its declared type:

final class MyAssetSource implements AssetSourceInterface
{
    public function __construct(
        private readonly string $assetSourceIdentifier,
        private readonly Options $options
    ) {
    }

   /* ... */
}

...and initialize the value object in the createFromConfiguration static factory method defined by the AssetSourceInterface:

final class MyAssetSource implements AssetSourceInterface
{
   /* ... */

    public static function createFromConfiguration(string $assetSourceIdentifier, array $assetSourceOptions): AssetSourceInterface
    {
        return new static(
            $assetSourceIdentifier,
            Options::fromArray($assetSourceOptions)
        );
    }
}

This failed with a Type Error, because the AssetSourceService, which is responsible for initializing asset sources, at one point does not utilize createFromConfiguration to perform initialization, but calls the asset source constructor directly:

$this->assetSources[$assetSourceIdentifier] = new $assetSourceConfiguration['assetSource']($assetSourceIdentifier, $assetSourceConfiguration['assetSourceOptions'] ?? []);

The Solution

I adjusted the aforementioned routine of the AssetSourceService to use the AssetSourceInterface-defined createFromConfiguration static factory method instead of the asset source's constructor.

Even though the pattern I described above only makes sense in a PHP >8.0 environment, I decided to target Neos 7.3 with this PR, because it should constitute a non-breaking bugfix.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 by reading

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine by reading

@bwaidelich bwaidelich merged commit d1a5094 into neos:7.3 Sep 27, 2023
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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.

BUG: (Neos.Media) AssetSourceService relies on constructor signature of custom asset sources
4 participants