-
Notifications
You must be signed in to change notification settings - Fork 17
IBX-9840: Implemented service fetching content fields from expression and validating if field is within expression #1500
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
Conversation
|
0da192d
to
78947c5
Compare
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.
Do we need to expose a method to validate an expression? Will it be possible to edit it by hand?
$this->expectSlash(); | ||
$fieldTokens = $this->parseSection(); | ||
} else { | ||
throw new RuntimeException('Invalid expression, expected one or two T_SLASH delimiters.'); |
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.
WDYT about throwing a dedicated exception, like SyntaxError/Exception? And what about catching it, should it be in service or by client code? T_SLASH or "/" like here https://github.com/ibexa/admin-ui/pull/1500/files#diff-74127d24456a18e891c75260b495952ea2359a27853f3bea40933252aa41f3ffR133?
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've added the Ibexa\AdminUi\Exception\FieldTypeExpressionParserException
and imo it should be catched by client code to throw 4xx (when there is one)
|
||
$contentTypes = $this->resolveContentTypes($extractedMetadata); | ||
|
||
return $this->mergeFieldIds($extractedMetadata[2], $contentTypes); |
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.
Instead of fetching by indexes, is it worth creating a structure for metadata?
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.
Do you mean by that that public function parseExpression(string $expression): array
should give us some kind of structured object instead?
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.
yes, like ExpresionMetadata or sth
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.
please see a5bbca7
7079418
to
0e1b4d7
Compare
|
a5bbca7
to
3cf3ce8
Compare
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.
Could you add tests for:
- leading slash
- trailing slash
- empty {} lists
- missing closing brace
); | ||
} | ||
|
||
return $finalFieldIds; |
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.
Is it possible to have duplicated IDs because of overlapping content types?
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.
Not anymore, please see 14228d3
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 would like to have a more specific description of what the intended use for this feature is. I know what it does (or have a rough idea), but I don't get how it will be utilized?
@Steveb-p it's used in selection of fields in taxonomy suggestions AI action config: https://www.figma.com/design/WkIyhysX3ZGSRwwxnPcFXE/-AI-Features--AI?node-id=9016-163408&t=oh4c1TG5zkMCmlFJ-0 |
@barw4 these? ![]() |
@Steveb-p yes, the ones at the bottom. Content Type Group can have "All" option selected - meaning we will store "*" in the db or just identifiers, and so on and so on |
Why are we building a full expression syntax if all we need is to store values selected in those forms? I don't intend to discard this work, but from my perspective it's very specific "language" (3 elements separated by You could even use Serializer if you want to transform value back and forth. This feels overengineered to be honest. Adds complexity, limitations to this construct (certain symbols can't be used) and hides the structure behind expression. I am sorry, but in my opinion it should not be merged 😞 |
@mikadamczyk I've added few test cases inside the |
14228d3
to
4fe64dd
Compare
4fe64dd
to
eb30ab2
Compare
|
Description:
The parser will parse the following expressions:
and fetch asked field definitions, then provide an output in the form of a list that consists of field definition ids.
Example of usage:
will result in:
There is also a possibility to check whether a
FieldDefinition
is within a given expression. The API reference is as follows:ContentTypeFieldsByExpressionServiceInterface
service:For QA:
Documentation: