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.

Sign up for GitHub

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

Jump to bottom

Fix display bugs in profiling #26731

Merged
merged 3 commits into from Dec 9, 2021
Merged

Fix display bugs in profiling #26731

merged 3 commits into from Dec 9, 2021

Conversation

lmeyer1
Copy link
Contributor

@lmeyer1 lmeyer1 commented Nov 24, 2021 •
edited by PierreRambaud

Questions Answers
Branch? develop
Description? Fixes display issues in Profiling mode
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #26718.
How to test? Enable Profiling mode and check output. On any page.
Possible impacts? Nothing to mention. Profiling is disabled by default.

This change is Reviewable

@lmeyer1 lmeyer1 requested a review from a team as a code owner November 24, 2021 14:31
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Nov 24, 2021
@kpodemski
Copy link
Contributor

Thanks @lmeyer1

@PrestaShop/prestashop-maintainers

I know it's not a regression, but can we move it to 1.7.8.x as this is for developers and it is quite safe?

matthieu-rolland
matthieu-rolland previously approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment •
edited

Choose a reason for hiding this comment

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

I agree with you @kpodemski , a third opinion would be nice

thank @lmeyer1 for your contribution 👍

kpodemski
kpodemski previously approved these changes Nov 25, 2021
View reviewed changes
@kpodemski
Copy link
Contributor

@marionf @MatShir

Can we push it for 1.7.8.x? It's only for developers, and these changes are helpful and safe.

@kpodemski kpodemski added the Waiting for PM Status: action required, waiting for product feedback label Nov 30, 2021
@marionf
Copy link
Contributor

marionf commented Dec 1, 2021

If this does not bring a risk of regression, we can make an exception

@marionf marionf removed the Waiting for PM Status: action required, waiting for product feedback label Dec 1, 2021
@eternoendless
Copy link
Member

This fixes a feature added in 1.7.8.0 (see #20673) so I guess it could be qualified as a regression?

eternoendless
eternoendless reviewed Dec 1, 2021
View reviewed changes
tools/profiling/templates/doubles.tpl Outdated
@@ -35,7 +35,7 @@
{sql_queries data=$nb}
</td>
<td class="pre">
<pre>{$q}</pre>
<pre>{$q nofilter}</pre>
Copy link
Member

Choose a reason for hiding this comment

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

WDYT @PierreRambaud ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eternoendless @PierreRambaud
$q always contains HTML, for highlighting the values replaces by XX. If you feel that nofilter could be dangerous, then we need another way to highlight the blue text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, PR updated. The nofilter is not really needed here, the code added is useless and I can add an XSS because of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

XSS inside the query coming from the software in the profiler displayed when the developer explicitly tells the application to do it? 🤔

PierreRambaud
PierreRambaud reviewed Dec 1, 2021
View reviewed changes
tools/profiling/Hook.php Outdated
@@ -63,6 +63,10 @@ public static function coreRenderWidget($module, $registeredHookName, $params)

$result = parent::coreRenderWidget($module, $registeredHookName, $params);

if ($registeredHookName === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid error inside the contact form page

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this would be best explained in a comment

PierreRambaud
PierreRambaud reviewed Dec 1, 2021
View reviewed changes
tools/profiling/templates/stopwatch.tpl
@@ -63,7 +63,11 @@
</td>
<td data-value="{$data['location']}">
<a href="javascript:void(0);" onclick="$('#callstack_{$callstack_md5}').toggle();">{$data['location']}</a>
<div id="callstack_{$callstack_md5}" style="display:none">{implode('<br>', $data['stack'])}</div>
<div id="callstack_{$callstack_md5}" style="display:none">
{foreach $data['stack'] as $stack}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to implode with nofilter when you can use foreach :)

Copy link
Member

Choose a reason for hiding this comment

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

You get a trailing <br> like this

PierreRambaud
PierreRambaud previously approved these changes Dec 1, 2021
View reviewed changes
@PierreRambaud PierreRambaud mentioned this pull request Dec 2, 2021
Closed
eternoendless
eternoendless approved these changes Dec 2, 2021
View reviewed changes
Copy link
Member

@eternoendless eternoendless left a comment

Choose a reason for hiding this comment

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

LGTM

@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Dec 3, 2021
@sarahdib sarahdib added the Waiting for dev Status: action required, waiting for tech feedback label Dec 3, 2021
@Progi1984 Progi1984 changed the title Fix display bugs in profiling. Fix display bugs in profiling Dec 6, 2021
@matks matks added the Key feature Notable feature to be highlighted label Dec 9, 2021
@atomiix
Copy link
Contributor

atomiix commented Dec 9, 2021

Before the PR:
Capture d’écran 2021-12-09 à 17 15 34

After the PR:
Capture d’écran 2021-12-09 à 17 14 38

This PR is QA ✔

@atomiix atomiix added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback labels Dec 9, 2021
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@atomiix atomiix merged commit ab8f416 into PrestaShop:develop Dec 9, 2021
@atomiix
Copy link
Contributor

atomiix commented Dec 9, 2021

Thanks @lmeyer1!

@atomiix atomiix added this to the 8.0.0 milestone Dec 9, 2021
@lmeyer1 lmeyer1 deleted the fix-profiling branch December 9, 2021 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpodemski kpodemski kpodemski left review comments

@eternoendless eternoendless eternoendless approved these changes

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@matthieu-rolland matthieu-rolland matthieu-rolland left review comments

Assignees
No one assigned
Labels
Bug fix Type: Bug fix develop Branch Key feature Notable feature to be highlighted QA ✔️ Status: check done, code approved
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Errors in output of Profiling mode
10 participants
@lmeyer1 @kpodemski @marionf @eternoendless @atomiix @prestonBot @PierreRambaud @matthieu-rolland @sarahdib @matks