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

Fixed Symfony error page #51

Closed
wants to merge 2 commits into from
Closed

Fixed Symfony error page #51

wants to merge 2 commits into from

Conversation

jazerix
Copy link

@jazerix jazerix commented Feb 24, 2022

This PR fixes the Symfony error page shown within Ignition, as occurring in issue #48.

This solution is most likely not how you want to solve this issue, but in that case, this can at least point you in the right direction.

I hope it helps :)

@jazerix
Copy link
Author

jazerix commented Feb 25, 2022

After some further testing, this does not seem to fix all issues. In general the Symfony error seems to be thrown during the json_encode of the stack trace :)

@jazerix
Copy link
Author

jazerix commented Feb 25, 2022

It looks like a cascade effect happening with json_encode. As it traverses the stacktraces array, it may find objects that implement the JsonSerializable method. If this method contains an error, then the exception is triggered anew within Ignition.

In my case I had buggy ResourceCollection which extends the JsonResource which implements the JsonSerializable interface.

@jazerix
Copy link
Author

jazerix commented Feb 25, 2022

The biggest issue currently and why my solution doesn't fully work, is that casted arrays aren't traversed. Unfortunately traversing those results in exceeding the frames, and looks like I'm hitting an endless loop somewhere in Laravel's framework (may be some cyclic references).

A better option could be to do a gradual json_encode using a try/catch along the way, to figure out which calls fails and then handle those.

@jazerix
Copy link
Author

jazerix commented Feb 25, 2022

 /**
 * @param array<string, mixed> $data
 * @return void
 */
private function recursiveConvertToArray(array &$data, $depth = 0) : void
{
    if ($depth == 8)
    {
        if (is_object($data))
            $data = null;
        return;
    }
    foreach ($data as $key => &$value)
    {
        if(is_object($value))
        {
            if (is_callable($value))
                continue;
            try
            {
                $value = (array)$value;

                $this->recursiveConvertToArray($value, $depth + 1);
            }
            catch (\Exception $e)
            {

            }
            continue;
        }
        if (is_array($value))
            $this->recursiveConvertToArray($value, $depth + 1);
    }
}

Something like this semi-solves the issue, it allows me to actually retrieve the json encoded string, but you also lose information once a certain depth is hit. Performance-wise this isn't great either.

@jazerix
Copy link
Author

jazerix commented Feb 25, 2022

This is probably the best solution I can come up with right now, without putting too much time into this 😄

/**
 * @param array<string, mixed> $data
 * @return void
 */
private function recursiveConvertToArray(array &$data,) : void
{
    foreach ($data as $key => &$value)
    {
        if(is_object($value))
        {
            try
            {
                $temp = json_encode($value);
                $value = json_decode($temp, true);

                $this->recursiveConvertToArray($value);
            }
            catch (\Exception $e)
            {
                $value = null;
            }
            continue;
        }
        if (is_array($value))
            $this->recursiveConvertToArray($value);
    }
}

I wouldn't call it pretty, but encoding and decoding only remove items that would also be removed when you do json_encode over the entire array.

This also removes calls that result in the inner exception being thrown, and timewise is much better than my proposal in the last comment.

@AlexVanderbist
Copy link
Member

Hi @jazerix, it seems like the bulk of this issue comes from the stack frame's arguments being included in the stacktrace (and some of those arguments not being serialisable). I think we're currently not using any of those arguments in Ignition, so getting rid of them in the serialised stacktrace might even be an easier solution. What do you think?

For reference: hardcoding arguments to be [] in here fixes the problem: https://github.com/spatie/flare-client-php/blob/main/src/Frame.php#L29

@moisish
Copy link

moisish commented Feb 25, 2022

It looks like a cascade effect happening with json_encode. As it traverses the stacktraces array, it may find objects that implement the JsonSerializable method. If this method contains an error, then the exception is triggered anew within Ignition.

This is the cause I have discovered as well.

@jazerix
Copy link
Author

jazerix commented Feb 25, 2022

Hi @jazerix, it seems like the bulk of this issue comes from the stack frame's arguments being included in the stacktrace (and some of those arguments not being serialisable). I think we're currently not using any of those arguments in Ignition, so getting rid of them in the serialised stacktrace might even be an easier solution. What do you think?

For reference: hardcoding arguments to be [] in here fixes the problem: https://github.com/spatie/flare-client-php/blob/main/src/Frame.php#L29

I honestly don't know, you most likely have a much better understanding of how the internal components work and communicate with each other 😄. I guess in the end it comes down to performance and what you want to preserve.

If you truly don't need the arguments anyway, then that is the obvious solution and definitely less costly than what I am proposing 😃

@AlexVanderbist
Copy link
Member

Hi again! It looks like removing attributes from the stack frames was all it took to fix the issue so I'm gonna close this PR for now. I really liked your approach to recursively add and check serialisable data before adding it to the report tho. If we ever need to add more dynamic data in the report again I'll re-open this 👍 Thanks for your time and great PR!

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