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

Fix highlighting in database debug toolbar #5129

Merged
merged 6 commits into from
Oct 3, 2021

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Sep 25, 2021

Highlighting wasn't working for multiple-word terms, as they were searched in the SQL string with nbsp entities instead of regular spaces.

This code could be optimized, though it's not crucial as it's a debug function.

@vlakoff
Copy link
Contributor Author

vlakoff commented Sep 25, 2021

Added a commit implementing replacement using regexp with word boundaries, to avoid overlaps / false positives with strings such as "OR", "IN"…

@sfadschm
Copy link
Contributor

Can you add before/after schreenshots?

@vlakoff
Copy link
Contributor Author

vlakoff commented Sep 26, 2021

I confirmed the fix, but if ever you want screenshots of this toolbar content, you may have a look at #5136.

@sfadschm
Copy link
Contributor

I confirmed the fix, but if ever you want screenshots of this toolbar content, you may have a look at #5136.

There is unfortunately none of the key words changed here visible in the other PR. Would be great to see just one example what has been fixed in this PR, too.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Since this PR addresses an error in highlighting multiple-word identifiers, please add before/after screenshots, as @sfadschm suggested, that show there are indeed problems with the current and how this PR fixes them.

@vlakoff
Copy link
Contributor Author

vlakoff commented Sep 29, 2021

Before:
5129-before

After:
5129-after

@vlakoff
Copy link
Contributor Author

vlakoff commented Sep 29, 2021

Maybe we should add the ASC and DESC keywords?

@vlakoff
Copy link
Contributor Author

vlakoff commented Sep 29, 2021

Also, I'd suggest either highlighting commas in addition to parentheses, or not highlighting parentheses. Your pick?

@sfadschm
Copy link
Contributor

I think adding the keywords would be good, but would rather remove the parentheses. But lets wait for other calls.

@kenjis
Copy link
Member

kenjis commented Sep 29, 2021

I would like removing parentheses, adding ASC and DESC.

@paulbalandan
Copy link
Member

Let's highlight only keywords, leaving out parentheses and other punctuation marks.

The word boundaries were not working with them.

Also, this change makes the preg_quote() superfluous.
As discussed, it's better to highlight the keywords only.
@vlakoff
Copy link
Contributor Author

vlakoff commented Sep 30, 2021

  • Removed the highlighting of parentheses
  • Added ASC and DESC keywords
  • Reduced the number of commits by removing the commits related to the Rector warning

@paulbalandan paulbalandan merged commit 56add79 into codeigniter4:develop Oct 3, 2021
@paulbalandan
Copy link
Member

Thank you, @vlakoff

@vlakoff vlakoff deleted the debug-toolbar branch October 8, 2021 09:08
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.

4 participants