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

WIP - Add wildcard to inject directive #2280

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

andershagbard
Copy link
Contributor

@andershagbard andershagbard commented Jan 25, 2023

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Resolves #1206

I need some help finishing this. It throws a weird error, probably due to how I made the test. If you run dd($this->toArray) on line 73 in ArgumentSet, 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?

@spawnia spawnia added the enhancement A feature or improvement label Jan 26, 2023
@andershagbard andershagbard changed the title WIP - Add wildcard to inject directive Add wildcard to inject directive Jan 29, 2023
@andershagbard andershagbard marked this pull request as ready for review January 29, 2023 14:50
while (count($keys) > 1) {
$key = array_shift($keys);

if ($key === '*') {
foreach ($argumentSet as $argument) {
Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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) {
Copy link
Collaborator

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.

src/Schema/Directives/InjectDirective.php Outdated Show resolved Hide resolved
Comment on lines 1763 to 1767
type Mutation {
updateUser(input: UpdateUser!): Task
@update
@inject(context: "user.id", name: "input.tasks.create.*.user_id")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@andershagbard
Copy link
Contributor Author

andershagbard commented Feb 1, 2023

I've taken a slightly different approach, just to make it easy. I've implemented ArrayAccess and let data_set handle the logic.

I've also removed all empty arrays from the values. It caused a array to string conversion error later in the code. In this test: testWillRejectValuesNotPlacedAtArrayWithWildcardInjection

TODO

@andershagbard andershagbard marked this pull request as draft February 1, 2023 18:27
@andershagbard andershagbard changed the title Add wildcard to inject directive WIP - Add wildcard to inject directive Feb 1, 2023
src/Execution/Arguments/ArgumentSet.php Outdated Show resolved Hide resolved
src/Execution/Arguments/ArgumentSet.php Outdated Show resolved Hide resolved
@andershagbard
Copy link
Contributor Author

Not sure how to solve the unit tests.

Copy link
Collaborator

@spawnia spawnia left a 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;
Copy link
Collaborator

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?

Comment on lines +122 to +124
$value = $this->value;

return new \ArrayIterator($value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$value = $this->value;
return new \ArrayIterator($value);
return new \ArrayIterator($this->value);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wildcard path for @inject directive
2 participants