Add a "run query" option in the profiler #1826
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ostrolucky
requested changes
Oct 16, 2024
| @@ -90,6 +90,13 @@ | |||
| connectionName: request.query.get('connection'), | |||
| query: request.query.get('query') | |||
| })) }} | |||
| {% elseif 'run' == page %} | |||
There was a problem hiding this comment.
I don't like that most of the lines are duplicated now. Something like this could be done instead:
{% if page in ['explain', 'page'] %}
{{ render(controller('Doctrine\\Bundle\\DoctrineBundle\\Controller\\ProfilerController::' ~ page ~ 'Action', {
token: token,
panel: 'db',
connectionName: request.query.get('connection'),
query: request.query.get('query')
})) }}
Comment on lines
+86
to
+95
| /** | ||
| * Renders the profiler panel for the given token. | ||
| * | ||
| * @param string $token The profiler token | ||
| * @param string $connectionName | ||
| * @param int $query | ||
| * | ||
| * @return Response A Response instance | ||
| */ | ||
| public function runAction($token, $connectionName, $query) |
There was a problem hiding this comment.
This is old school PHP 5 style code, we don't use it for new stuff
Suggested change
| /** | |
| * Renders the profiler panel for the given token. | |
| * | |
| * @param string $token The profiler token | |
| * @param string $connectionName | |
| * @param int $query | |
| * | |
| * @return Response A Response instance | |
| */ | |
| public function runAction($token, $connectionName, $query) | |
| public function runAction(string $token, string $connectionName, int $query): Response |
Comment on lines
-104
to
+111
| .sql-runnable button { align-self: end; } | ||
| .sql-runnable.sf-toggle-content.sf-toggle-visible { display: flex; flex-direction: row; align-items: center} | ||
| .sql-runnable pre { flex-grow: 1} |
There was a problem hiding this comment.
I'm not a fan of this design change. Let's have buttons next to each other, instead on top of each other
| </div> | ||
| {% endif %} | ||
|
|
||
| {% if query.explainable %} | ||
| <div id="explain-{{ i }}-{{ loop.parent.loop.index }}" class="sql-explain"></div> | ||
| {% endif %} | ||
|
|
||
| <div id="run-{{ i }}-{{ loop.parent.loop.index }}" class="sql-explain"></div> |
There was a problem hiding this comment.
shouldn't this be wrapped in {% if query.runnable %} condition? Or you believe the previous condition is not necessary?
Comment on lines
+483
to
+509
| function run(link) { | ||
| "use strict"; | ||
|
|
||
| var targetId = link.getAttribute('data-target-id'); | ||
| var targetElement = document.getElementById(targetId); | ||
|
|
||
| if (targetElement.style.display != 'block') { | ||
| if (targetElement.getAttribute('data-sfurl') !== link.href) { | ||
| fetch(link.href, { | ||
| headers: {'X-Requested-With': 'XMLHttpRequest'} | ||
| }).then(async function (response) { | ||
| targetElement.innerHTML = await response.text() | ||
| targetElement.setAttribute('data-sfurl', link.href) | ||
| }, function () { | ||
| targetElement.innerHTML = 'An error occurred while loading the query explanation.'; | ||
| }) | ||
| } | ||
|
|
||
| targetElement.style.display = 'block'; | ||
| link.innerHTML = 'Hide query result'; | ||
| } else { | ||
| targetElement.style.display = 'none'; | ||
| link.innerHTML = 'Run query'; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Please parametrize the function that was copied instead of copying whole thing and just changing few lines
Comment on lines
+1
to
+23
| {% if result is not empty %} | ||
| <table> | ||
| <thead> | ||
| <tr> | ||
| {% for key, value in result[0] %} | ||
| <th>{{ key }}</th> | ||
| {% endfor %} | ||
| </tr> | ||
|
|
||
| </thead> | ||
| <tbody> | ||
| {% for row in result %} | ||
| <tr> | ||
| {% for key, value in row %} | ||
| <td>{{ value }}</td> | ||
| {% endfor %} | ||
| </tr> | ||
| {% endfor %} | ||
| </tbody> | ||
| </table> | ||
| {% else %} | ||
| Result is empty | ||
| {% endif %} |
There was a problem hiding this comment.
this was also copied (explain.html.twig), please modularize through block/embed it if you believe some stuff have to be different
ostrolucky
force-pushed
the
2.13.x
branch
2 times, most recently
from
01c7700 to
7bfb8e8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inside the "view runnable query" panel, a run button enables running the query and checking the results just like "explain query" does