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

Add support for transformation max depths #699

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

rubenvanassche
Copy link
Member

No description provided.

config/data.php Outdated
* You can disable this behaviour by setting this option to true which will return an
* empty array.
*/
'fail_when_max_transformation_depth_reached' => true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'fail_when_max_transformation_depth_reached' => true,
'throw_when_max_transformation_depth_reached' => true,

I think throw makes it more clear that there's an exception

Copy link
Member Author

Choose a reason for hiding this comment

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

Jup

@@ -22,6 +23,8 @@

class TransformedDataCollectableResolver
{
use ChecksTransformationDepths;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use ChecksTransformationDepths;
use ChecksTransformationDepth;

@@ -20,6 +21,8 @@

class TransformedDataResolver
{
use ChecksTransformationDepths;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is worth being in a trait

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to be somewhere, also makes it easy for other people who want to build casts based upon max depth checks to include it.

@rubenvanassche rubenvanassche marked this pull request as ready for review March 13, 2024 11:08
@rubenvanassche rubenvanassche merged commit 65865fc into main Mar 13, 2024
24 checks passed
@rubenvanassche rubenvanassche deleted the feature/transformation-depth branch March 13, 2024 11:09
@inmanturbo
Copy link

@rubenvanassche
It looks like this broke some packages that depend on laravel-data.

Spatie\LaravelData\Support\Transformation\TransformationContext::__construct(): Argument #11 ($throwWhenMaxDepthReached) must be of type bool, null given

It looks like updating the config to include 'throw_when_max_transformation_depth_reached' => true, will probably fix it at the app level, but I'm not sure how to fix the packages' ci.

Any advice?

@inmanturbo
Copy link

for now I'm adding

config()->set('data.throw_when_max_transformation_depth_reached', true);

to getEnvironmentSetUp() in the test case and it seems to work but it feels strange to have to explicitly set the config for a dependency to its default value. These packages don't have any published laravel-data configuration nor do they interact with its config.

@rubenvanassche
Copy link
Member Author

See the answer here: #731

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.

3 participants