Skip to content

Conversation

@ralphvandenhoudt
Copy link
Contributor

No description provided.

@Kingdutch Kingdutch added the 4.x label Apr 15, 2023
Copy link
Contributor

@klausi klausi 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 to me, I have 2 minor comments that I will fix in a follow-up.

*/
public function resolve($path, RefinableCacheableDependencyInterface $metadata) {
$redirect = $this->redirectRepository ? $this->redirectRepository->findMatchingRedirect($path, []) : NULL;
public function resolve($path, $language, RefinableCacheableDependencyInterface $metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use type hints for all new parameters, in this case string.

* ),
* "language" = @ContextDefinition("string",
* label = @Translation("Language"),
* required = FALSE
Copy link
Contributor

Choose a reason for hiding this comment

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

we should specify a default value und here, which is Language::LANGCODE_NOT_SPECIFIED

@klausi klausi merged commit 24f86e2 into drupal-graphql:8.x-4.x Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants