Change editor mapping to support multiple changes to the same substring

[adrianbj]

Sorry, I didn’t think about the scenario of wanting make multiple changes to a substring. Of course strtr fails at this. This version allows these sorts of changes. In my scenario I can now change from the compiled version to the source version, as well as map production server URLs to the local dev source.

Hopefully you are happy with these changes.

[JanTvrdik]

What about

$file = str_replace(array_keys(Debugger::$editorMapping), array_values(Debugger::$editorMapping), $file));

[adrianbj]

Yeah - that's what I was originally planning on doing, but I thought I should follow the approach that @dg took yesterday when he implemented substr_replace.

I am certainly happy to go with the array_keys / array_values approach though - it's certainly less code.

Would you like me to modify the PR?

[dg]

Whats wrong with strtr()?

[adrianbj]

strtr won't replace the same substring again once it has been modified, so subsequent rules in the array are ignored.

My fault for not thinking through the possibility of wanting to make multiple adjustments to the same parts of a path.

[dg]

I fear that multiple substitutions in one string can cause problems elsewhere. Is it really necessary?

[adrianbj]

Well in my use case it is definitely necessary. As I mentioned I absolutely have to modify the path from the compiled version to the source version. The editor link is no use at all without this.

Then in addition I want to be able to modify the links from the production server path to the local dev path so that if an error shows up on a production site, there is an easy link to the same file on your local dev machine (or to a mapped path to the production server if you like to work that way).

What problems do you foresee ?

[dg]

I need to know exactly what do you want, more specifically I need to have a test, because otherwise I can not guarantee that in the future it will behave as you need.

[adrianbj]

@dg - it's pretty simple - I just need the ability to make more than one replacement on the path. This PR takes care of that and is working fine here. @JanTvrdik's suggestion of using array_keys() and array_values() is cleaner in my mind, but I wanted to match your coding style from the previous commit you made, which is why I went with the loop approach.

I can try to put a test together for this, but I don't really have any time this weekend and am on vacation for the next two weeks.

[dg]

The test would be great.

[adrianbj]

Ok, I haven't tested this test, but based on my reading about Assert, this should do the trick:

<?php
/**
 * Test: Tracy\Debugger::editorMapping()
 */
use Tracy\Debugger;
use Tracy\Helpers;
use Tester\Assert;

require __DIR__ . '/../bootstrap.php';

Debugger::$editorMapping = array(
    '/site/assets/cache/FileCompiler' => '',
    '/home/public_html/mysite/' => '/Users/me/sites/mysite/'
);

$url = '/home/public_html/mysite/site/assets/cache/FileCompiler/site/templates/home.php&line=7';

Assert::same('/Users/me/sites/mysite/site/templates/home.php&line=7', Helpers::$editorMapping($url));

This is based on the new Helpers::$editorMapping method in this PR. Of course I would be happy if:

    public static function editorMapping($file)
    {
        foreach (Debugger::$editorMapping as $k => $v) {
            $file = str_replace($k, $v, $file);
        }
        return $file;
    }

is changed to:

    public static function editorMapping($file)
    {
        return str_replace(array_keys(Debugger::$editorMapping), array_values(Debugger::$editorMapping), $file));
    }

[adrianbj]

PS - maybe it's confusing having the Debugger::$editorMapping variable and the Helpers::$editorMapping() method having the same name?

[adrianbj]

Hi @dg - any thoughts on this change and the test?

Thanks!

[dg]

I still don't know what you exactly want. What are your source paths, destination paths and your mapping. Create (failng) test file and it becomes clear.

[adrianbj]

@dg - sorry I don't know what else to tell you - in the test above (#176 (comment)) you can see the exact mapping and you can compare $url with the first argument of Assert::same which shows the source and destination paths I need.

I don't think I really know how to create a failing test for this - sorry I am new to Nette and am only familiar with Tracy, not the rest of the framework. If the test I wrote works, then I think everything is fine. I am using the code from this PR and it's working perfectly. As I mentioned, the problem with my initially proposed strtr() won't allow multiple changes to the same substring. This new approach of simple str_replace - either via foreach, or array_keys / array_values allows the user to define as many replacements as they need. Surely if they fail to match, then it's their fault, not the fault of this mapping method?

Sorry if I am still not understanding what you need.

[dg]

Perhaps we do not understand :-) I'm not asking you to learn Tester, I am just trying to understand your use case.

This URL

$url = '/home/public_html/mysite/site/assets/cache/FileCompiler/site/templates/home.php&line=7';

can be changed to this

$dest = '/Users/me/sites/mysite/site/templates/home.php&line=7';

with this mapping:

Debugger::$editorMapping = array(
    '/home/public_html/mysite/site/assets/cache/FileCompiler/' =>  '/Users/me/sites/mysite/',
);

I simply can't understand why you want to split it into more replacements.

[adrianbj]

I simply can't understand why you want to split it into more replacements.

Because it think it should be flexibly enough to handle any scenario a user may want to throw at it.

Also, in the case of my example, I don't know if the /site/assets/cache/FileCompiler component of the path is going to exist or not because not all files are always in the compiled path, so without knowing the initial value of $file it is hard to determine what the final src and dest paths would be. It might be possible for some user's situations, but I think it will be difficult/impossible in my case. This is why the option for multiple replacements is easier and I think more powerful.

[dg]

Also, in the case of my example, I don't know if the /site/assets/cache/FileCompiler component of the path is going to exist or not

So what about this?

Debugger::$editorMapping = array(
    '/home/public_html/mysite/site/assets/cache/FileCompiler/' =>  '/Users/me/sites/mysite/',
    '/home/public_html/mysite/' =>  '/Users/me/sites/mysite/',
);

IMHO it is perfectly readable, understandable and expectable.

I can improve editorMapping() that order of rules will not matter.

[adrianbj]

The problem with that version is that it doesn't work on your local dev setup - it only works to make links from remote production server map to local. It doesn't handle mapping of local dev compiled directory to the local dev src file.

[dg]

And this?

Debugger::$editorMapping = array(
    '/home/public_html/mysite/site/assets/cache/FileCompiler/' =>  '/Users/me/sites/mysite/',
    '/home/public_html/mysite/' =>  '/Users/me/sites/mysite/',
    '/Users/me/sites/mysite/site/assets/cache/FileCompiler/' =>  '/Users/me/sites/mysite/',
);

[adrianbj]

I expect that strtr() will work with that (will need to test to be certain), but it seems way more complicated than just going with str_replace() on the array key value pairs. I feel like I don't really understand the reason to avoid str_replace for mapping.

[dg]

Multiple replacements may lead to unwanted results. Yes, it is unlikely that it will happen, if you have long path names (i.e long keys in $editorMapping), but these things may happen and there is no way how to solve it (ie. how to say „make only one replacement and finish“). Maybe I am afraid unnecessarily, but still I find it better to avoid it by design and use more explicit mappings.

[adrianbj]

Multiple replacements may lead to unwanted results.

Ok, fair enough - better to be safe than sorry I guess.

By automatically building up the array of replacements modified from your suggestion here (#176 (comment)), I have something that now works with strtr(), so all good.

Thanks for spending the time to work through this.