Feature/extensible metadata properties#12
Feature/extensible metadata properties#12c4ll-m3-j4ck wants to merge 9 commits intoneos:feature/extensible-metadata-propertiesfrom
Conversation
| return $spacePoint; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
just wondering: shouldn't this fail instead?
There was a problem hiding this comment.
As discussed: We probably don't even need to expose this
There was a problem hiding this comment.
We use this in the changes made to Flowpack.Media.Ui currently, but it could be replaced by using the new map() implementation and filtering on the consumer-side.
So I concur, I'll remove it.
There was a problem hiding this comment.
It's definitely not an important point. But if we remove it, we don't have to settle on the failure behavior :)
c4ll-m3-j4ck
force-pushed
the
feature/extensible-metadata-properties
branch
from
b89f276 to
86a1684
Compare
bwaidelich
left a comment
bwaidelich
left a comment
There was a problem hiding this comment.
@c4ll-m3-j4ck Thanks a lot for your work, it's a great direction IMO.
I just added some (mostly nitpicky) comments.
IMO this can be merged onto the feature branch so that we can work on a common code base
| /** | ||
| * A point in the dimension space with coordinates DimensionName => DimensionValue. | ||
| * E.g.: ["language" => "es", "country" => "ar"] | ||
| * E.g.: ["language" => ["es"], "country" => ["ar"]] |
There was a problem hiding this comment.
This code was more or less copied from the original AbstractDimensionSpacePoint.php.
AFAIK, the DSP is a concrete point in the dimension – now this is rather describing the whole dimension space
There was a problem hiding this comment.
I made this change to the structure to achieve the same hashing result as in the CR in v8. The dsp hashes are generated by hashing over exactly this structure (dimensions => [value]). I honestly don't know why it is done that way but I wanted to align the hashes.
But yeah, technically this could describe the entire dimension space. I don't have a strong opinion on this one, it's probably clearer to the intent if we do it correctly. The only thing this would mean is that we don't have a direct relation between the dsp-hashes of the old cr and ours.
There was a problem hiding this comment.
OK, I was looking at the Neos 9 code where the hash for empty is d751713988987e9331980363e24189ce (which is equivalent to md5(json_encode([]))) and the hash for en_UK is 046d33049fe2ab3d1c3c54ca5340186f which is the equivalent to md5(json_encode(['language' => 'en_US'])).
But you are correct, that this is stored differently in Neos 8 – I wasn't aware of that.
I would suggest to keep the DTOs clean though and somehow "translate" it in the concrete v8 adapter if possible
| return $spacePoint; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
As discussed: We probably don't even need to expose this
| yield from $this->spacePoints; | ||
| } | ||
|
|
||
| public function getHashIterator(): Traversable |
There was a problem hiding this comment.
nitpick: It's called getIterator above only because that's the interface, but it is not meant to be invoked manually.
If we expose all hashes I would suggest to replace
'dimensionHashes' => iterator_to_array($dimensionSpacePoints->getHashIterator()),by
'dimensionHashes' => $dimensionSpacePoints->getHashes(),or: don't expose them and go for
final readonly class MetaDataDimensionSpacePoints implements IteratorAggregate {
// ...
/**
* @template T
* @param Closure(MetaDataDimensionSpacePoint): T
* @return T[]
*/
public function map(Closure $callback): array {
return array_map($callback, $this->spacePoints);
}and
'dimensionHashes' => $dimensionSpacePoints->map($spacePoint->hash)2 participants
This PR adds necessary changes to work with the updated implementation in Flowpack.Media.Ui