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

FEATURE: `DataStructure` assumes nesting for subkeys without specified type #2729

Conversation

@mficzel
Copy link
Member

mficzel commented Oct 14, 2019

When a DataStructure is rendered for nested keys that have no specified type the fusion type DataStructure is assumed. This makes it much easier to describe larger data structure in fusion for instance to create json ld structures or define complex mappings.

…btypes without specified type

When a `DataStructure` is rendered for nested keys that have no specified type the fusion  type `DataStructure` is assumed.
This makes it much easier to describe larger data structure in fusion for instance to create json ld structures.

```
jsonLd = Neos.Fusion:DataStructure {
        '@context' = "http://schema.org"
        '@type' = "Organization"
        name = "Neos"
        url = "http://www.neos.io"
        contactPoint {
            0 {
                '@type' = "ContactPoint"
                contactType = "Chat"
                url = "https://slack.neos.io"
            }
            1 {
                '@type' = "ContactPoint"
                contactType = "Forum"
                url = "https://discuss.neos.io"

            }
        }
    }
```
Which then can be rendered in afx:
```
renderer = afx`<script type="application/ld+json">{Json.stringify(props.jsonLd)}</script>`
```
@mficzel mficzel requested a review from Sebobo Oct 14, 2019
@mficzel mficzel changed the title FEATURE: `DataStructure` assumes nested `DataStructure` for nested subkeys without specified type FEATURE: `DataStructure` assumes nesting for subkeys without specified type Oct 14, 2019
@mficzel mficzel changed the title FEATURE: `DataStructure` assumes nesting for subkeys without specified type WIP: FEATURE: `DataStructure` assumes nesting for subkeys without specified type Oct 15, 2019
@mficzel

This comment has been minimized.

Copy link
Member Author

mficzel commented Oct 17, 2019

An additional approach for efficient definition of large jason structures would be a json dsl as described here https://gist.github.com/mficzel/15be6b92dd0f90cb6029e34d429e3841 '

However this would not make this pr obsolete.

@Sebobo
Sebobo approved these changes Oct 22, 2019
Copy link
Member

Sebobo left a comment

Looks good :)
Tiny bit of magic but doesn't feel wrong to do it.

But would be good to have more input from others.

@jonnitto jonnitto added this to In progress in Neos 5.1 & Flow 6.1 Release Board via automation Nov 6, 2019
@mficzel mficzel moved this from In progress to Not this time in Neos 5.1 & Flow 6.1 Release Board Nov 26, 2019
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 29, 2019

Oh, I love that one! Is there a chance that we still get it into 5.1?

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 29, 2019

@mficzel what's WIP about this one?

@mficzel

This comment has been minimized.

Copy link
Member Author

mficzel commented Nov 29, 2019

@bwaidelich i like the feature but am not entirely sure this is best possible implementation already. This list of conditions is a bit ugly and i would really like a short discussion wether this is the best way to do this.

&& !array_key_exists('__objectType', $this->properties[$key])
&& !array_key_exists('__eelExpression', $this->properties[$key])
&& !array_key_exists('__value', $this->properties[$key])
&& !array_key_exists('__stopInheritanceChain', $this->properties[$key])

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Nov 29, 2019

Member

I think that could be replaced by sth like:

if (is_array($this->properties[$key]) && empty(array_intersect_key($keys, $this->properties[$key]))

where $keys contains the reserved words.

But personally I'm more concerned about that taking place within the try/catch block.
What about this:

private const RESERVED_KEYS = ['__objectType' => true, '__eelExpression' => true, '__value' => true, '__stopInheritanceChain' => true];

  // ...

            $path = $this->isUntypedProperty($this->properties[$key]) ? $key . '<Neos.Fusion:DataStructure>' : $key;
            try {
                $value = $this->fusionValue($path);
            } catch (\Exception $e) {
                $value = $this->runtime->handleRenderingException($this->path . '/' . $key, $e);
            }

  // ...

    private function isUntypedProperty($property): bool
    {
        if (!is_array($property)) {
            return false;
        }
        return array_intersect_key(self::RESERVED_KEYS, $property) === [];
    }

?

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Nov 29, 2019

Member

I just realized, we could also refer to the reserved keys like this:

return array_intersect_key(array_flip(Fusion\Core\Parser::$reservedParseTreeKeys), $property) === [];
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 29, 2019

@mficzel Thanks for clarifying. I left comments with suggestions. If you agree I can take care of a follow-up commit

@mficzel

This comment has been minimized.

Copy link
Member Author

mficzel commented Nov 29, 2019

@bwaidelich totally fine go for it. There also was a canRender method somewhere in the fusion runtime that might be a good point to ask aswell.

@bwaidelich bwaidelich moved this from Not this time to In progress in Neos 5.1 & Flow 6.1 Release Board Nov 30, 2019
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 30, 2019

I added an issue for this in order to simplify release management and to collect higher-level details for the release notes: #2794

@bwaidelich bwaidelich removed this from In progress in Neos 5.1 & Flow 6.1 Release Board Nov 30, 2019
@bwaidelich bwaidelich changed the title WIP: FEATURE: `DataStructure` assumes nesting for subkeys without specified type FEATURE: `DataStructure` assumes nesting for subkeys without specified type Nov 30, 2019
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 30, 2019

@mficzel What do you think about that?

@bwaidelich bwaidelich dismissed their stale review Dec 2, 2019

ah, failing tests.

@mficzel

This comment has been minimized.

Copy link
Member Author

mficzel commented Dec 2, 2019

@bwaidelich i like this approach, really clean and simple now.

The only problem that might be caused is that anything that is not renderable will now be interpreted as DataStructure. If this leads to hard to understand errors we should adjust this in future but for now i think this is totally fine.

Offcourse the tests have to be green but the current problems seem unrelated.

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Dec 2, 2019

The only problem that might be caused is that anything that is not renderable will now be interpreted as DataStructure

Good call, but it's only true for objects within a DataStructure. And that should run into

            } catch (\Exception $e) {
                $value = $this->runtime->handleRenderingException($this->path . '/' . $key, $e);
            }

part then.. I'll add a test for this case and see how the result differs from the current behavior

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Dec 2, 2019

OK, the Runtime::canRender() approach didn't work, as it would turn some/path<NonExistingObject> into some/path<Neos.Fusion:DataStructure> of course. I reworked it now as suggested earlier, and that seems to be backwards compatible

@Sebobo

This comment has been minimized.

Copy link
Member

Sebobo commented Dec 2, 2019

But tests seem to be broken now

@@ -41,12 +41,12 @@ public function positionalSubElements()
return [
[
'Position end should put element to end',
['second' => ['__meta' => ['position' => 'end']], 'first' => []],
['second' => ['__meta' => ['position' => 'end']], 'first' => ['__meta' => []]],

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Dec 2, 2019

Member

These fixtures are a bit weird now, but IMO the whole unit test is a bit weird already because it exposes quite a lot of internal logic.. this could be easily covered by a functional test with real fusion fixture.

@Sebobo
Sebobo approved these changes Dec 3, 2019
@mficzel

This comment has been minimized.

Copy link
Member Author

mficzel commented Dec 3, 2019

I like this, let us merge it.

@mficzel

This comment has been minimized.

Copy link
Member Author

mficzel commented Dec 3, 2019

And yes we eventually need a way to write functional fusion tests in fusion itself.

@davidspiola davidspiola merged commit b3e2a4d into neos:master Dec 3, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Dec 3, 2019

we eventually need a way to write functional fusion tests in fusion itself

mh, interesting idea. And should be doable with some custom implementation...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.