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

Fix for translation not working in graphql #17

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

raivisdejus
Copy link
Contributor

<plugin name="graphql_init_translations"
type="ScandiPWA\CmsGraphQl\Plugin\InitGraphQlTranslations"/>
</type>
<type name="ScandiPWA\PersistedQuery\Plugin\PersistedQuery">

Choose a reason for hiding this comment

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

is this actually necessary?
processRequest call in PersistedQuery is initiated by aroundDispatch method, so I assume that beforeDispatch plugin on the GraphQl controller would be enough

not 100% sure, perhaps an interaction with the around plugin makes it necessary, but please double check

Copy link

@aleksandrsm aleksandrsm left a comment

Choose a reason for hiding this comment

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

see minor comments

& thanks for porting this fix to core PWA

@@ -0,0 +1,93 @@
<?php
/**
* ScandiPWA_CmsGraphQl

Choose a reason for hiding this comment

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

personally, I'd put it in PersistedQuery module rather than CmsGraphQl, because it already has plugins on the GrapQL controller

@raivisdejus
Copy link
Contributor Author

@aleksandrsm Please check my changes and comments

Copy link

@aleksandrsm aleksandrsm left a comment

Choose a reason for hiding this comment

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

Approved, but I still don't see the reason for plugin on ScandiPWA\PersistedQuery\Plugin\PersistedQuery - please double check that

@raivisdejus
Copy link
Contributor Author

processGraphqlRequest() is triggered in aroundPlugin and is to process cached queries. It is executed before the plugin on graphql controller
https://github.com/scandipwa/persisted-query/blob/master/src/Plugin/PersistedQuery.php#L227

So we need also this plugin to make translations work for all cases

@aleksandrsm
Copy link

Odd, as processRequest is triggered by aroundDispatch, which I thought would be executing after the beforeDispatch, which the new plugin adds on the base class. Not 100% sure on how the before-around interaction on plugins work, specially with multiple plugins involved.
I guess it doesn't work when the beforeDispatch is defined on the base class but not the PersistedQuery plugin.

Keep as is if it is necessary, and thanks for checking.

@carinadues carinadues merged commit f3a1ea4 into scandipwa:master Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants