Skip to content

Conversation

Kingdutch
Copy link
Contributor

#878 made some changes already but left a reset(array_keys()) call behind which on PHP 7.2 provides a notice when opening the ServerForm.

Notice: Only variables should be passed by reference in Drupal\graphql\Form\ServerForm->form() (line 95 of modules/contrib/graphql/src/Form/ServerForm.php).
Drupal\graphql\Form\ServerForm->form(Array, Object) (Line: 149)
[...]

The attached commit solves this by introducing a temporary variable. Based on feedback from PHPMD I've also simplified the expression and added a comment for people who may not be very familiar with the null-coalescing operator.

`reset` only takes variables so we can not pass the result of
`array_keys` directly but have to use an intermediate variable.
`array_key_exists('schema', $input) ? $input['schema'] : NULL` is equal
to `$input['schema'] ?? NULL` on PHP versions that support the
null-coalescing operator. Since we already use this elsewhere we already
require such PHP versions.

This allows us to substitute the simplified one-use variable leading to
`$input['schema'] ?? NULL ?? $server->get('schema')`. The expression `??
NULL ??` can be safely transformed to `??` to result in the simplified
expression in this commit.
@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

❗ No coverage uploaded for pull request base (8.x-4.x@b235e07). Click here to learn what that means.
The diff coverage is 42.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##             8.x-4.x    #1080   +/-   ##
==========================================
  Coverage           ?   51.06%           
  Complexity         ?      674           
==========================================
  Files              ?      118           
  Lines              ?     1837           
  Branches           ?        0           
==========================================
  Hits               ?      938           
  Misses             ?      899           
  Partials           ?        0           
Impacted Files Coverage Δ Complexity Δ
src/Access/ExplorerAccessCheck.php 0.00% <0.00%> (ø) 2.00 <2.00> (?)
src/Access/VoyagerAccessCheck.php 0.00% <0.00%> (ø) 2.00 <2.00> (?)
src/Cache/Context/StaticCacheContext.php 0.00% <0.00%> (ø) 4.00 <4.00> (?)
src/Controller/SubrequestExtractionController.php 0.00% <0.00%> (ø) 1.00 <0.00> (?)
src/Form/PersistedQueriesForm.php 0.00% <0.00%> (ø) 18.00 <18.00> (?)
src/GraphQL/Buffers/BufferBase.php 89.65% <ø> (ø) 11.00 <0.00> (?)
src/GraphQL/Buffers/EntityBuffer.php 80.76% <0.00%> (ø) 7.00 <0.00> (?)
src/GraphQL/Buffers/EntityRevisionBuffer.php 0.00% <0.00%> (ø) 7.00 <7.00> (?)
src/GraphQL/Resolver/Condition.php 0.00% <0.00%> (ø) 8.00 <8.00> (?)
src/GraphQL/Resolver/Context.php 0.00% <0.00%> (ø) 5.00 <5.00> (?)
... and 91 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b235e07...c06defc. Read the comment docs.

Copy link

@BalintCsuthy BalintCsuthy left a comment

Choose a reason for hiding this comment

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

nice code improvement

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.

Thanks, this is now the second time the server form has PHP warnings in interesting ways on different PHP versions. Before we do any further changes we need to come up with test coverage for the server form. I think a simple browser test to visit the form and configure an example server should be enough?

Can you add a test case?

@Kingdutch
Copy link
Contributor Author

I don't currently have time to write a test for this, so would hope either someone else can do that or it be moved to a follow-up :)

@klausi klausi merged commit 668b413 into drupal-graphql:8.x-4.x Nov 28, 2020
@klausi
Copy link
Contributor

klausi commented Nov 28, 2020

While working on PHPStan level 5 it also detected this error, so will merge this without test since we will have coverage by PHPStan.

@Kingdutch Kingdutch deleted the bugfix/variable-by-reference branch November 28, 2020 07:43
klausi added a commit to klausi/graphql that referenced this pull request Sep 21, 2023
…l-graphql#1080)

* Fix notice about variables passed by reference

`reset` only takes variables so we can not pass the result of
`array_keys` directly but have to use an intermediate variable.

* Merge variable fallback expressions

`array_key_exists('schema', $input) ? $input['schema'] : NULL` is equal
to `$input['schema'] ?? NULL` on PHP versions that support the
null-coalescing operator. Since we already use this elsewhere we already
require such PHP versions.

This allows us to substitute the simplified one-use variable leading to
`$input['schema'] ?? NULL ?? $server->get('schema')`. The expression `??
NULL ??` can be safely transformed to `??` to result in the simplified
expression in this commit.

Co-authored-by: Klaus Purer <[email protected]>
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