Drupal Practice ref added#1074
Conversation
klausi
left a comment
klausi
left a comment
There was a problem hiding this comment.
Nice, looks like we don't need to fix too much. Some small things to change.
| <rule ref="Drupal"></rule> | ||
| <rule ref="DrupalPractice"> | ||
| <!-- Here Variables would have had to been removed--> | ||
| <exclude-pattern>src/Plugin/GraphQL/DataProducer/DataProducerProxy.php</exclude-pattern> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| dependencies: | ||
| - graphql:graphql | ||
| - node:node | ||
| core_version_requirement: 8.x |
There was a problem hiding this comment.
should be core_version_requirement: ^8 || ^9 like in the main graphql.info.yml file
| '#type' => 'checkboxes', | ||
| '#required' => FALSE, | ||
| '#title' => t('Enabled extensions'), | ||
| '#title' => $this->t('Enabled extensions'), |
There was a problem hiding this comment.
not sure if this class has the StringTranslationTrait so that this method is available.
Can you test this in local Drupal install by going to the graphql user interface?
There was a problem hiding this comment.
Ok figured out that it doesn't have that method
There was a problem hiding this comment.
Ok, then we need to add use StringTranslationTrait; to this class.
See for example src/PermissionProvider.php which implements the trait and also has the namespace use statement for it.
| dependencies: | ||
| - graphql:graphql | ||
| - node:node | ||
| core_version_requirement: 8.x |
Codecov Report
@@ Coverage Diff @@
## 8.x-4.x #1074 +/- ##
==========================================
Coverage 50.51% 50.51%
Complexity 674 674
==========================================
Files 118 118
Lines 1827 1827
==========================================
Hits 923 923
Misses 904 904
Continue to review full report at Codecov.
|
|
Looks like you mixed in the phstan changes here, let's do that in a separate pull request. so we only need to add the StringTranslationTrait for this one class and then we should be good here. |
| <rule ref="Drupal"></rule> | ||
| <rule ref="DrupalPractice"></rule> | ||
|
|
||
| <rule ref="DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable"> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * @param \Drupal\graphql\Entity\ServerInterface $graphql_server | ||
| * The server instance. | ||
| * @param \GraphQL\Server\OperationParams|\GraphQL\Server\OperationParams[] $operations | ||
| * @param \Drupal\graphql\Controller\OperationParams|\Drupal\graphql\Controller\OperationParams[] $operations |
There was a problem hiding this comment.
we should not change this type, tests are failing. We should keep \GraphQL\Server\OperationParams.
| * @throws \Exception | ||
| */ | ||
| public function handleRequest(ServerInterface $graphql_server, $operations) { | ||
| public function handleRequest(ServerInterface $graphql_server, OperationParams $operations) { |
There was a problem hiding this comment.
we should not change this function signature.
|
Travis CI is green, we don't care about the code coverage part. Thank you! |
2 participants
DrupalPractice Reference added