Skip to content

refactor(metadata): no default graphql operation name - #5348

Closed
soyuka wants to merge 2 commits into
api-platform:mainfrom
soyuka:refactor/graphql-metadata
Closed

refactor(metadata): no default graphql operation name#5348
soyuka wants to merge 2 commits into
api-platform:mainfrom
soyuka:refactor/graphql-metadata

Conversation

@soyuka

Copy link
Copy Markdown
Member

No description provided.

return [];
}

// TODO: it'd be nice to get rid of this hard coded name

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not using instanceof here?


if (!$useWrappedType) {
$wrappedOperationName = $operation instanceof Query ? $operationName : 'item_query';
$wrappedOperation = $resourceMetadataCollection->getGraphQlOperation(null, forceCollection: $operation instanceof Query);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why using a named argument here and not elsewhere?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I want to start using them I can revert for the codestyle

@alanpoulainalanpoulainJan 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally I think it's better like this indeed. But you should uniformize the call.

->withClass($type->getClassName())
->withShortName($operation?->getShortName() ?? (new \ReflectionClass($type->getClassName()))->getShortName())
->withDescription($operation?->getDescription());
$enumOperation = new Query(name: 'item_query', class: $type->getClassName(), shortName: $operation?->getShortName() ?? (new \ReflectionClass($type->getClassName()))->getShortName(), description: $operation?->getDescription());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

name is not necessary, isn't it?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

we do need a name

forceEager: $forceEager,
priority: $priority,
name: $name ?: 'item_query',
name: $name,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

because we want to remove logic from our operations

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, why not.

@stale

staleBot commented Mar 21, 2023

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@soyuka

Copy link
Copy Markdown
MemberAuthor

This is done in #5657

@soyukasoyuka closed this Jul 25, 2023
Sign up for freeto 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.

2 participants

@soyuka@alanpoulain