-
-
Notifications
You must be signed in to change notification settings - Fork 438
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
WIP - Add wildcard to inject directive #2280
base: master
Are you sure you want to change the base?
Conversation
while (count($keys) > 1) { | ||
$key = array_shift($keys); | ||
|
||
if ($key === '*') { | ||
foreach ($argumentSet as $argument) { |
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 sure if we should throw an error with it's not an array here?
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 think so, yeah. *
makes no sense otherwise - it is not a valid identifier for a field.
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 updated it now. Not sure if i used the correct exception.
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.
Use DefinitionException
, in this case the error must be solved by the developer.
while (count($keys) > 1) { | ||
$key = array_shift($keys); | ||
|
||
if ($key === '*') { | ||
foreach ($argumentSet as $argument) { |
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 think so, yeah. *
makes no sense otherwise - it is not a valid identifier for a field.
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
type Mutation { | ||
updateUser(input: UpdateUser!): Task | ||
@update | ||
@inject(context: "user.id", name: "input.tasks.create.*.user_id") | ||
} |
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.
type Mutation { | |
updateUser(input: UpdateUser!): Task | |
@update | |
@inject(context: "user.id", name: "input.tasks.create.*.user_id") | |
} | |
type Mutation { | |
updateUser(input: UpdateUserInput!): Task | |
@update | |
@inject(context: "user.id", name: "input.tasks.create.*.user_id") | |
} | |
input UpdateUserInput { | |
id: ID! | |
tasks: UpdateTasksHasManyInput | |
} | |
input UpdateTasksHasManyInput { | |
create: [CreateTaskInput!] | |
} | |
input CreateTaskInput { | |
title: String! | |
} |
Makes me wonder if we should make the injection optional. In this example, I could see input.tasks.create
being optional, perhaps with input.tasks.update
being another possible argument that should also be injected into if present.
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 guess the same "issue" could exists with just dot notation. But it would make sense to discard it if an array does not exists at the asterisk segment.
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.
With just dot notation, we force the value at the defined position - and create intermediary inputs if necessary. I guess this could not be desired in every case, but it is how it currently works. At least it is well defined.
In the case of an array, we have the problem that it is less clear what to insert. What number of arguments at the desired position should be added? 1 or another number? I would argue it should always be 0 - that being equivalent to injecting nothing at all.
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
…ghthouse into inject-wildcard
I've taken a slightly different approach, just to make it easy. I've implemented I've also removed all empty arrays from the values. It caused a TODO
|
Not sure how to solve the unit tests. |
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.
Have you considered adding to ArgumentSetTest
? It should be pretty straightforward to test the many possible edge cases there and examine exactly what the result will be.
|
||
public function offsetExists(mixed $offset): bool | ||
{ | ||
$argument = $this; |
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 do not get why you keep aliasing $this
- why not just it directly?
$value = $this->value; | ||
|
||
return new \ArrayIterator($value); |
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.
$value = $this->value; | |
return new \ArrayIterator($value); | |
return new \ArrayIterator($this->value); |
Resolves #1206
I need some help finishing this. It throws a weird error, probably due to how I made the test. If you rundd($this->toArray)
on line 73 inArgumentSet
, you can see it injects the context with the correct value.Changes
You can now use wildcard in the inject directive.
Breaking changes
None I think?