Skip to content

Change getData() so that it can be mixed instead of array #77

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

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

rcjsuen
Copy link
Contributor

@rcjsuen rcjsuen commented Aug 17, 2023

🔀 Pull Request

What does this PR do?

The constructor itself allows $data to be mixed and the functions in AccessHelper has various conditional statements to process the data depending on whether it is an array or an implementation of ArrayAccess. However, getData() is explicitly typed to return an array which means any attempts to get the source object after creating a JSONPath of it will cause a TypeError to be raised. Changing getData() to return mixed instead of array will help fix this inconsistency.

Thank you for reviewing and considering this change.

Test Plan

public function testChaining(): void
{
$container = new ArrayObject($this->getData('conferences'));
$jsonPath = new JSONPath($container);

diff --git a/tests/JSONPathArrayAccessTest.php b/tests/JSONPathArrayAccessTest.php
index 118b2aa..6066e16 100644
--- a/tests/JSONPathArrayAccessTest.php
+++ b/tests/JSONPathArrayAccessTest.php
@@ -28,6 +28,7 @@ class JSONPathArrayAccessTest extends TestCase
     {
         $container = new ArrayObject($this->getData('conferences'));
         $jsonPath = new JSONPath($container);
+        $jsonPath->getData();
 
         $teams = $jsonPath
             ->find('.conferences.*')

If you just add that one line the test will no longer run. This demonstrates the issue that I have described above where you can create a JSONPath but then calling getData() again to retrieve the original content will fail.

1) Flow\JSONPath\Test\JSONPathArrayAccessTest::testChaining
TypeError: Flow\JSONPath\JSONPath::getData(): Return value must be of type array, ArrayObject returned

/app/src/JSONPath.php:143
/app/tests/JSONPathArrayAccessTest.php:31

Related PRs and Issues

None that I am aware of.

The constructor itself allows $data to be mixed and the functions in
AccessHelper has various conditional statements to process the data
depending on whether it is an array or an implementation of ArrayAccess.
However, getData() is explicitly typed to return an array which means
any attempts to get the source object after creating a JSONPath of it
will cause a TypeError to be raised. Changing getData() to return mixed
instead of array will help fix this inconsistency.

Signed-off-by: Remy Suen <[email protected]>
@SoftCreatR SoftCreatR merged commit b82e7c5 into SoftCreatR:main Aug 17, 2023
@SoftCreatR
Copy link
Owner

Thanks :)

@rcjsuen rcjsuen deleted the align-getData-return-type branch August 17, 2023 20:03
@rcjsuen
Copy link
Contributor Author

rcjsuen commented Aug 17, 2023

@SoftCreatR Thank you for the speedy review. We were using the original implementation and encountered issues when converting from data() to getData() so we thought we would contribute a fix.

Could you create a new release with this fix at your earliest convenience? Thanks! 🙇

@SoftCreatR
Copy link
Owner

Done :octocat:

@rcjsuen
Copy link
Contributor Author

rcjsuen commented Aug 17, 2023

Thanks! I've updated my composer.json and our tests are passing now. :) Have a wonderful day/evening!

Sign up for free to 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