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

[Debug] Improve keyword highlighting and escaping of query strings. #5200

Merged
merged 9 commits into from
Oct 16, 2021
Merged

[Debug] Improve keyword highlighting and escaping of query strings. #5200

merged 9 commits into from
Oct 16, 2021

Conversation

sfadschm
Copy link
Contributor

@sfadschm sfadschm commented Oct 12, 2021

Description
This PR adds a few more composite query keywords to the highlight list for the debug toolbar.
I also decided to sort the keywords alphabetically, as it was a pain checking which ones are missing. (This is debatable of course.)

  • Deconstruct composite keywords.
  • Sort kewords alphabetically.
  • Modify regex pattern to ignore kewords within escaped string values ('').
  • [WIP] Add HTML escaping.

Depends on #5196.

Checklist:

  • Securely signed commits
  • Conforms to style guide

Before:
2

After:
1

@sfadschm
Copy link
Contributor Author

sfadschm commented Oct 13, 2021

Added a first basic test.
However, I am wondering what this

if (empty($this->finalQueryString)) {
$this->compileBinds(); // @codeCoverageIgnore
}

is trying to achieve? compileBinds does:
$sql = $this->finalQueryString;

So if finalQueryString was empty before, it will also be empty after, or do I miss something?

I think, debugToolbarDisplay() should rather call getQuery() to ensure the final query string is set before highlighting.

@paulbalandan
Copy link
Member

That part I added for the sake of prudence. Essentially that is useless because before calling getDebugToolbarDisplay() on Query we are sure that compileBinds was already called when the sql was executed. Added there only as guard.

I think with the proposed changes in #5127 we'll get a different approach on how to treat these things.

@sfadschm
Copy link
Contributor Author

That part I added for the sake of prudence. Essentially that is useless because before calling getDebugToolbarDisplay() on Query we are sure that compileBinds was already called when the sql was executed. Added there only as guard.

I think with the proposed changes in #5127 we'll get a different approach on how to treat these things.

Alright, did not see that #5127 already handles this. I will leave it like it is then.
However, depending on which PR is merged first, this line can be removed from the test then:

$query->getQuery();

@sfadschm sfadschm changed the title [Debug] Extend highlighted composite keywords. [Debug] Improve keyword highlighting and escaping of query strings. Oct 15, 2021
@kenjis
Copy link
Member

kenjis commented Oct 15, 2021

Add HTML escaping, please.
#5196 (comment)

@sfadschm
Copy link
Contributor Author

Added htmlspecialchars and a corresponding test. Will this do?
I chose to enable ENT_COMPAT, as we escape als values in the query string with single quotes.

@paulbalandan
Copy link
Member

How about using esc?

@sfadschm
Copy link
Contributor Author

How about using esc?

Seems to work in principle, too.
But also escapes the single quotes such that if we do esc then preg_replace, the regex doesn't work anymore to ignore keywords in values. If we do the other way around the highlighing gets escaped, too.

Possible workaround would be to change the regex from looking for ' to look for '.
Does not look to pretty to me, but I'm open for all ways 😄

@kenjis
Copy link
Member

kenjis commented Oct 15, 2021

The escaping way looks good to me.

@sfadschm
Copy link
Contributor Author

The escaping way looks good to me.

The esc() or htmlspecialchars() way?

@lonnieezell
Copy link
Member

Use esc() please.

@sfadschm
Copy link
Contributor Author

Should be working.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

This looks awesome! I haven't read all the code but seeing as the others have: I approve the screenshot. 🤣

Whoever merges please squash.

@paulbalandan paulbalandan merged commit 2007d64 into codeigniter4:develop Oct 16, 2021
@sfadschm sfadschm deleted the debug-query-highlight-keywords branch October 16, 2021 16:11
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.

5 participants