Skip to content

Conversation

@WendellAdriel
Copy link
Contributor

This is basically a syntax sugar for:

$collection = collect([1,2,3]); $collection->toJson(JSON_PRETTY_PRINT);

To be called like this:

$collection = collect([1,2,3]); $collection->toPrettyJson();
@taylorotwell
Copy link
Member

If we were to do this (which I'm not entirely opposed to tbh) we may want to look at other spots we have toJson as a method and bring parity to those places too.

@taylorotwell taylorotwell marked this pull request as draft August 20, 2025 18:57
@WendellAdriel
Copy link
Contributor Author

I can work on it!

Should I also update the Jsonable interface?

I think this would be a breaking change tho if we update the interface.

I can either not update the interface or target this to master for the 13.x version.

Which approach do you want me to follow @taylorotwell?

@rodrigopedra
Copy link
Contributor

Should I also update the Jsonable interface?

Many projects and packages implement that interface.

There is a large impact on migrating all this code just to add a wrapper around another method with a fixed flag.

I like the addition to the Collection class and potentially to other spots. But I would vote against adding it to the interface.

@WendellAdriel
Copy link
Contributor Author

@rodrigopedra Yeah, I think it's not worth it.

@WendellAdriel
Copy link
Contributor Author

@taylorotwell I added the toPrettyJson to all places that have a toJson method, except into the Illuminate\Contracts\Support\Jsonable interface because that would be a breaking change.

I see that some tests failed, but are not related with the changes, so IDK what to do in these cases.

@WendellAdriel WendellAdriel marked this pull request as ready for review August 21, 2025 10:16
@rodrigopedra
Copy link
Contributor

@WendellAdriel the failing test seems related to the changes:

Method jsonSerialize() from Mockery_193_Illuminate_Http_Resources_Json_JsonResource should be called

Note that the error is listed before the risky test listing.

From https://github.com/laravel/framework/actions/runs/17123763507/job/48570565794?pr=56697#logs

Time: 01:25.939, Memory: 350.00 MB There was 1 error: 1) Illuminate\Tests\Integration\Auth\ApiAuthenticationWithEloquentTest::testAuthenticationViaApiWithEloquentUsingWrongDatabaseCredentialsShouldNotCauseInfiniteLoop Mockery\Exception\InvalidCountException: Method jsonSerialize(<Any Arguments>) from Mockery_193_Illuminate_Http_Resources_Json_JsonResource should be called  exactly 1 times but called 2 times. /home/runner/work/framework/framework/vendor/mockery/mockery/library/Mockery/CountValidator/Exact.php:32 /home/runner/work/framework/framework/vendor/mockery/mockery/library/Mockery/Expectation.php:739 /home/runner/work/framework/framework/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:202 /home/runner/work/framework/framework/vendor/mockery/mockery/library/Mockery/Container.php:583 /home/runner/work/framework/framework/vendor/mockery/mockery/library/Mockery/Container.php:519 /home/runner/work/framework/framework/vendor/mockery/mockery/library/Mockery.php:176 /home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/InteractsWithMockery.php:22 /home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/ApplicationTestingHooks.php:127 /home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/Testing.php:70 /home/runner/work/framework/framework/src/Illuminate/Collections/helpers.php:236 /home/runner/work/framework/framework/vendor/orchestra/sidekick/src/functions.php:87 /home/runner/work/framework/framework/src/Illuminate/Collections/helpers.php:236 /home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/Testing.php:98 /home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/TestCase.php:61 -- There was 1 risky test: 1) Illuminate\Tests\Integration\Auth\ApiAuthenticationWithEloquentTest::testAuthenticationViaApiWithEloquentUsingWrongDatabaseCredentialsShouldNotCauseInfiniteLoop * Test code or tested code did not remove its own error handlers * Test code or tested code did not remove its own exception handlers /home/runner/work/framework/framework/tests/Integration/Auth/ApiAuthenticationWithEloquentTest.php:38 ERRORS! Tests: 11934, Assertions: 36005, Errors: 1, Warnings: 1, Skipped: 477, Risky: 1.
@WendellAdriel
Copy link
Contributor Author

@rodrigopedra thanks, but I don't see how this could be related TBH, since I didn't touch the jsonSerialize() method. But if you find the root cause, please let me know. I'll keep trying to find where the error is.

@WendellAdriel WendellAdriel changed the title [12.x] Add toPrettyJson method to collections [12.x] Add toPrettyJson method Aug 21, 2025
@rodrigopedra
Copy link
Contributor

@WendellAdriel you are right.

I saw Mockery_193_Illuminate_Http_Resources_Json_JsonResource and thought it was related.

Sorry =/

@WendellAdriel
Copy link
Contributor Author

@rodrigopedra no worries, thanks for checking, because it could be something that I was not seeing! 💪

@taylorotwell
Copy link
Member

Looks like we have a test failing.

@taylorotwell taylorotwell marked this pull request as draft August 21, 2025 14:45
@WendellAdriel
Copy link
Contributor Author

@taylorotwell I thought the error was not related to these changes since I didn't touch the jsonSerialize() method, but I'll take another look to see if I find what's causing this.

@WendellAdriel
Copy link
Contributor Author

I'm validating it because when I run the test for this method, this file, for the tests/Integration/Auth folder I don't get any errors:

image

I also run all the integration tests from tests/Integration and it's not throwing any errors for me.
However, when I run the full test suite, I saw that it is failing.
I'm checking now

@WendellAdriel
Copy link
Contributor Author

I found what was causing it!
Now it's ok to review!
Sorry for the mess 😅

@WendellAdriel WendellAdriel marked this pull request as ready for review August 21, 2025 15:25
@taylorotwell taylorotwell merged commit 3a66665 into laravel:12.x Aug 21, 2025
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants