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

TASK: #899 Make Fizzle filter nested object properties #924

Merged

Conversation

joachimmathes
Copy link

@joachimmathes joachimmathes commented Apr 2, 2017

What I did
Implements #899. I extended the FlowQuery grammar to filter nested object properties.

How I did it
It was not possible to just extend the Identifier rule in AbstractParser.peg.inc since that led to a lot of regressions in the Eel parser. Thus, a new rule PropertyPath was necessary.

How to verify it
Unit-Tests in Neos.Eel run successfully.

@mention-bot
Copy link

@joachimmathes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @robertlemke, @skurfuerst and @gerhard-boden to be potential reviewers.

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Awesome <3

Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this, congrats to your first PR and warm welcome :) Apart from my one question this looks pretty good from reading!

@@ -12,7 +12,7 @@ namespace Neos\Eel;
* source code.
*/

require_once __DIR__ . '/../../../Resources/Private/PHP/php-peg/Parser.php';
require_once __DIR__ . '/../Resources/Private/PHP/php-peg/Parser.php';
Copy link
Member

Choose a reason for hiding this comment

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

Is this really correct? Not rather '/../PHP/php-peg/Parser.php'?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! :)

And thanks for pointing this out. When I generated AbstractParser.php from AbstractParser.peg.inc for the first time, i.e. with /../../../Resources ..., the Unit-Tests didn't run at all due to the wrong path. So I changed it to /../Resources/ ..., since the resulting AbstractParser.php is placed in Classes/. That's why I'm pretty sure this is correct. And not only the Unit-Tests have worked as expected by then, but also the respective line in AbstractParser.php hasn't changed in the diff.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, must be a misconception on my side and I guess I have a clue on what it is (this just being the code template for the actual AbstractParser, which resides somewhere else and not being actually executed itself)... well, no objections from my side then :)

Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification! :)

@@ -12,7 +12,7 @@ namespace Neos\Eel;
* source code.
*/

require_once __DIR__ . '/../../../Resources/Private/PHP/php-peg/Parser.php';
require_once __DIR__ . '/../Resources/Private/PHP/php-peg/Parser.php';
Copy link
Member

Choose a reason for hiding this comment

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

Hm, must be a misconception on my side and I guess I have a clue on what it is (this just being the code template for the actual AbstractParser, which resides somewhere else and not being actually executed itself)... well, no objections from my side then :)

@joachimmathes
Copy link
Author

The final parsers are created from the inc templates by Neos.Eel/Scripts/generateEelParser.sh.

Copy link
Contributor

@hlubek hlubek left a comment

Choose a reason for hiding this comment

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

Looks good by reading and running the tests. Thx for the PR ;)

But we need an accompanying change to update the usage of 'Identifer' in Neos.ContentRepository/Classes/Eel/FlowQueryOperations/ChildrenOperation.php:106 or we put an additional entry Identifier in the parsed result object to stay compatible (also with user operations). I'd tend to the latter one because it would not be breaking which the change would be otherwise for 3rd party operations.

Copy link
Contributor

@hlubek hlubek left a comment

Choose a reason for hiding this comment

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

I added the compatibility by adding an additional Identifier key in the result and checked that with the Neos.ContentRepository tests, works fine.

@joachimmathes
Copy link
Author

Lesson learned: Always check the NEOS development package when changing fundamental code in the Flow development package. :)

@kitsunet kitsunet merged commit 66e402b into neos:master Apr 7, 2017
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.

None yet

5 participants