Added ability to config the trace file and line number #162

Merged
merged 14 commits into from Nov 10, 2016

Projects

None yet

6 participants

@thiagotalma
Contributor
Q A
Is bugfix? no
New feature? no
Breaks BC? no
Tests pass? no
@dynasource
Member

this would be more consistent indeed. Can you add a link to this convention in this topic?

@bashkarev
Contributor
bashkarev commented Nov 7, 2016 edited

It will be more universal. + closes #144

[
    'modules' => [
        'debug' => [
            'class' => 'yii\debug\Module',
            'editor' => ['phpstorm://open?file={file}&line={line}', '{file}:{line}']
        ],
    ],
]
public function editorLink($trace)
{
    if ($this->editor === null) {
        return "{$trace['file']}:{$trace['line']}";
    }
    if (is_string($this->editor)) {
        return strtr($this->editor, ['{file}' => $trace['file'], '{line}' => $trace['line']]);
    }
    return Html::a(strtr($this->editor[0], ['{file}' => $trace['file'], '{line}' => $trace['line']]), strtr($this->editor[1], ['{file}' => $trace['file'], '{line}' => $trace['line']]));
}
@samdark
samdark approved these changes Nov 7, 2016 View changes
@samdark

Need a changelog.

@thiagotalma thiagotalma added this to the 2.0.7 milestone Nov 7, 2016
@samdark samdark was assigned by thiagotalma Nov 7, 2016
@thiagotalma
Contributor

@samdark,, I made the changes that colleagues asked. Please check again.

@thiagotalma thiagotalma changed the title from Show line number using the colon convention to Added ability to config the trace file and line number Nov 7, 2016
Panel.php
+ /**
+ * @param array $trace The log trace
+ *
+ * @return string the trace line
Panel.php
+ if (is_callable($this->module->traceLink)) {
+ return call_user_func($this->module->traceLink, $trace, $this);
+ }
+
@dynasource
dynasource Nov 7, 2016 Member

blank line may be removed.

@@ -91,6 +91,19 @@ class Module extends \yii\base\Module implements BootstrapInterface
* You may want to enable the debug logs if you want to investigate how the debug module itself works.
*/
public $enableDebugLogs = false;
+ /**
+ * @var mixed the string with placeholders to be be substituted or an anonymous function that returns the trace line string.
@dynasource
dynasource Nov 7, 2016 edited Member

its unclear how this string should look like?

@cebe
cebe Nov 8, 2016 Member

yes, an example would be good.

Panel.php
@@ -114,4 +114,22 @@ public function getUrl($additionalParams = null)
return Url::toRoute($route);
}
+
+ /**
+ * @param array $trace The log trace
@dynasource
dynasource Nov 7, 2016 Member

missing method description

@bashkarev
Contributor

What do you think?

$links = [
    'textmate' => 'txmt://open?url={file}&line={line}',
    'macvim' => 'mvim://open?url={file}&line={line}',
    'emacs' => 'emacs://open?url={file}&line={line}',
    'sublime' => 'subl://open?url={file}&line={line}',
    'phpstorm' => 'phpstorm://open?url={file}&line={line}',
];
[
    'modules' => [
        'debug' => [
            'class' => 'yii\debug\Module',
            'ide' =>'phpstorm'
        ],
    ],
]
views/default/panels/db/detail.php
@@ -98,7 +98,7 @@
function debug_db_detail() {
$('.db-explain a').on('click', function(e) {
e.preventDefault();
-
+
@dynasource
dynasource Nov 7, 2016 Member

are these lines added accidentally?

@samdark
samdark Nov 7, 2016 Member

It's automatic PhpStorm's removal of whitespace on blank lines. I agree that it should not be in this commit...

@dynasource

thanks for including the traceLink functionality. Looking forward to have this integrated with PhpStorm.

@samdark
Member
samdark commented Nov 7, 2016

@bashkarev this is interesting. Presets for common IDEs and editors could be very handy. Alternatively it could be put into readme.

@bashkarev
Contributor
bashkarev commented Nov 7, 2016 edited

please add options text.
see example for \yii\web\ErrorHandler.php

public function traceLink($file, $line)
{
    /**
     * @var $debug \yii\debug\Module
     */
    if (
        $file !== null
        && $line !== null
        && ($debug = Yii::$app->getModule('debug')) !== null
    ) {
        return $debug->traceLink([
            'file' => $file,
            'line' => $line,
            'text' => 'open file' //!!!
        ]);
    }
    return null;
}
@thiagotalma
Contributor

