-
-
Notifications
You must be signed in to change notification settings - Fork 237
FEATURE: Asset usage generic compatiblility with value objects in node properties #5683
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
base: 9.1
Are you sure you want to change the base?
FEATURE: Asset usage generic compatiblility with value objects in node properties #5683
Conversation
| /** | ||
| * @return list<AssetId> | ||
| */ | ||
| public function getAssetIds(): array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i went with an "always plural" interface as we have to account for the empty case and that a value object handles more than one asset too. So this seems the best fit.
Also we should probably specify iterable so people could yield the asset ids for simplicity during nesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterable wound indeed be a better fit than array.
| return $this->href->getHost() . $this->href->getPath(); | ||
| } | ||
|
|
||
| public function extractNodeAggregateId(): ?NodeAggregateId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a big fan of tryExtractWhatever if whatever is nullable, for readability
|
|
||
| use Neos\Media\Domain\Model\AssetId; | ||
|
|
||
| interface ProvidesAssetIdsInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name sounds a bit strange why not AssetIdProviderInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IUntil now i have seen the following patterns for handlers / extractors / converter … thingys
- A chain of objects that get asked wether they can handle the type
- A configuration that specifies the extractor for each type
The approach here would be a third pattern but it has the clear benefit allowing a really type safe implementation. Only downside is that there is no delegation as everything is implemented by the object itself. Actually this might be the biggest advantage aswell.
I think in total it makes sense. If there is prior art of this pattern in the codebase i would agree even more
Followup to #5682
Separated because discussions might block main thread.
Upgrade instructions
Review instructions
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions