[FrameworkBundle] avoid tables to have apparently long blank line breaks and be too far appart for long nested array params #9720
Conversation
|
Can you paste before/after screenshots? |
|
|
@fabpot ^^ |
| @@ -198,7 +198,8 @@ protected function formatValue($value) | |||
| protected function formatParameter($value) | |||
| { | |||
| if (is_bool($value) || is_array($value) || (null === $value)) { | |||
| return json_encode($value); | |||
| $ellipsis = strlen(json_encode($value)) > 60; | |||
There was a problem hiding this comment.
You shouldnt encode twice here?
There was a problem hiding this comment.
what do you mean encode twice? I don't get it
There was a problem hiding this comment.
Line below json_encode($value) is called again.
There was a problem hiding this comment.
👍 json_encode should be done only once
There was a problem hiding this comment.
nevermind i see it 👶
|
done @fabpot , would be good if you are around working on symfony/symfony today to meet 👶 I am 5 minutes from the Westin 👶 |
| return json_encode($value); | ||
| $jsonString = json_encode($value); | ||
|
|
||
| return substr($jsonString, 0, 60).(strlen($jsonString) > 60 ? ' ...' : '' ); |
There was a problem hiding this comment.
i'd suggest
return strlen($jsonString) <= 60 ? $jsonString : substr_replace($jsonString, "...", 57)
to really limit the string to 60 chars (including the dots, that is)
There was a problem hiding this comment.
i will lookup how twig does it
|
@Burgov this proposal is inspired by twig's way of doing things so this should be better |
| return json_encode($value); | ||
| $jsonString = json_encode($value); | ||
|
|
||
| if (mb_strlen($jsonString) > 60) { |
There was a problem hiding this comment.
IIRC you can't depend on mbstring extension as this adds hard dependency on it, and it's not always available.
There was a problem hiding this comment.
We usually check for existence and use a regular function if mb_* is not there. Like here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Application.php#L674
|
ping @cordoval |
|
@fabpot i am still on transit in Heathrow London now, i will tackle things as soon as I reach a safe heaven |
|
ok finally taking this 👶 |
|
done @fabpot ^^ 👶 |
| } | ||
|
|
||
| return (string) $value; |
There was a problem hiding this comment.
It's probably safer to keep the cast to string.
|
@cordoval Can you take some time to finish this one, it's quite easy but should be done se that everything works in the same way with and without mbstring, which is not the case in the PR. |
|
@fabpot yeah i will finish this one for sure, just that niece and daughter are playing around on and off. 👶 👧 👧 |
… appart for long nested array params
…k line breaks and be too far appart for long nested array params (cordoval) This PR was submitted for the 2.4-dev branch but it was merged into the 2.4 branch instead (closes #9720). Discussion ---------- [FrameworkBundle] avoid tables to have apparently long blank line breaks and be too far appart for long nested array params | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | no | License | MIT | Doc PR | na This PR fixes the uncomfortable long parameter array dumps on the tables, if one needs to see the details of a parameter they can do so by inspecting the specific parameter rather than having all the info in the table and render it out of whack. Commits ------- a588ece avoid tables to have apparently long blank line breaks and be too far appart for long nested array params
This PR fixes the uncomfortable long parameter array dumps on the tables, if one needs to see the details of a parameter they can do so by inspecting the specific parameter rather than having all the info in the table and render it out of whack.