adjustments done!

views/default/panels/db/detail.php
- 'item' => function ($trace) {
- return "<li>{$trace['file']} ({$trace['line']})</li>";
+ 'item' => function ($trace) use ($panel) {
+ return '<li>' . $panel->traceLink($trace) . '</li>';
@bashkarev
bashkarev Nov 7, 2016 Contributor

if set 'traceLink' => 'phpstorm://open?url={file}&line={line}', this not link

@thiagotalma
thiagotalma Nov 7, 2016 Contributor

@bashkarev really! my bad....
fixed in README

+ ...
+];
+```
+
@samdark
samdark Nov 7, 2016 Member

I suggest putting common IDE strings here.

@thiagotalma
thiagotalma Nov 7, 2016 Contributor

@samdark In fact, there is no a list of possible settings per IDE.

The user is who sets the string when it does the setup in the operating system. There is a link that talks about this in the example I quoted: https://pla.nette.org/en/how-open-files-in-ide-from-debugger

@dynasource
dynasource Nov 7, 2016 Member

correct, shortly described at #144 (comment)

@bashkarev
bashkarev Nov 7, 2016 Contributor

example symfony

@bashkarev
Contributor

again for about option text

public function traceLink($options)
{
    if (!isset($options['text'])) {
        $options['text'] = "{$options['file']}:{$options['line']}";
    }
    if ($this->module->traceLink === null) {
        return $options['text'];
    }
    if (is_callable($this->module->traceLink)) {
        return call_user_func($this->module->traceLink, $options, $this);
    }
    if (is_string($this->module->traceLink)) {
        return strtr($this->module->traceLink, ['{file}' => $options['file'], '{line}' => $options['line'], '{text}' => $options['text']]);
    }
    return null;
}
$debug->traceLink(['file'=>'...','line'=>1,'text'=>'<svg></svg>'])
@thiagotalma
Contributor

@bashkarev finally, done! ;)

@dynasource
Member

alright, are you sure everything is working correctly? Thats 'traceLink' method has reached a certain level of complexity that it deserves some unit tests.

@thiagotalma
Contributor

Thats 'traceLink' method has reached a certain level of complexity that it deserves some unit tests.

@bashkarev you can help with tests?

@dynasource dynasource locked and limited conversation to collaborators Nov 9, 2016
@dynasource dynasource dismissed their review Nov 9, 2016

not ready yet

@cebe cebe unlocked this conversation Nov 9, 2016
@dynasource
Member

alright. I've commited some fixes & tests.

It also tried to integrate it with PhpStorm on a windows machine. That was quite some work and the good news is thats its working. You need several patches for that:

  • install a url protocol to make the links work in the browser
  • create the correct links with files & line numbers
  • use a bat file to link to PhpStorm
  • without having any CMD windows left open

Took a few hours ;) but it works now. Unfortunately yet only with files. With the line numbers, I do not see any activity in the PhpStorm. Its very likely a bug on their side.

@samdark
Member
samdark commented Nov 9, 2016

They have an issue tracker: http://youtrack.jetbrains.net/issues/WI

@thiagotalma
Contributor
thiagotalma commented Nov 9, 2016 edited

@dynasource I got it with a minimum of work.
I've created a gist for you to try to use, just copy the files to the "C:\Program Files\PhpStorm Protocol (Win)" folder and run .reg

https://gist.github.com/thiagotalma/fa5a54e1e34133f78f4538eb96c87ae8

EDIT:
Change the .js file according to the configuration of your machine.
This solution was taken from the link I included in the README.

@dynasource
Member
dynasource commented Nov 10, 2016 edited

my solution is a little different and smaller. Its uses a bat file so you only have to put it to the registry once (and change the bat file if PhpStorm upgrades):

Windows Registry Editor Version 5.00

[HKEY_CLASSES_ROOT\phpstorm]
@="\"URL:phpstorm Protocol\""
"URL Protocol"=""

[HKEY_CLASSES_ROOT\phpstorm\shell]

[HKEY_CLASSES_ROOT\phpstorm\shell\open]

[HKEY_CLASSES_ROOT\phpstorm\shell\open\command]
@="\"C:\\Program Files (x86)\\JetBrains\\phpstorm.bat\" %1"

Custom bat file 'phpstorm.bat' pointing that one of PhpStorm:

Start /b "" "C:\Program Files (x86)\JetBrains\PhpStorm 2016.2.1\bin\PhpStorm.bat" %2 --line %3
@dynasource
Member
dynasource commented Nov 10, 2016 edited

@dynasource I got it with a minimum of work.

This was time consuming:

  • trying to fix it with file:// and a chrome extension, but this seems to be restricted anno 2016
  • Fix the bat file to be headless
  • the line is not processed by PhpStorm 2016.2.2
@dynasource
Member

nice. You have to do --line before the file argument. Now it works fine!

@dynasource
Member

@thiagotalma, I tried your WScript. Its better because its headless and faster. Had to modify it to make it work ;) :

  • The x64 is reversed
Module.php
@@ -25,7 +25,7 @@
*/
class Module extends \yii\base\Module implements BootstrapInterface
{
- const TRACELINK_PHPSTORM_WINDOWS = '<a href="phpstorm:// {file} {line}">{file}:{line}</a>';
+ const TRACELINK_PHPSTORM = '<a href="phpstorm://open?url=file://{file}&line={line}">{file}:{line}</a>';
@bashkarev
bashkarev Nov 10, 2016 Contributor
const TRACELINK_PHPSTORM = '<a href="phpstorm://open?url=file://{file}&line={line}">{text}</a>';
Module.php
@@ -25,6 +25,8 @@
*/
class Module extends \yii\base\Module implements BootstrapInterface
{
+ const TRACELINK_PHPSTORM = '<a href="phpstorm://open?url=file://{file}&line={line}">{text}</a>';
@samdark
samdark Nov 10, 2016 Member

Why is it called PHPSTORM if the link is the same for all IDEs?

Panel.php
+ }
+ if ($this->module->traceLink === null) {
+ return $options['text'];
+ }else{
@samdark
samdark Nov 10, 2016 Member

Formatting!

Panel.php
+ if ($this->module->traceLink === null) {
+ return $options['text'];
+ }else{
+ $options['file'] = str_replace('\\','/',$options['file']);
@samdark
samdark Nov 10, 2016 Member

Formatting!

docs/guide/installation.md
+Wouldn't it be nice to be able to open files directly from the debug trace?
+
+Well, you can!
+With a few settings and you're ready to go!
@samdark
samdark Nov 10, 2016 Member

No need for and here.

tests/bootstrap.php
@@ -8,8 +8,13 @@
$_SERVER['SCRIPT_NAME'] = '/' . __DIR__;
$_SERVER['SCRIPT_FILENAME'] = __FILE__;
-require_once(__DIR__ . '/../vendor/autoload.php');
-require_once(__DIR__ . '/../vendor/yiisoft/yii2/Yii.php');
+if(is_dir(__DIR__ . '/../vendor/')){
@samdark
samdark Nov 10, 2016 Member

Formatting!

@samdark
samdark Nov 10, 2016 Member

Why is such condition needed?

@dynasource
dynasource Nov 10, 2016 Member

please read the next lines

dynasource added some commits Nov 10, 2016
@dynasource dynasource - changed to a default IDE protocol in the CONST
- renamed to traceLine
- set traceLine to the CONST as default
- fixed a false setting showing a textual line
e6500f9
@dynasource dynasource fix doc 606da5b
@dynasource dynasource guide updates 71ce739
@dynasource dynasource merged commit ed07955 into yiisoft:master Nov 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dynasource
Member

thanks to you all. Great work.

@thiagotalma thiagotalma deleted the thiagotalma:lineNumber branch Nov 10, 2016
@ilgiz-badamshin ilgiz-badamshin referenced this pull request in bedezign/yii2-audit Nov 29, 2016
Merged

Fix dbPanel errors #180

@ilgiz-badamshin

Windows

What about instruction for unix systems?

@cebe
Member
cebe commented Nov 29, 2016

@ilgiz-badamshin just found this: http://www.tuicool.com/articles/ANBBVj looks like it could be used as a basis for writing linux instructions in the docs. Would you be willing to write a guide and send a pull request?

@cebe
Member
cebe commented Nov 29, 2016

created an issue for this: #172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment