Skip to content

Conversation

@VasekPurchart
Copy link
Contributor

Right now there is possibility to configure collapsed paths in BlueScreen with BlueScreen::$collapsePaths. Since the name of the property contains "paths" I think all paths should be allowed.

But because in the implementation a / is added at the end, file paths never match. Adding / at the end is fine (actually necessary, because otherwise there will be false positives for dirs - I have added a test for this). With files there is of course similar issue with false positives, so matching must be done "exactly" (not starts with).

So implementations for dirs and files must differ and I've not come up with a different solution (without an API change) other than checking the actual given path if it is a file or directory. I hope that should not be a problem.

@JanTvrdik
Copy link
Contributor

This will result in multiple disk IO operations for every NDB query when ConnectionPanel is used https://github.com/nette/database/blob/187df045a31a22b2d1871e300f819ebada208799/src/Bridges/DatabaseTracy/ConnectionPanel.php#L58

@JanTvrdik
Copy link
Contributor

Will this work?

$file = strtr($file, '\\', '/'); foreach ($this->collapsePaths as $path) { $path = strtr($path, '\\', '/'); $length = strlen($path); if (strncmp($file, $path, $length) === 0 && (!isset($file[$length]) || $file[$length] === '/')) { return TRUE; } } return FALSE;

Another rewrite (PHP compares first lengths of strings -> the char by char comparison is done only once)

$file = strtr($file, '\\', '/'); foreach ($this->collapsePaths as $path) { $path = strtr($path, '\\', '/'); if ($file === $path || strncmp($file, "$path/", strlen($path) + 1) === 0) { return TRUE; } } return FALSE;
@VasekPurchart VasekPurchart force-pushed the collapse-paths-files branch from 9a5274e to cbbc3c2 Compare July 8, 2015 08:40
@VasekPurchart
Copy link
Contributor Author

This will result in multiple disk IO operations

That is not necessarily true, because of the stat cache and since these are paths from where source codes were probably loaded (if not using opcache), the information might have already been loaded.

Will this work?

But of course if we can avoid the chance, it's better. I think it will work (I can't come up with a situation where your solution won't work), so I updated the PR, thanks! 👍

@dg
Copy link
Member

dg commented Jul 8, 2015

What about simple if (strncmp("$file/", "$path/", strlen($path) + 1) === 0) {

@VasekPurchart
Copy link
Contributor Author

Yes, that would work too, but I think current solution is more readable, so it is up to you, if you want to do this microoptimalization.

@JanTvrdik
Copy link
Contributor

@dg 👍

@dg dg force-pushed the master branch 2 times, most recently from e8be759 to 13f9a7b Compare July 12, 2015 10:12
@dg dg closed this Jul 12, 2015
@dg
Copy link
Member

dg commented Jul 12, 2015

(possible BC break: when path in $collapsePaths contains trailing /)

@VasekPurchart
Copy link
Contributor Author

With the submitted

 if ($file === $path || strncmp($file, "$path/", strlen($path) + 1) === 0) {

version, this wouldn't be the issue, would it? Is the "faster" version worth it?

@dg
Copy link
Member

dg commented Jul 12, 2015

Sry, it is not BC break.

@VasekPurchart
Copy link
Contributor Author

Yea, you are right :) Thanks for merging!

@VasekPurchart VasekPurchart deleted the collapse-paths-files branch July 13, 2015 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants