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

Remove PropertyCollectionInterface #4464

Closed
bwaidelich opened this issue Sep 1, 2023 · 4 comments · Fixed by #4513
Closed

Remove PropertyCollectionInterface #4464

bwaidelich opened this issue Sep 1, 2023 · 4 comments · Fixed by #4513
Assignees

Comments

@bwaidelich
Copy link
Member

As discussed in todays weekly we decided to replace the PropertyCollectionInterface by a final PropertyCollection.

The reason is that the collection should have a defined behavior (e.g. return all properties as stored in the database, don't throw on non-existing props, ...)
See related discussion in #4304 (comment)

@mhsdesign
Copy link
Member

mhsdesign commented Sep 3, 2023

Moved https://github.com//issues/4317#issuecomment-1725654764

Just to be noted, i like to stub my nodes (without providing a correct node type) and currently the interface allows me to do this simple thing:

        $properties = ['foo' => "bar"];

        $propertyCollection = new class ($properties) extends \ArrayObject implements PropertyCollectionInterface {
            public function serialized(): SerializedPropertyValues
            {
                throw new \BadMethodCallException(sprintf('Method %s, not supposed to be called.', __METHOD__));
            }
        };

after:

$textNodeProperties = new PropertyCollection(
    SerializedPropertyValues::fromArray([
        'foo' => new SerializedPropertyValue('bar', 'string')
    ]),
    new PropertyConverter(
        new Serializer([
            new DateTimeNormalizer(),
            new ScalarNormalizer(),
            new BackedEnumNormalizer(),
            new ArrayNormalizer(),
            new UriNormalizer(),
            new ValueObjectArrayDenormalizer(),
            new ValueObjectBoolDenormalizer(),
            new ValueObjectFloatDenormalizer(),
            new ValueObjectIntDenormalizer(),
            new ValueObjectStringDenormalizer(),
            new CollectionTypeDenormalizer()
        ])
    )
);

related: #4317

im currently trying to find a good solution - i hijacked this pr to also implement this task #4328

@mhsdesign
Copy link
Member

mhsdesign commented Sep 4, 2023

Moved #4483 (comment)

@mhsdesign
Copy link
Member

Related #4483

@bwaidelich bwaidelich moved this from Prioritized 🔥 to In Progress 🚧 in Neos 9.0 Release Board Sep 7, 2023
@bwaidelich bwaidelich assigned mhsdesign and unassigned mhsdesign Sep 14, 2023
bwaidelich added a commit that referenced this issue Sep 14, 2023
Removes the `PropertyCollectionInterface` in order to achieve
a more reliable behavior and not to give the impression of extension
points that are actually not extensible.

**Note:** This change disables some functional `NodeHelperTest` because
there is currently no easy way to mock the `Node` read model.
We'll address this with #4317

Resolves: #4464
@bwaidelich bwaidelich moved this from In Progress 🚧 to Under Review 👀 in Neos 9.0 Release Board Sep 14, 2023
@mhsdesign
Copy link
Member

mhsdesign commented Sep 18, 2023

Moved https://github.com//issues/4317#issuecomment-1725654764

For a mock helper i like to have an option to tell the node that these are its properties in a simple array format.

i experimented with using the serializer to archive this:

    public function createNode(
         ?NodeAggregateId $nodeAggregateId = null,
         SerializedPropertyValues|array|null $propertyValues = null,
     ): Node {
         if (is_array($propertyValues)) {
             $propertyValues = $this->propertyConverter->serializePropertyValues(
                 PropertyValuesToWrite::fromArray($propertyValues), // hacky as we are using this NodeModification command dto model
                 $nodeType // i also want to pass the nodeType here hacky, as it needs to have the correct property type defnitions
             );
         }
      # ...

      return new Node($propertyValues);

@github-project-automation github-project-automation bot moved this from Under Review 👀 to Done ✅ in Neos 9.0 Release Board Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants