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

Refactor Database Collector display #5553

Merged
merged 3 commits into from
Jan 7, 2022

Conversation

paulbalandan
Copy link
Member

Description
Fixes #5539
Supersedes and closes #5543

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan paulbalandan added bug Verified issues on the current code behavior or pull requests that will fix them refactor Pull requests that refactor code labels Jan 7, 2022
@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

This is not because of this PR, but I got JS error and when I click the Toolbar icon, it does not show up (remain closed).

Uncaught TypeError: this.toolbar is undefined
    showTab http://localhost:8080/ line 37 > injectedScript:92
    createListeners http://localhost:8080/ line 37 > injectedScript:64
    init http://localhost:8080/ line 37 > injectedScript:21
    onreadystatechange http://localhost:8080/index.php?debugbar:49
    loadDoc http://localhost:8080/index.php?debugbar:14
    EventListener.handleEvent* http://localhost:8080/index.php?debugbar:1
line 37 > injectedScript:92:14
    showTab http://localhost:8080/ line 37 > injectedScript:92
    (Async: EventListener.handleEvent)
    createListeners http://localhost:8080/ line 37 > injectedScript:64
    init http://localhost:8080/ line 37 > injectedScript:21
    onreadystatechange http://localhost:8080/index.php?debugbar:49
    (Async: EventHandlerNonNull)
    loadDoc http://localhost:8080/index.php?debugbar:14
    (Async: EventListener.handleEvent)
    <anonymous> http://localhost:8080/index.php?debugbar:1

@paulbalandan
Copy link
Member Author

This is not because of this PR, but I got JS error and when I click the Toolbar icon, it does not show up (remain closed).

Mine, too.

Uncaught TypeError: Cannot read properties of undefined (reading 'querySelectorAll')
at HTMLAnchorElement.showTab (:88:27)

@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

The bug comes from #5544.

@kenjis kenjis added the hotfix label Jan 7, 2022
@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

@paulbalandan I rebased this branch on my local.

Two things:

  • line number color is gone
  • the buttom line of backtrace is changed

Before:
Screenshot 2022-01-07 16 54 37

After:
Screenshot 2022-01-07 16 54 41

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

  • line number needs different color
  • the bottom line of back trace is not correct

@paulbalandan
Copy link
Member Author

Sorry, I didn't see these comments when I force pushed my new changes.

  • For the change in last item, I am not sure where it changed, but seems that the calling function is duplicated as the last item (that's why I removed it now)
  • For the coloring, I opted to color the file:line instead to be the same with our CLI rendering

@paulbalandan
Copy link
Member Author

image

@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

The last line was fixed, from APPPATH/Controllers/Home.php:11 to FCPATH/index.php:37.
Before this PR, it was SYSTEMPATH/Commands/Server/rewrite.php:43, but we don't need it.

Screenshot 2022-01-07 17 30 38

@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

For the coloring, I opted to color the file:line instead to be the same with our CLI rendering

Okay, you changed the output, and it is acceptable.

@paulbalandan
Copy link
Member Author

Another quirk of PHP, providing DEBUG_BACKTRACE_IGNORE_ARGS ignores args for most but not require_once() args

1) CodeIgniter\Debug\Toolbar\Collectors\DatabaseTest::testDisplay
Failed asserting that an array does not have the key 'args'.

/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Debug/Toolbar/Collectors/DatabaseTest.php:58
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:97

@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

How about removing the first two lines?

  1    SYSTEMPATH/Events/Events.php:149
        CodeIgniter\Debug\Toolbar\Collectors\Database::collect()
  2    SYSTEMPATH/Database/BaseConnection.php:636
        CodeIgniter\Events\Events::trigger()

@paulbalandan
Copy link
Member Author

Why?

@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

These lines are DBQuery event trigger and always the same.

@kenjis
Copy link
Member

kenjis commented Jan 7, 2022

LGTM.

Screenshot 2022-01-08 5 05 19

Screenshot 2022-01-08 5 06 09

@lonnieezell
Copy link
Member

This looks good to me. Thanks everyone! Merging.

@lonnieezell lonnieezell merged commit 67d00b3 into codeigniter4:develop Jan 7, 2022
kenjis pushed a commit that referenced this pull request Jan 8, 2022
@paulbalandan paulbalandan deleted the backtrace-collector branch January 8, 2022 02:15
@kenjis kenjis mentioned this pull request Jan 8, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them hotfix refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: ErrorException: Undefined index: file: SYSTEMPATH\Debug\Toolbar\Collectors\Database.php at line 142
3 participants